Alon Bar-Lev has posted comments on this change. Change subject: ovirt-live: migrate ovirt live plugin to otopi ......................................................................
Patch Set 5: (15 comments) .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/__init__.py Line 29: Line 30: @util.export Line 31: def createPlugins(context): Line 32: core.Plugin(context=context) Line 33: two spaces .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/core.py Line 32: from otopi import filetransaction Line 33: from otopi import constants as otopicons Line 34: Line 35: Line 36: import oliveconst I suggest move this to ovirt_engine_setup Line 37: Line 38: from ovirt_engine_setup import constants as osetupcons Line 39: from ovirt_engine_setup import dialog Line 40: Line 33: from otopi import constants as otopicons Line 34: Line 35: Line 36: import oliveconst Line 37: two spaces Line 38: from ovirt_engine_setup import constants as osetupcons Line 39: from ovirt_engine_setup import dialog Line 40: Line 41: Line 53: stage=plugin.Stages.STAGE_INIT, Line 54: ) Line 55: def _init(self): Line 56: self.environment.setdefault( Line 57: oliveconst.OVIRTLIVEEnv.ENABLE, OVIRTLIVEEnv? Line 58: True Line 59: ) Line 60: Line 61: @plugin.event( Line 82: import ovirtsdk.api Line 83: from ovirtsdk.xml import params Line 84: Line 85: self._ovirtsdk_api = ovirtsdk.api Line 86: self._ovirtsdk_xml = ovirtsdk.xml I suggest to put the above in validation, so we fail if missing before doing harm. Line 87: engine_api = self._ovirtsdk_api.API( Line 88: url='https://{fqdn}:{port}/api'.format( Line 89: fqdn=self.environment[osetupcons.ConfigEnv.FQDN], Line 90: port=self.environment[osetupcons.ConfigEnv.PUBLIC_HTTPS_PORT], Line 105: ).storagedomains.add( Line 106: self.environment[ Line 107: oliveconst.ConfigEnv.DEFAULT_ISO_NAME, Line 108: ] Line 109: ) just to make sure... usually it is logical that add method returns the object... so you can do something: xxxxx.add('name').activate() :) Line 110: engine_api.datacenters.get( Line 111: self.environment[ Line 112: oliveconst.ConfigEnv.DEFAULT_LOCAL_DATA_CENTER, Line 113: ] Line 126: ), Line 127: ) Line 128: def _copyiso(self): Line 129: self.logger.debug('Copying Iso Files') Line 130: fileList = glob.glob("/home/oVirtuser/oVirtLiveFiles/iso/*.iso") single quotes please Line 131: targetPath = os.path.join( Line 132: self.environment[osetupcons.ConfigEnv. Line 133: ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT], Line 134: self.environment[osetupcons.ConfigEnv. Line 132: self.environment[osetupcons.ConfigEnv. Line 133: ISO_DOMAIN_DEFAULT_NFS_MOUNT_POINT], Line 134: self.environment[osetupcons.ConfigEnv. Line 135: ISO_DOMAIN_SD_UUID], Line 136: "images", "11111111-1111-1111-1111-111111111111") please reformat above. for the 11111111 you have Const.ISO_DOMAIN_IMAGE_UID Line 137: for filename in fileList: Line 138: shutil.move(filename, targetPath) Line 139: os.chown( Line 140: os.path.join(targetPath, filename), Line 147: self.environment[ Line 148: osetupcons.ConfigEnv.DEFAULT_SYSTEM_GROUP_KVM, Line 149: ], Line 150: ) Line 151: ) why can't we use our own code that does the same? or I miss something. Line 152: Line 153: @plugin.event( Line 154: stage=plugin.Stages.CLOSEUP, Line 155: condition=lambda self: self._enabled, Line 161: def _createvm(self): Line 162: from ovirtsdk.xml import params Line 163: self.logger.debug("Creating VM") Line 164: Line 165: engine_api = self._ovirtsdk_api.API( why don't you store the engine_api as member and reuse it? Line 166: url='https://{fqdn}:{port}/api'.format( Line 167: fqdn=self.environment[ Line 168: osetupcons.ConfigEnv.FQDN, Line 169: ], Line 193: engine_api.vms.add( Line 194: params.VM( Line 195: name='local_vm', Line 196: memory='1024MB', Line 197: os=os, cluster=engine_api.clusters.get('local_cluster'), cluster at own line Line 198: template=engine_api.templates.get('Blank'), Line 199: ) Line 200: ) Line 201: Line 213: diskParam = params.Disk( Line 214: storage_domains=params.StorageDomains( Line 215: storage_domain=[ Line 216: engine_api.storagedomains.get( Line 217: 'local_storage') move the 'local_strage') up Line 218: ] Line 219: ), Line 220: size='6000MB', Line 221: type_='data', Line 217: 'local_storage') Line 218: ] Line 219: ), Line 220: size='6000MB', Line 221: type_='data', type_ ? Line 222: interface='virtio', Line 223: format='cow', Line 224: bootable=True, Line 225: alias='local_disk', Line 234: diskParam Line 235: ) Line 236: ) Line 237: Line 238: two spaces Line 239: .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/oliveconst.py Line 1: @util.export please put license and stuff. missing import of utils at least... Line 2: class Stages(object): Line 3: CONFIG_STORAGE = 'ovirtlivesetup.core.core.configstorage' Line 4: COPY_ISO = 'ovirtlivesetup.core.copy.iso' Line 5: CREATE_VM = 'ovirtlivesetup.core.create.vm' -- 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: 5 Gerrit-Project: ovirt-live Gerrit-Branch: master Gerrit-Owner: Ohad Basan <[email protected]> Gerrit-Reviewer: Alex Lourie <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: David Caro <[email protected]> Gerrit-Reviewer: Eyal Edri <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Moran Goldboim <[email protected]> Gerrit-Reviewer: Ofer Schreiber <[email protected]> Gerrit-Reviewer: Ohad Basan <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
