Merge lp:~lazypower/charm-helpers/add-workload-version into lp:charm-helpers

[Charles Butler]

Adds:

- application-version-set hookenv command
- Test coverage to support the added code

[Stuart Bishop (stub)]

I'd rather see this do nothing than fail if it is called from 1.25. It currently raises a not implemented error, which will bite people maintaining charms that need to support old Juju. Unlike other tools like leader-set, I think this is more like status_set where it is better to just make do (in this case, do nothing). I'll let this approach through though, as it is somewhat a matter of opinion. But my personal opinion is I'd rather not have to guard all my calls in the PostgreSQL charm and have charm-helpers DWIM if it happens to be running with Juju 1.25.

I personally hate the @translate_exc approach, since it masks lots of other errors than 'not running a recent enough version of juju'. I much prefer "if has_juju_version('2.0')", even though group think is that you should do it this way with all its caveats and hidden bugs rather than to concisely state that this is a Juju 2.0 feature. But I won't block review on this point since someone else has gone to the trouble of rewriting things using the less readable and more error prone approach :-/

review: Approve

[Stuart Bishop (stub)]

Per irc discussion.

I have charms that support both 1.25 and 2.0
If I want to call application_version_set, with what you have here I need to do:

try:
    application_version_set(ver)
except NotImplementedError:
    pass

I think charmhelpers should do that, so in the PostgreSQL charm I can just do 'application_version_set(ver)' and it works under 2.0 and does nothing under 1.25

Ignore my rant about version sniffing, since this way is consistent with the rest of the code base (someone has 'fixed' the counter examples).

review: Needs Fixing

[Charles Butler (lazypower)]

I'm not against that proposal stub. Just to be precise, if we go through to modify the other methods, we're looking at changing the behavior of:

- leader_set
- leader_get
- is_leader
- payload_register
- payload_unregister
- payload_status_set
- resource_get
- network_get_primary_address

We should probably file an issue and work through that elsewhere, but I don't know that we can reasonably change the behavior of charm-helpers.core.hookenv at this point as it will ripple throughout every charm that uses charm-helpers, and that's most if not all of them.

[lp:~lazypower/charm-helpers/add-workload-version]

623. By Charles Butler on 2016-09-07

Refactor the application-version-set stanza to only log if not on a 2.0+ host.

[Stuart Bishop (stub)]

I'm not suggesting changing the behaviour of any other functions. Calling any of the ones you cite from an old version of Juju should most certainly fail, because charms calling them are relying on their behavour and effects. application_version_set() is like status_set(), where we can provide fall back behaviour for old versions of Juju.

[Stuart Bishop (stub)]

This looks good. I'll land it.

review: Approve

[Charles Butler (lazypower)]

Ok, I misunderstood the context of your reply, but seems like we got there anyway! \o/ Thanks stub.

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2016-07-06 14:41:05 +0000
+++ charmhelpers/core/hookenv.py	2016-09-07 16:37:52 +0000
@@ -843,6 +843,20 @@
     return inner_translate_exc1
 
 
+def application_version_set(version):
+    """Charm authors may trigger this command from any hook to output what
+    version of the application is running. This could be a package version,
+    for instance postgres version 9.5. It could also be a build number or
+    version control revision identifier, for instance git sha 6fb7ba68. """
+
+    cmd = ['application-version-set']
+    cmd.append(version)
+    try:
+        subprocess.check_call(cmd)
+    except OSError:
+        log("Application Version: {}".format(version))
+
+
 @translate_exc(from_exc=OSError, to_exc=NotImplementedError)
 def is_leader():
     """Does the current unit hold the juju leadership

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2016-06-23 07:10:31 +0000
+++ tests/core/test_hookenv.py	2016-09-07 16:37:52 +0000
@@ -1204,6 +1204,11 @@
         check_call_.assert_called_with(['payload-status-set', 'monitoring',
                                         'abc123', 'Running'])
 
+    @patch('subprocess.check_call')
+    def test_application_version_set(self, check_call_):
+        hookenv.application_version_set('v1.2.3')
+        check_call_.assert_called_with(['application-version-set', 'v1.2.3'])
+
 
 class HooksTest(TestCase):
     def setUp(self):