Alon Bar-Lev has posted comments on this change. Change subject: ovirt-live: migrate ovirt live plugin to otopi ......................................................................
Patch Set 3: (6 inline comments) .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/core.py Line 165: Line 166: engine_api = self._ovirtsdk_api.API( Line 167: url='https://{fqdn}:{port}/api'.format( Line 168: fqdn=self.environment[osetupcons.ConfigEnv.FQDN], Line 169: port=self.environment[osetupcons.ConfigEnv. please? Line 170: DEFAULT_NETWORK_HTTPS_PORT], Line 171: ), Line 172: username='{user}@{domain}'.format( Line 173: user=osetupcons.Const.USER_ADMIN, Line 166: engine_api = self._ovirtsdk_api.API( Line 167: url='https://{fqdn}:{port}/api'.format( Line 168: fqdn=self.environment[osetupcons.ConfigEnv.FQDN], Line 169: port=self.environment[osetupcons.ConfigEnv. Line 170: DEFAULT_NETWORK_HTTPS_PORT], what is default network? why do you use default? Line 171: ), Line 172: username='{user}@{domain}'.format( Line 173: user=osetupcons.Const.USER_ADMIN, Line 174: domain=osetupcons.Const.DOMAIN_INTERNAL, .................................................... File fedora/oVirtLiveFiles/root/usr/share/ovirt-engine/setup/plugins/ovirt-engine-setup/olive/oliveconst.py Line 6: @util.export Line 7: class Stages(object): Line 8: OVIRTLIVE_CONFIG_STORAGE = 'ovirtlivesetup.core.core.configstorage' Line 9: OVIRTLIVE_COPY_ISO = 'ovirtlivesetup.core.copy.iso' Line 10: OVIRTLVE_CREATE_VM = 'ovirtlivesetup.core.create.vm' spelling.... And... there is no need for OVIRTLIVE prefix if within own module... as you have olivecons.Stages.XXXX so no need live twice. Line 11: Line 12: Line 13: @util.export Line 14: @util.codegen Line 12: Line 13: @util.export Line 14: @util.codegen Line 15: @osetupattrsclass Line 16: class OVIRTLIVEEnv(object): same here (I think) you can drop the prefix... but if you want OvirtLive is better then all capitals. I would have put CoreEnv... Line 17: ENABLE = 'OVESETUP_OVIRTLIVE/enable' Line 18: Line 19: CONFIGURE = 'OVESETUP_OL/configure' Line 20: LOCAL_DATA_CENTER = 'OVESETUP_AIO/localDataCenter' Line 36: STORAGE_DOMAIN_NAME = 'OVESETUP_AIO/storageDomainName' Line 37: Line 38: Line 39: @util.export Line 40: class OVIRTLIVEDefaults(object): Defaults no prefix Line 41: DEFAULT_LOCAL_DATA_CENTER = 'local_datacenter' Line 42: DEFAULT_LOCAL_CLUSTER = 'local_cluster' Line 43: DEFAULT_LOCAL_HOST = 'local_host' Line 44: DEFAULT_STORAGE_DOMAIN_NAME = 'local_storage' Line 46: Line 47: Line 48: @util.export Line 49: @util.codegen Line 50: class OVIRTLIVEConst(object): Const no prefix, this will be aligned with other modules we have. Line 51: MINIMUM_SPACE_STORAGEDOMAIN_MB = 10240 Line 52: -- 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
