David Caro has posted comments on this change. Change subject: ovirt-live: migrate ovirt live plugin to otopi ......................................................................
Patch Set 3: (9 inline comments) .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/core.py Line 23: import os Line 24: import shutil Line 25: import glob Line 26: import gettext Line 27: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-engine-setup') If able, is recommended to use def instead of assigning the lambda to a variable: http://www.python.org/dev/peps/pep-0008/#programming-recommendations Line 28: Line 29: Line 30: from otopi import util Line 31: from otopi import plugin Line 95: ), Line 96: password=self.environment[osetupcons.ConfigEnv.ADMIN_PASSWORD], Line 97: ca_file=osetupcons.FileLocations.OVIRT_ENGINE_PKI_ENGINE_CA_CERT, Line 98: ) Line 99: engine_version = self._ovirtsdk_xml.params.Version( Is this is used anywhere? Line 100: major=self._version[0], Line 101: minor=self._version[1], Line 102: ) Line 103: self.logger.debug('Creating the local data storage domain') Line 116: storage=stParams, Line 117: type='data' Line 118: ) Line 119: Line 120: engine_api.storagedomains.add(sdParams) Why do not use Alon's recommendations and get rid of the extra useless unused randomly named variable? engine_api.storagedomains.add( params.StorageDomain( name='local_storage', data_center=engine_api.datacenters.get( self.environment[ oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER ] ), storage_format='v3', host=engine_api.hosts.get('local_host'), storage=stParams, type='data', ) ) Line 121: Line 122: engine_api.datacenters.get( Line 123: self.environment[oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER] Line 124: ).storagedomains.add( Line 143: ISO_DOMAIN_SD_UUID], Line 144: "images", "11111111-1111-1111-1111-111111111111") Line 145: for filename in fileList: Line 146: shutil.move(filename, targetPath) Line 147: filename = os.path.join(targetPath, filename) You can get rid of this variable too: os.chown( os.path.join(targetPath, filename), self.environment[ osetupcons.ConfigEnv.DEFAULT_SYSTEM_USER_VDSM ], self.environment[ osetupcons.ConfigEnv.DEFAULT_SYSTEM_GROUP_KVM ], ) Line 148: os.chown( Line 149: filename, self.environment[osetupcons. Line 150: ConfigEnv.DEFAULT_SYSTEM_USER_VDSM], Line 151: self.environment[osetupcons.ConfigEnv.DEFAULT_SYSTEM_GROUP_KVM] Line 175: ), Line 176: password=self.environment[osetupcons.ConfigEnv.ADMIN_PASSWORD], Line 177: ca_file=osetupcons.FileLocations.OVIRT_ENGINE_PKI_ENGINE_CA_CERT, Line 178: ) Line 179: engine_version = self._ovirtsdk_xml.params.Version( This one does not seem to be used neither. Line 180: major=self._version[0], Line 181: minor=self._version[1], Line 182: ) Line 183: Line 181: minor=self._version[1], Line 182: ) Line 183: Line 184: # Defins OS param for the boot option Line 185: os = params.OperatingSystem( Avoid defining vars with the same name as modules you import. You can also get rid of it as before: engine_api.vms.add( params.VM(name='local_vm', memory='1024MB', os=params.OperatingSystem( type_='unassigned', boot=[ params.Boot(dev='cdrom'), params.Boot(dev='hd'), ] ), cluster=engine_api.clusters.get('local_cluster'), template=engine_api.templates.get('Blank'), ), ) Line 186: type_='unassigned', Line 187: boot=[params.Boot(dev='cdrom'), Line 188: params.Boot(dev='hd')]) Line 189: Line 198: engine_api.vms.get( Line 199: 'local_vm').nics.add( Line 200: params.NIC(name='eth0', Line 201: network=params.Network(name='ovirtmgmt'), Line 202: interface='virtio')) Use the same format for the chained calls here too. Line 203: Line 204: sd = engine_api.storagedomains.get('local_storage') Line 205: diskParam = params.Disk( Line 206: storage_domains=params.StorageDomains(storage_domain=[sd]), Line 201: network=params.Network(name='ovirtmgmt'), Line 202: interface='virtio')) Line 203: Line 204: sd = engine_api.storagedomains.get('local_storage') Line 205: diskParam = params.Disk( Another variable you can get rid of. Line 206: storage_domains=params.StorageDomains(storage_domain=[sd]), Line 207: size='6000MB', Line 208: type_='data', Line 209: interface='virtio', Line 213: Line 214: engine_api.disks.add(diskParam) Line 215: engine_api.vms.get( Line 216: 'local_vm').disks.add( Line 217: engine_api.disks.get(alias='local_disk')) Please use the same format you used before when chaining calls: engine_api.vms.get( 'local_vm' ).disks.add( engine_api.disks.get(alias='local_disk') ) Line 218: Line 219: -- To view, visit http://gerrit.ovirt.org/17518 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I703f64dc1183a6fe176d9d0352f93de381d906bb Gerrit-PatchSet: 3 Gerrit-Project: ovirt-live Gerrit-Branch: master Gerrit-Owner: Ohad Basan <oba...@redhat.com> Gerrit-Reviewer: Alex Lourie <alou...@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: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com> Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com> Gerrit-Reviewer: Ohad Basan <oba...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
