Yedidyah Bar David 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 package. Also can't find it mentioned in the feature page. afaiu from the code, it's a utility to get from the engine some vmconsole-related data (users and consoles). Perhaps name it vmconsole-proxy-helper? vmconsole-info? Not sure. Also not sure why it's in services. I find also the naming of various other files and configuration constants confusing. 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? 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 comment that package management/upgrade is missing. 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 enough. 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 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 you probably copied this from. 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_VMCONSOLE_PROXY_CONFIG. wsp didn't do that because we then had just one constants file, see [1], and tries to support older answer files. But you can. [1] https://gerrit.ovirt.org/27647 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? 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? 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 ? 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 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 None 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: yum install websocket-proxy-setup-plugin engine-setup Configure websocket proxy? Yes Then it installs it for you. If not doing that now, please add a TODO comment, perhaps open also a new bug for that. You can also, for now, just require it in the spec file, thus no need to check. If you do that, add a comment there too please. 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. You might want to use [1] once it's merged (should be soon). For examples see queryPassword inside it and [2]. [1] https://gerrit.ovirt.org/42017 [2] https://gerrit.ovirt.org/42032 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 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? 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 speaker, so not sure my opinion matters. I also do not understand why ask again if you already asked about VMCONSOLE_PROXY_CONFIG. 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
