Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: Inform the user about availability of DWH and Reports ......................................................................
Patch Set 8: (5 comments) nice job! some comments inside. https://gerrit.ovirt.org/#/c/38277/8/packaging/setup/ovirt_engine_setup/engine/constants.py File packaging/setup/ovirt_engine_setup/engine/constants.py: Line 35: Line 36: Line 37: from . import config Line 38: Line 39: # Sync with dwh/reports _upstream_ no need for '_upstream_', upstream and downstream source is the same Line 40: Line 41: Line 42: @util.export Line 43: @util.codegen https://gerrit.ovirt.org/#/c/38277/8/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/core/advertise_dwh.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/core/advertise_dwh.py: Line 48: osetupcons.Stages.DIALOG_TITLES_S_SUMMARY, Line 49: ), Line 50: condition=lambda self: self.environment[oenginecons.CoreEnv.ENABLE], Line 51: ) Line 52: def _closeup(self): we usually call setdefault as follows during stage init, so that all the following code can rely on it Line 53: self.environment.setdefault( Line 54: osetupcons.DocsEnv.DOCS_LOCAL, Line 55: False Line 56: ) Line 52: def _closeup(self): Line 53: self.environment.setdefault( Line 54: osetupcons.DocsEnv.DOCS_LOCAL, Line 55: False Line 56: ) Do we set DOCS_LOCAL anywhere? I suggested to check if a manual is found locally, this way if(/when?) upstream gets one too, it will Just Work. Line 57: Line 58: self.environment.setdefault( Line 59: osetupcons.DocsEnv.DWH_DOC_URL, Line 60: osetupcons.Const.DWH_DOC_URL Line 73: if not self.environment.get(osetupcons.DocsEnv.DOCS_LOCAL): Line 74: self.dialog.note( Line 75: text=_( Line 76: 'Note! If you want to gather statistical' Line 77: ' information you can install Reports and/or DHW:\n' DWH You must have DWH for Reports, you can write "DWH and/or Reports" When the code is basically ready, please ask Laura to review the texts. Also, perhaps put the text in a variable so you do not copy it below. Line 78: ' {dwhURL}\n' Line 79: ' {reportsURL}\n' Line 80: ).format( Line 81: dwhURL=self.environment[ Line 86: ], Line 87: ) Line 88: ) Line 89: else: Line 90: if self.environment[oengcommcons.ConfigEnv.JBOSS_AJP_PORT]: Not sure you really need this exact test, but if you do want it to work even without AJP (probably meaning jboss is standalone, not being accessed through apache), make sure it works as expected Line 91: engineURI = oenginecons.Const.ENGINE_URI Line 92: else: Line 93: engineURI = '/' Line 94: -- To view, visit https://gerrit.ovirt.org/38277 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I61fb88a6430eec6b6425075df0e749644654753a Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
