Merge lp:~adam-collard/charm-helpers/status-get-message into lp:charm-helpers

[Adam Collard]

Change status_get() to return both the status and the message associated with it. This makes status_set(*status_get()) end up with the unit having the same state and message.

I searched the whole charmstore for files in hooks/ which used status_get() and weren't in charmhelpers itself. Only the Cassandra charm was using it, and I have filed bug 1489120 pre-emptively to note the failure.

[Chris Glass (tribaal)]

Thanks for your branch.

I think in this particular case it would be better to simply return a 2-tuple with the message every time. I see no reason not to, and adding switches that default to a non-ideal behavior seems broken to me.

Since none of the charms I grep'd use this method (yet?), I would go for breaking and not being backwards compatible.

In other words, make status_get() not take a parameter, and always pass --include-data, returning a 2-tuple. Bonus points for making it an explicit 2-tuple so future readers see it right away.

review: Needs Fixing

[lp:~adam-collard/charm-helpers/status-get-message]

436. By Adam Collard on 2015-08-26

Switch to always returning status and message (tribaal's review)

[Chris Glass (tribaal)]

Ok looks good now, thanks!

As commented in the cassandra bug the failure this will introduce will be picked up and fixed next time they synv C-H.

+1!

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2015-08-04 20:02:59 +0000
+++ charmhelpers/core/hookenv.py	2015-08-26 17:32:40 +0000
@@ -767,21 +767,23 @@
 
 
 def status_get():
-    """Retrieve the previously set juju workload state
-
-    If the status-set command is not found then assume this is juju < 1.23 and
-    return 'unknown'
+    """Retrieve the previously set juju workload state and message
+
+    If the status-get command is not found then assume this is juju < 1.23 and
+    return 'unknown', ""
+
     """
-    cmd = ['status-get']
+    cmd = ['status-get', "--format=json", "--include-data"]
     try:
-        raw_status = subprocess.check_output(cmd, universal_newlines=True)
-        status = raw_status.rstrip()
-        return status
+        raw_status = subprocess.check_output(cmd)
     except OSError as e:
         if e.errno == errno.ENOENT:
-            return 'unknown'
+            return ('unknown', "")
         else:
             raise
+    else:
+        status = json.loads(raw_status.decode("UTF-8"))
+        return (status["status"], status["message"])
 
 
 def translate_exc(from_exc, to_exc):

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2015-07-29 15:33:40 +0000
+++ tests/core/test_hookenv.py	2015-08-26 17:32:40 +0000
@@ -1354,23 +1354,25 @@
 
     @patch('subprocess.check_output')
     def test_status_get(self, check_output):
-        check_output.return_value = 'active\n'
+        check_output.return_value = json.dumps(
+            {"message": "foo",
+             "status": "active",
+             "status-data": {}}).encode("UTF-8")
         result = hookenv.status_get()
-        self.assertEqual(result, 'active')
-        check_output.assert_called_with(['status-get'], universal_newlines=True)
+        self.assertEqual(result, ('active', 'foo'))
+        check_output.assert_called_with(
+            ['status-get', "--format=json", "--include-data"])
 
     @patch('subprocess.check_output')
     def test_status_get_nostatus(self, check_output):
         check_output.side_effect = OSError(2, 'fail')
         result = hookenv.status_get()
-        self.assertEqual(result, 'unknown')
-        check_output.assert_called_with(['status-get'], universal_newlines=True)
+        self.assertEqual(result, ('unknown', ''))
 
     @patch('subprocess.check_output')
     def test_status_get_status_error(self, check_output):
         check_output.side_effect = OSError(3, 'fail')
         self.assertRaises(OSError, hookenv.status_get)
-        check_output.assert_called_with(['status-get'], universal_newlines=True)
 
     @patch('subprocess.check_output')
     @patch('glob.glob')