search

ome / ansible-role-omero-server

Installs and configures OMERO.server
https://galaxy.ansible.com/ome/omero_server/
BSD 2-Clause "Simplified" License
4 stars 14 forks source link

Fix Ansible version_compare change in behaviour #39

Closed manics closed 4 years ago

manics commented 4 years ago

Non-standard strings (e.g. CI job names) can no longer be compared against versions. Since this usually indicates a CI build unconditionanally upgrade.

Also fixes a deprecation warning: [DEPRECATION WARNING]: Using tests as filters is deprecated. Instead of using result|version_compare use result is version_compare. This feature will be removed in version 2.9. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.

sbesson commented 4 years ago

To try and understand the various combinations of versions/variables before and after this PR, I tried to summarize various scenarios using an installed OMERO 5.5.0 server in the following table.

omero_server_release installed version omero_server_upgrade Behavior (without PR) Behavior (proposed)
latest 5.5.0 true upgrade upgrade
latest 5.5.0 false no upgrade no upgrade
present 5.5.0 true no upgrade no upgrade
present 5.5.0 false no upgrade no upgrade
5.5.1 5.5.0 true upgrade upgrade
5.5.1 5.5.0 false no upgrade no upgrade
OMERO-build 5.5.0 true no upgrade upgrade
OMERO-build 5.5.0 false no upgrade no upgrade

Is this representation correctly capturing the current behavior and the proposed change? If so, I agree we need some mechanism to force an upgrade using a given QA/CI build.

The primary question for me is whether we want to keep handling this scenario via the omero_server_release variable especially as no comparison with the current version is involved in the upgrade decision. One idea would be a separate development variable e.g. omero_server_build that could override omero_server_release. If the scenario of forcing a server upgrade using a standard version (e.g. 5.5.1 -> 5.5.0) is also in scope, an alternative would be to extend the upgrade semantics to include some force option.

manics commented 4 years ago

I think (though I'm not sure) there should be no change in behaviour. Previously comparing "non-numeric-string" < 5.5.1 was allowed though always returned the same value, now it's not, so this should be a bug fix.

If it is a change in behaviour then I think it's acceptable since these parameters aren't documented and it's for dev use.

I agree it would be nice to have a proper way of handling specifying versions (both release and dev versions) but there's a very old trello card and it's unlikely to be resolved anytime soon 😀.

sbesson commented 4 years ago

Previously comparing "non-numeric-string" < 5.5.1 was allowed though always returned the same value, now it's not, so this should be a bug fix.

Ah I missed this from my representation above. Is it related an upstream Ansible change? Which versions should be tested with/without this PR?

manics commented 4 years ago

It's an upstream Ansible change. I don't know which version though. I think as long as as the release/production deployment is unchanged we can have some leeway on development or hidden parameters.

manics commented 4 years ago

Thanks for the sleuthing!