Alon Bar-Lev has posted comments on this change.
Change subject: ovirt-live: prepare ovirt-live for ovirt 3.3
......................................................................
Patch Set 1:
(16 comments)
....................................................
File centos/build.sh
Line 1
why bash?
Line 10
Line 11
Line 12
Line 13
Line 14
BTW: I would have probably written that as Makefile
Line 1: #!/bin/bash
Line 2: if [[ ! $(whoami) == "root" ]]; then
should be:
if [ $(id -u) != 0 ]; then
...
fi
1. please avoid bash [[ ]]
2. please avoid using user name
Line 3: echo "Please run as root"
Line 4: exit 1
Line 5: fi
Line 6: rpm -q livecd-tools > /dev/null
Line 3: echo "Please run as root"
Line 4: exit 1
Line 5: fi
Line 6: rpm -q livecd-tools > /dev/null
Line 7: if [[ $? -ne 0 ]]; then
space
and should be:
rpm -q livecd-tools > /dev/null || yum install -y livecd-tools
or:
if ! rpm -q livecd-tools > /dev/null; then
yum install -y livecd-tools
fi
Line 8: yum install -y livecd-tools
Line 9: fi
Line 10: #wget -N
http://distro.ibiblio.org/tinycorelinux/4.x/x86/release/TinyCore-current.iso
Line 11: if [[ ! -d oVirtLiveFiles/rpms/ ]]; then
Line 12: mkdir oVirtLiveFiles/rpms/
Line 13: fi
Line 14: if [[ ! -d oVirtLiveFiles/iso/ ]]; then
Line 15: mkdir oVirtLiveFiles/iso/
Line 16: fi
no need for the above...
mkdir -p oVirtLiveFiles/rpms oVirtLiveFiles/iso
will always work
Line 17: wget -N
http://kojipkgs.fedoraproject.org//packages/yad/0.14.2/1.fc14/x86_64/yad-0.14.2-1.fc14.x86_64.rpm
Line 18: mv -f *.rpm oVirtLiveFiles/rpms
Line 19: mv -f *.iso oVirtLiveFiles/iso
Line 20: PWD=$(pwd)
Line 18: mv -f *.rpm oVirtLiveFiles/rpms
Line 19: mv -f *.iso oVirtLiveFiles/iso
Line 20: PWD=$(pwd)
Line 21: sed -i '/name=local/d' kickstart/ovirt-live-base.ks
Line 22: awk -v n=23 -v s="repo \-\-name=local
\-\-baseurl=file://${PWD}/oVirtLiveFiles\/rpms\/" 'NR == n {print s} {print}'
kickstart/ovirt-live-base.ks > kickstart/ovirt-live-base.temp
this is bad...
1. Rename kickstart/ovirt-live-base.ks to kickstart/ovirt-live-base.ks.in
within source tree.
2. Put place holders at the form of @XXXX@ within that template file.
3. Use sed -e 's#@XXX@#value#g' -e 's#...#g' kickstart/ovirt-live-base.ks.in >
kickstart/ovirt-live-base.ks
Line 23: mv kickstart/ovirt-live-base.temp kickstart/ovirt-live-base.ks
Line 24: rm -f kickstart/ovirt-live-base.temp
....................................................
File centos/oVirtLiveFiles/ovirt-answer
why not put this in filesystem structure?
Line 1: [environment:default]
Line 2: OSETUP_RPMDISTRO/enableUpgrade=bool:False
Line 3: OVESETUP_CORE/engineStop=none:None
Line 4: OVESETUP_DIALOG/confirmSettings=bool:True
....................................................
File
centos/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/core.py
Line 33: from otopi import filetransaction
Line 34: from otopi import constants as otopicons
Line 35:
Line 36:
Line 37: import oliveconst
please move this after the from ovirt_engine_setup import block
Line 38:
Line 39:
Line 40: from ovirt_engine_setup import util as osetuputil
Line 41: from ovirt_engine_setup import constants as osetupcons
Line 157: 'images',
Line 158: osetupcons.Const.ISO_DOMAIN_IMAGE_UID
Line 159: )
Line 160: self.logger.debug('target path' + targetPath)
Line 161: for filename in fileList:
?
for filename in glob.glob('/home/liveuser/oVirtLiveFiles/iso/*.iso'):
Line 162: self.logger.debug(filename)
Line 163: shutil.move(filename, targetPath)
Line 164: os.chown(
Line 165: os.path.join(targetPath, os.path.basename(filename)),
Line 168: ),
Line 169: osetuputil.getGid(
Line 170: osetupcons.Defaults.DEFAULT_SYSTEM_GROUP_KVM
Line 171: )
Line 172: )
best to use FileTransaction instead of you put files and chown. This should be
done at MISC stage.
Line 173:
Line 174: @plugin.event(
Line 175: stage=plugin.Stages.STAGE_CLOSEUP,
Line 176: condition=lambda self: self._enabled,
Line 192: MB = 1024*1024
Line 193: GB = 1024*MB
Line 194:
Line 195: # Create VM
Line 196: vm = self._engine_api.vms.add(
why do you need vm variable?
Line 197: params.VM(
Line 198: name='local_vm',
Line 199: memory=1*GB,
Line 200: os=os,
Line 196: vm = self._engine_api.vms.add(
Line 197: params.VM(
Line 198: name='local_vm',
Line 199: memory=1*GB,
Line 200: os=os,
?
os=params.OperatingSystem(
type_='unassigned',
boot=(
....
)
),
Line 201:
cluster=self._engine_api.clusters.get('local_cluster'),
Line 202: template=self._engine_api.templates.get('Blank'),
Line 203: ),
Line 204: )
Line 226: )
Line 227:
Line 228: self._engine_api.vms.get(
Line 229: 'local_vm'
Line 230: ).disks.add(diskParam)
?
disk.add(
parms.Disk(
....
)
)
Line 231:
Line 232:
....................................................
File
centos/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/oliveconst.py
Line 16: #
Line 17:
Line 18: import gettext
Line 19: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-engine-setup')
Line 20:
two spaces
Line 21: from otopi import util
Line 22: from ovirt_engine_setup import constants as osetupcons
Line 23:
Line 24:
Line 63: DEFAULT_LOCAL_CLUSTER = 'local_cluster'
Line 64: DEFAULT_LOCAL_HOST = 'local_host'
Line 65: DEFAULT_STORAGE_DOMAIN_NAME = 'local_storage'
Line 66: DEFAULT_ISO_NAME = 'ISO_DOMAIN'
Line 67:
two spaces
Line 68: @util.export
Line 69: @util.codegen
Line 70: @osetupcons.osetupattrsclass
Line 71: class IsoEnv(object):
Line 69: @util.codegen
Line 70: @osetupcons.osetupattrsclass
Line 71: class IsoEnv(object):
Line 72: ISO_NAME = 'ISO_DOMAIN'
Line 73:
two spaces
Line 74: @util.export
Line 75: @util.codegen
Line 76: class Const(object):
Line 77: MINIMUM_SPACE_STORAGEDOMAIN_MB = 10240
--
To view, visit http://gerrit.ovirt.org/19200
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3930256ebddb4eed912d216922861f7593d93dea
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-live
Gerrit-Branch: master
Gerrit-Owner: Ohad Basan <oba...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: David Caro <dcaro...@redhat.com>
Gerrit-Reviewer: Eyal Edri <ee...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Ohad Basan <oba...@redhat.com>
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
