Simone Tiraboschi has posted comments on this change. Change subject: packaging: setup: generate cloud-init ISO image ......................................................................
Patch Set 8: (6 comments) https://gerrit.ovirt.org/#/c/38811/8/src/ovirt_hosted_engine_setup/constants.py File src/ovirt_hosted_engine_setup/constants.py: Line 649: return 'OVEHOSTED_VM/vmCDRom' Line 650: Line 651: GENERATE_CLOUD_INIT_ISO = 'OVEHOSTED_VM/cloudInitISO' Line 652: CLOUD_INIT_ROOTPWD = 'OVEHOSTED_VM/cloudinitRootPwd' Line 653: CLOUD_INIT_INSTANCEHNAME = 'OVEHOSTED_VM/cloudinitInstanceHostName' > I'd call it CLOUD_INIT_INSTANCE_HOSTNAME even if it will require '\'... Done Line 654: Line 655: @ohostedattrs( Line 656: answerfile=True, Line 657: ) https://gerrit.ovirt.org/#/c/38811/8/src/plugins/ovirt-hosted-engine-setup/vm/boot_cdrom.py File src/plugins/ovirt-hosted-engine-setup/vm/boot_cdrom.py: Line 127: ohostedcons.VMEnv.GENERATE_CLOUD_INIT_ISO Line 128: ] in ( Line 129: ohostedcons.Const.CLOUD_INIT_SKIP, Line 130: ohostedcons.Const.CLOUD_INIT_GENERATE, Line 131: ) > any reason to not just check if it's CLOUD_INIT_EXISTING ? You have a value for that only if you are booting from disk but you could still select to boot from CDRom or from PXE Line 132: ) Line 133: ) Line 134: def _customization(self): Line 135: mode = 'installation' https://gerrit.ovirt.org/#/c/38811/8/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py File src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py: Line 105: ohostedcons.VMEnv.CLOUD_INIT_ROOTPWD Line 106: ] or Line 107: self.environment[ Line 108: ohostedcons.VMEnv.CLOUD_INIT_INSTANCEHNAME Line 109: ] > Perhaps even: Yes, it's definitely more readable :-) Line 110: ) is not None: Line 111: interactive = False Line 112: Line 113: if interactive: Line 152: ] == ohostedcons.Const.CLOUD_INIT_GENERATE: Line 153: instancehname = self.dialog.queryString( Line 154: name='CI_INSTANCE_HOSTNAME', Line 155: note=_( Line 156: "Enter appliance hostname (leave it empty to skip): " > I'd write something else, not sure exactly. Why not run this after we ask f Engine FQDN is currently asked in ENGINE_FQDN section which comes later on. I prefer to keep these questions in the VM section which currently comes before the engine one. I'm already using that value also for OVIRT_HOSTED_ENGINE_FQDN if the user already provided it here; in that case I'm not asking it again to keep it simpler but the user can still override using answerfile if for any reasons want them being different. I agree on making the note more descriptive. Line 157: ), Line 158: prompt=True, Line 159: default='', Line 160: ) Line 193: self.environment[ Line 194: ohostedcons.VMEnv.CLOUD_INIT_ROOTPWD Line 195: ] = password Line 196: else: Line 197: self.logger.error(_('Passwords do not match')) > We should really have a function for this... We already have at least two o I'm already looping on line 169 Refactor it to a dedicated function is a good idea. I'll do it in subsequent patch. Line 198: else: Line 199: self.environment[ Line 200: ohostedcons.VMEnv.CLOUD_INIT_ROOTPWD Line 201: ] = False Line 278: os.unlink(f_user_data) Line 279: self.environment[ohostedcons.VMEnv.CDROM] = f_cloud_init_iso Line 280: os.chown( Line 281: self._directory_name, Line 282: pwd.getpwnam('qemu').pw_uid, > qemu? Not vdsm? Yes, it's qemu that directly have to read and attach the local iso image. At this point we still don't have any ISO storage domain for VDSM. Line 283: pwd.getpwnam('qemu').pw_uid, Line 284: ) Line 285: os.chmod(f_cloud_init_iso, 0o600) Line 286: os.chown( -- To view, visit https://gerrit.ovirt.org/38811 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec4f409203e3a2d6da314208e9a0f422be00ce1b Gerrit-PatchSet: 8 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
