-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
omero admin diagnostics: move OMERO.py version as part of the output #339
Conversation
While this PR immediately helps clarifying a confusion reported several few times, this work is partly related to phase 4 ome/omero-certificates#26 as it adds an API for the discovery of properties stored under |
a3e3993
to
3861fce
Compare
Now included in the daily builds and should be ready for review - see e.g. https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-server/1402/console
|
Comparing the
Change 1 (vanishing of the version of the diagnostics itself - pretty confusing number, good riddance imho)
merge
Change 2
which make sense when looking at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-python-superbuild-build/ and https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-build/ (just slightly surprising that on merge-ci is the version number so low, not 5.6.5, but I guess this is another issue) lgtm |
src/omero/plugins/admin.py
Outdated
@@ -1112,6 +1117,16 @@ def _glacier2_icessl_xml(self, config_props): | |||
return ['<property name="%s" value="%s"/>' % kv | |||
for kv in list(glacier2_icessl.items())] | |||
|
|||
def get_omero_server_version(self): | |||
"""Returns the value of omero.version stored in omero.properties""" | |||
omero_props_file = old_div(self._get_etc_dir(), "omero.properties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the usage of old_div
gives me trouble during the API generation in some cases. Any reason not to use os.path.join
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the usage of old_div
comes from my copying the implementation in
omero-py/src/omero/plugins/admin.py
Lines 1094 to 1095 in d23ae9f
def _get_omero_properties(self): | |
omero_props_file = old_div(self._get_etc_dir(), "omero.properties") |
And the old_div
usage there itself comes the application of futurize
across the code (3e17e99) which substituted division operators and path separators alike. I think in that case os.path.join
is appropriate and I'll push a commit for testing appropriately but this might require a wider review.
is that a new error? related to #348 ? |
I suspect the problem was due to #350 |
The historical behavior of the `omero admin diagnostics` is to output a title line of type `OMERO Diagnostics (plugin) version`. Since OMERO 5.6.0 and the decoupling of OMERO.py from OMERO.server, this leads to ambiguity as this will display the OMERO.py version but will typically be executed on a server environment. This commit proposes to strip the version from the command title and instead report the versions of OMERO.py and OMERO.server as separate entries under the report.
9a8c63b
to
895da20
Compare
rtd build is now green |
@pwalczysko are you okay to redo the functional testing after the last round of adjustments? |
No changes in functionality detected, works as before. |
thanks @pwalczysko |
The historical behavior of the
omero admin diagnostics
is to output a title line of typeOMERO Diagnostics (plugin) version
. Since OMERO 5.6.0 and the decoupling of OMERO.py from OMERO.server, this leads to ambiguity as this will display the OMERO.py version but will typically be executed on a server environment.This commit proposes to strip the version from the command title and instead report the versions of OMERO.py and OMERO.server as separate entries under the report.
To test this PR, run
omero admin diagnostics
on OMERO.server ideally with and without this change and compare the output