Francesco Romani has posted comments on this change. Change subject: services, setup: ovirt-vmconsole integration ......................................................................
Patch Set 45: (17 comments) https://gerrit.ovirt.org/#/c/35906/45/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 592: Line 593: %description setup-plugin-websocket-proxy Line 594: Setup and upgrade specific plugins for websocket-proxy Line 595: Line 596: %package vmconsole-proxy > I find this name confusing. We already have a proxy in the ovirt-vmconsole To reduce confusione, will add -helper suffix everywhere. Will move the helper proper into packaging/libexec/vmconsole-proxy-helper to reduce confusione, and because it indeed fits poorly under services. Line 597: Summary: %{ovirt_product_name_short} VMconsole Proxy Helper Line 598: Group: %{ovirt_product_group} Line 599: Requires: %{name}-lib >= %{version}-%{release} Line 600: Requires: %{name}-setup-plugin-vmconsole-proxy >= %{version}-%{release} https://gerrit.ovirt.org/#/c/35906/45/packaging/setup/ovirt_engine_setup/vmconsole_proxy/__init__.py File packaging/setup/ovirt_engine_setup/vmconsole_proxy/__init__.py: Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2014-2015 Red Hat, Inc. > 2014? oops, will remove. Line 4: # Line 5: # Licensed under the Apache License, Version 2.0 (the "License"); Line 6: # you may not use this file except in compliance with the License. Line 7: # You may obtain a copy of the License at https://gerrit.ovirt.org/#/c/35906/45/packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py File packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py: Line 37: class Const(object): Line 38: Line 39: VMCONSOLE_PROXY_SERVICE_NAME = 'ovirt-vmconsole-proxy' Line 40: Line 41: VMCONSOLE_PROXY_PACKAGE_NAME = 'ovirt-engine-vmconsole-proxy' > You do not seem to use this and similar consts yet. Perhaps add a TODO comm THey are relics, indeed. Will remove... and ALSO add a TODO comment Line 42: Line 43: VMCONSOLE_PROXY_SETUP_PACKAGE_NAME = \ Line 44: 'ovirt-engine-setup-plugin-vmconsole-proxy' Line 45: Line 48: VMCONSOLE_PROXY_HELPER_PKI_NAME = 'vmconsole-proxy-helper' Line 49: Line 50: OVIRT_VMCONSOLE_PROXY_EKU = '1.3.6.1.4.1.2312.13.1.2.1.1' Line 51: Line 52: OVIRT_VMCONSOLE_ALLOWED_USER = 'ovirt-vmconsole' > OVIRT_VMCONSOLE_USER ? What's "allowed"? Even VMCONSOLE_USER might be enoug removed _ALLOWED Line 53: Line 54: OVIRT_VMCONSOLE_ALLOWED_GROUP = 'ovirt-vmconsole' Line 55: Line 56: Line 50: OVIRT_VMCONSOLE_PROXY_EKU = '1.3.6.1.4.1.2312.13.1.2.1.1' Line 51: Line 52: OVIRT_VMCONSOLE_ALLOWED_USER = 'ovirt-vmconsole' Line 53: Line 54: OVIRT_VMCONSOLE_ALLOWED_GROUP = 'ovirt-vmconsole' > same ditto Line 55: Line 56: Line 57: @util.export Line 58: class FileLocations(object): Line 115: Line 116: Line 117: @util.export Line 118: class Stages(object): Line 119: > Add also here a comment "sync with engine". One should be also in wsp where Done Line 120: CA_AVAILABLE = 'osetup.pki.ca.available' Line 121: Line 122: CONFIG_VMCONSOLE_ENGINE_CUSTOMIZATION = \ Line 123: 'setup.config.vmconsole-engine.customization' Line 147: @util.codegen Line 148: @osetupattrsclass Line 149: class ConfigEnv(object): Line 150: Line 151: VMCONSOLE_PROXY_HOST = 'OVESETUP_CONFIG/vmconsoleProxyHost' > Perhaps use a different prefix, e.g. OVESETUP_VMP_CONFIG or OVESETUP_VMCONS Done Line 152: Line 153: VMCONSOLE_PROXY_PORT = 'OVESETUP_CONFIG/vmconsoleProxyPort' Line 154: Line 155: VMCONSOLE_PROXY_CONFIGD_PATH = \ https://gerrit.ovirt.org/#/c/35906/45/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/__init__.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/__init__.py: Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2014-2015 Red Hat, Inc. > 2014? Done Line 4: # Line 5: # Licensed under the Apache License, Version 2.0 (the "License"); Line 6: # you may not use this file except in compliance with the License. Line 7: # You may obtain a copy of the License at https://gerrit.ovirt.org/#/c/35906/45/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py: Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2014-2015 Red Hat, Inc. > 2014? Done Line 4: # Line 5: # Licensed under the Apache License, Version 2.0 (the "License"); Line 6: # you may not use this file except in compliance with the License. Line 7: # You may obtain a copy of the License at Line 15: # limitations under the License. Line 16: # Line 17: Line 18: Line 19: """vmconsole proxy plugin.""" > vmconsole proxy config plugin ? Done Line 20: Line 21: import os.path Line 22: Line 23: import gettext Line 38: Line 39: Line 40: @util.export Line 41: class Plugin(plugin.PluginBase): Line 42: """vmconsole proxy plugin.""" > same Done Line 43: Line 44: def __init__(self, context): Line 45: super(Plugin, self).__init__(context=context) Line 46: Line 57: None Line 58: ) Line 59: self.environment.setdefault( Line 60: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT, Line 61: ovmpcons.Defaults.DEFAULT_VMCONSOLE_PROXY_PORT > If you always query for this (not sure you should), please setdefault to No Done Line 62: ) Line 63: self.environment.setdefault( Line 64: ovmpcons.EngineConfigEnv.ENGINE_FQDN, Line 65: 'localhost' Line 75: osetupcons.Stages.DIALOG_TITLES_S_PRODUCT_OPTIONS, Line 76: ), Line 77: ) Line 78: def _customization(self): Line 79: if not _existsUserGroup( > Why not just install it? E.g. if you: Because Engine does not require the proxy to be available. Granted, the feature ultimately needs the proxy package to work, but not every user may want to have it installed. Thus, in order to avoid to always carry unnecessary baggage, I choose not to hard-require the proxy package. This choice makes it necessary to detect the presence the package. Line 80: self.logger, Line 81: ovmpcons.Const.OVIRT_VMCONSOLE_ALLOWED_USER, Line 82: ovmpcons.Const.OVIRT_VMCONSOLE_ALLOWED_GROUP Line 83: ) or not os.path.exists( Line 128: supply_default=True, Line 129: ) Line 130: Line 131: while True: Line 132: try: > Check if it's already set in the answer file, and then do not query. Done Line 133: port = osetuputil.parsePort( Line 134: self.dialog.queryString( Line 135: name='VMCONSOLE_PROXY_HOST', Line 136: note=_( Line 136: note=_( Line 137: 'Engine vmconsole port [@DEFAULT@]: ' Line 138: ), Line 139: prompt=True, Line 140: default=self.environment[ > default=DEFAULT_PORT Done Line 141: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT Line 142: ] Line 143: ) Line 144: ) Line 146: ovmpcons.ConfigEnv.VMCONSOLE_PROXY_PORT Line 147: ] = port Line 148: break Line 149: except ValueError: Line 150: pass > Perhaps warn? How should the user know why we ask again? Good point. fixed. Line 151: Line 152: @plugin.event( Line 153: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 154: condition=lambda self: self.environment[ Line 200: ] = dialog.queryBoolean( Line 201: dialog=self.dialog, Line 202: name='OVESETUP_CONFIG_VMCONSOLE_PROXY_INTEG_CONF_MAKE', Line 203: note=( Line 204: 'Make configuration for the ovirt-vmconsole-proxy ' > I don't like the text (and const name), but I am not a native English speak Probably overkill. Will remove and depend on VMCONSOLE_PROXY_CONFIG alone Line 205: 'package (@VALUES@) [@DEFAULT@]: ' Line 206: ), Line 207: prompt=True, Line 208: default=True, -- 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: 45 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: 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
