is_leader requires juju 1.22.0 or better #16
Conversation
| @@ -24,6 +25,10 @@ | |||
|
|
|||
| @hooks.hook('config-changed') | |||
| def config_changed(): | |||
| if not version_check: | |||
| hookenv.log('CRITICAL', 'This charm requires juju 1.22.0 or greater.' | |||
There was a problem hiding this comment.
I believe you are calling log() incorrectly here. The message is first and the level is the second argument.
charmhelpers.core.hookenv.log(message, level=None)
Write a message to the juju log
|
Hey Chuck, Thanks for the quick turn around on this issue! I agree that this is the right way to handle this issue, if the charm makes use of a feature that is in a different version of Juju then we should exit. |
lazypower
force-pushed
the
minimum-juju-version
branch
from
016c4c5
to
5d4e1cc
Compare
The ETCD charm fails with a non committal error message when probing for is_leader on juju versions <= 1.21, 1.18 is still the default shipping in universe, and will cause some headaches if users of older juju versions attempt to use this charm as its leveraging the leader election components. There is probably a charm helper for consuming these methods that has a sane fallback strategy, but for now this will satisfy the requirement of blocking installation on anything < 1.22.0 Introduces a new dependency 'semver' to perform the version calculation
|
@mbruzek thanks for the review, i've updated the hookenv.log statement |
|
Hey Chuck, Could we re-factor this to call "is_leader" and if the command does not exist by exception or return code then just error out with the same message? This would prevent having to import a new package. They do something similar here: What do you think about that? |
|
I'm a +1 to testing for features instead of trying to make assumptions. You can easily modify the version method to just execute is_leader and capture OSError output. There could be situations where in later versions of juju is_leader is depreciated or removed. There is also the possibility that the juju version format changes (as it has several times already) |
This favors a `which is_leader` command to probe if the command is available. It still panic's and bails on the installation if is_leader is not available (the current defacto method to determine if leader election is available on a unit - see whitmo#16 for more information on why this is probing as it were.
|
@mbruzek @marcoceppi I refactored this to favor a LMK if there is more that needs to be done here. |
| return True | ||
| else: | ||
| return False | ||
| except OSError as e: |
There was a problem hiding this comment.
I'd remove the entire try block here, since OSError will simply raise just like any other exception. There really isn't any exception handling here. You can also simplify the if block with
return ret == 0
Finally, it appears that charmhelpers has some way of translating whether or not is-leader is available. Doing this
from charmhelpers.core.hookenv import is_leader
try:
is_leader()
except NotImplementedError:
print("panic message")
would achieve the same goal.
|
I had some more pedantic comments, but this is better IMO as is |
lazypower
force-pushed
the
minimum-juju-version
branch
from
a64d013
to
23d17ca
Compare
refactored from a bash based sniff to just consume charm helpers and abort on NotImplementedError
|
LGTM +1 |
|
+1 LGTM |
is_leader requires juju 1.22.0 or better
The ETCD charm fails with a non committal error message when probing for
is_leader on juju versions <= 1.21, 1.18 is still the default shipping
in universe, and will cause some headaches if users of older juju
versions attempt to use this charm as its leveraging the leader election
components.
There is probably a charm helper for consuming these methods that has a
sane fallback strategy, but for now this will satisfy the requirement of
blocking installation on anything < 1.22.0
Introduces a new dependency 'semver' to perform the version calculation