Simone Tiraboschi has posted comments on this change. Change subject: services, setup: ovirt-vmconsole integration ......................................................................
Patch Set 29: (13 comments) https://gerrit.ovirt.org/#/c/35906/29/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py: Line 94: prompt=True, Line 95: default=True, Line 96: ) Line 97: Line 98: if enabled and not exists_user_group( It's better to check VM Console Proxy availability before asking about configuring it. Line 99: self.logger, Line 100: ovmpcons.PKIEnv.OVIRT_VMCONSOLE_ALLOWED_USER, Line 101: ovmpcons.PKIEnv.OVIRT_VMCONSOLE_ALLOWED_GROUP Line 102: ): Line 100: ovmpcons.PKIEnv.OVIRT_VMCONSOLE_ALLOWED_USER, Line 101: ovmpcons.PKIEnv.OVIRT_VMCONSOLE_ALLOWED_GROUP Line 102: ): Line 103: self.logger.warning( Line 104: 'VM Console Proxy seems not installed on this host. ' Please use '_()' to allow localization Line 105: 'Disabled configuration.') Line 106: enabled = False Line 107: Line 108: self.environment[ Line 106: enabled = False Line 107: Line 108: self.environment[ Line 109: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_CONFIG Line 110: ] = enabled You are going to enforce the availability of VM Console Proxy only if the user is configuring interactively ( self.environment[ovmpcons.ConfigEnv.VMCONSOLE_PROXY_CONFIG]=None ) but we have also to properly support answerfile based configuration: you still need to enforce it and fail otherwise. Line 111: Line 112: self._enabled = self.environment[ Line 113: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_CONFIG Line 114: ] Line 137: Line 138: while True: Line 139: try: Line 140: port = osetuputil.parsePort( Line 141: self.dialog.queryString( We need to support non interactive configuration also here Line 142: name='VMCONSOLE_PROXY_HOST', Line 143: note=_( Line 144: 'Engine vmconsole port [@DEFAULT@]: ' Line 145: ), Line 144: 'Engine vmconsole port [@DEFAULT@]: ' Line 145: ), Line 146: prompt=True, Line 147: default=self.environment[ Line 148: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT] Please add a newline before ']' Line 149: ) Line 150: ) Line 151: self.environment[ Line 152: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT] = port Line 148: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT] Line 149: ) Line 150: ) Line 151: self.environment[ Line 152: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT] = port here too Line 153: break Line 154: except ValueError: Line 155: pass Line 156: Line 212: ) Line 213: ) Line 214: Line 215: @plugin.event( Line 216: stage=plugin.Stages.STAGE_CUSTOMIZATION, Please reorder to make it coherent with the execution flow (being at STAGE_CUSTOMIZATION it's going to be executed before _misc_config witch is at STAGE_MISC) Line 217: name=ovmpcons.Stages.CONFIG_VMCONSOLE_PROXY_CUSTOMIZATION, Line 218: condition=lambda self: self.environment[ Line 219: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_CONFIG Line 220: ], Line 299: def exists_user_group(log, user, group): Line 300: try: Line 301: pwd.getpwnam(user) Line 302: except KeyError: Line 303: log.warn(('User {user} does not exist.'.format(user=user))) Please use '_()' to allow localization Line 304: return False Line 305: Line 306: try: Line 307: grp.getgrnam(group) Line 305: Line 306: try: Line 307: grp.getgrnam(group) Line 308: except KeyError: Line 309: log.warn(('Group {group} does not exist.'.format(group=group))) Here too Line 310: return False Line 311: Line 312: return True Line 313: https://gerrit.ovirt.org/#/c/35906/29/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py: Line 67: oenginecons.PKIEnv.STORE_PASS We are already initializing it in packaging/setup/plugins/ovirt-engine-rename/ovirt-engine/pki.py Line 68: oengcommcons.Defaults.DEFAULT_PKI_STORE_PASS Line 69: ) Line 70: self.environment.setdefault( Line 71: oenginecons.PKIEnv.COUNTRY, Line 72: oengcommcons.Defaults.DEFAULT_PKI_COUNTRY Here too Line 73: ) Line 74: self.environment.setdefault( Line 75: oenginecons.PKIEnv.ORG, Line 76: None Line 71: oenginecons.PKIEnv.COUNTRY, Line 72: oengcommcons.Defaults.DEFAULT_PKI_COUNTRY Line 73: ) Line 74: self.environment.setdefault( Line 75: oenginecons.PKIEnv.ORG, Also here Line 76: None Line 77: ) Line 78: Line 79: @plugin.event( Line 92: ) and Line 93: self.environment[oenginecons.PKIEnv.ORG] is None Line 94: ), Line 95: ) Line 96: def _customization(self): I don't think you need to duplicate this code: you already have it if you are deploying on the engine host and you need something different (generating a local CSR, remotely sign on the engine host and transfer back) if you are on a different host Line 97: org = 'Test' Line 98: if '.' in self.environment[osetupcons.ConfigEnv.FQDN]: Line 99: org = self.environment[ Line 100: osetupcons.ConfigEnv.FQDN -- To view, visit https://gerrit.ovirt.org/35906 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I034ef8e6d10da5dc93eda61e0c5c518ca13a5a28 Gerrit-PatchSet: 29 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@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
