Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: generate cloud-init ISO image ......................................................................
Patch Set 8: (6 comments) Very nice! Is it possible to make the content configurable? So that a user can pass additional config, either using existing code (answer file etc) or an additional plugin? 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 '\'... 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 ? 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: ] 'is not None' below should be above? 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 for the engine fqdn and copy from there, instead? Otherwise, perhaps copy the text from there? BTW, in theory they can be different - one name (IP address) for "public" access, one for the host to access it. 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 other places asking for passwords. Perhaps loop until they match? 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? We should have a constant for that, perhaps even make it configurable like in the engine (USER_VDSM, USER_APACHE etc.) 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
