Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: let the user configure engine VM networking ......................................................................
Patch Set 4: (4 comments) https://gerrit.ovirt.org/#/c/41357/4/src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py File src/plugins/ovirt-hosted-engine-setup/vm/cloud_init.py: Line 99: return self._validate_ip_cidr(ip + '/32') Line 100: Line 101: def _getMyIPAddress(self): Line 102: address = None Line 103: stdout = '' iface = ( self.environment[ohostedcons.NetworkEnv.BRIDGE_NAME] if self.environment[ohostedcons.NetworkEnv.BRIDGE_NAME] in ethtool.get_devices() else self.environment[ohostedcons.NetworkEnv.BRIDGE_IF] ) log execute Line 104: if ( Line 105: self.environment[ohostedcons.NetworkEnv.BRIDGE_NAME] in Line 106: ethtool.get_devices() Line 107: ): Line 149: interactive = self.environment[ Line 150: ohostedcons.VMEnv.CLOUD_INIT_VM_STATIC_CIDR Line 151: ] is None Line 152: Line 153: if interactive: Can you try to make most of the code common? So as to not copy the tests/error messages/etc.? We do this elsewhere. If not, perhaps at least keep the texts in variables and not copy them. Line 154: while self.environment[ Line 155: ohostedcons.VMEnv.CLOUD_INIT_VM_STATIC_CIDR Line 156: ] is None: Line 157: static = self.dialog.queryString( Line 246: ), Line 247: prompt=True, Line 248: default='', Line 249: ) Line 250: if not dns: Shouldn't this be outside of the loop? Line 251: self.environment[ Line 252: ohostedcons.VMEnv.CLOUD_INIT_VM_DNS Line 253: ] = False Line 254: Line 530: note=_( Line 531: 'Automatically push a line for this host ' Line 532: 'under /etc/hosts on the engine VM\n' Line 533: 'Note: ensuring that this host could resolve the ' Line 534: 'engine VM hostname is still up to you\n' > Done Actually, no need for the 'Note:', because we verify the engine fqdn already. Actually2, this is quite annoying, because if you do not have a static reservation in dhcp, you have to lie (add a name to /etc/hosts with a fake address and fix that after booting the vm and getting a lease). IIRC we even have an RFE for allowing to set the engine fqdn only after vm is installed. Line 535: '(@VALUES@)[@DEFAULT@]? ' Line 536: ), Line 537: prompt=True, Line 538: validValues=(_('Yes'), _('No')), -- To view, visit https://gerrit.ovirt.org/41357 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic09f4d92e44f1bf810f06ada703e533b8c5e2061 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
