Repository: canonical/operator
Pull request #311 (2020-06-01 12:57:08)

ops/model.py: Proper support for status-get
The StatusBase types weren't being properly registered without
instantiating one. Now they are registered once they are declared,
rather than only after being created.
Futher, actually parse the output of status-get properly, and the
application status output is structured differently from the unit status
output.

fixes: #288 

facundobatista: What we REALLY need here is this:

```python
class Status:

    def __init_subclass__(cls):
        print(cls, cls.name)


class Child(Status):
    name = 'foo'
```

Sadly, it's only available in 3.6 :(

So, let's work with what we have...

What about instead of doing that call after each class definition, use a decorator? Something like....

```python
@StatusBase.register
class ActiveStatus(StatusBase):
    ...
```

That said, other option would be registering them very similar to what you did, but *in one place only*, something like...

```python
# now register them all
StatusBase.register_children(ActiveStatus, OtherStatus, ...)
```

We can even make that "automatic": get `globals()`, filter by "all the classes that inherit StatusBase", register them. This will be more robust to future changes, but maybe is too much trouble for something like this that doesn't change often.
chipaca: Huh.

I agree with facundo, and like the decorator approach the best. Maybe leave a comment next to register that we should move to init_subclass.

Patch:
diff --git a/ops/model.py b/ops/model.py
index da883e0..10ae534 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -713,6 +713,7 @@ class StatusBase:
     """
 
     _statuses = {}
+    name = None
 
     def __init__(self, message: str):
         self.message = message
@@ -720,7 +721,6 @@ class StatusBase:
     def __new__(cls, *args, **kwargs):
         if cls is StatusBase:
             raise TypeError("cannot instantiate a base class")
-        cls._statuses[cls.name] = cls
         return super().__new__(cls)
 
     def __eq__(self, other):
@@ -739,7 +739,15 @@ class StatusBase:
         else:
             return cls._statuses[name](message)
 
+    @classmethod
+    def register(cls, child):
+        if child.name is None:
+            raise AttributeError('cannot register a Status which has no name')
+        cls._statuses[child.name] = child
+        return child
+
 
+@StatusBase.register
 class UnknownStatus(StatusBase):
     """The unit status is unknown.
 
@@ -757,6 +765,7 @@ class UnknownStatus(StatusBase):
         return "UnknownStatus()"
 
 
+@StatusBase.register
 class ActiveStatus(StatusBase):
     """The unit is ready.
 
@@ -768,6 +777,7 @@ class ActiveStatus(StatusBase):
         super().__init__(message)
 
 
+@StatusBase.register
 class BlockedStatus(StatusBase):
     """The unit requires manual intervention.
 
@@ -776,6 +786,7 @@ class BlockedStatus(StatusBase):
     name = 'blocked'
 
 
+@StatusBase.register
 class MaintenanceStatus(StatusBase):
     """The unit is performing maintenance tasks.
 
@@ -787,6 +798,7 @@ class MaintenanceStatus(StatusBase):
     name = 'maintenance'
 
 
+@StatusBase.register
 class WaitingStatus(StatusBase):
     """A unit is unable to progress.
 
@@ -1051,7 +1063,30 @@ class _ModelBackend:
             is_app: A boolean indicating whether the status should be retrieved for a unit
                 or an application.
         """
-        return self._run('status-get', '--include-data', '--application={}'.format(is_app))
+        content = self._run(
+            'status-get', '--include-data', '--application={}'.format(is_app),
+            use_json=True,
+            return_output=True)
+        # Unit status looks like (in YAML):
+        # message: 'load: 0.28 0.26 0.26'
+        # status: active
+        # status-data: {}
+        # Application status looks like (in YAML):
+        # application-status:
+        #   message: 'load: 0.28 0.26 0.26'
+        #   status: active
+        #   status-data: {}
+        #   units:
+        #     uo/0:
+        #       message: 'load: 0.28 0.26 0.26'
+        #       status: active
+        #       status-data: {}
+
+        if is_app:
+            return {'status': content['application-status']['status'],
+                    'message': content['application-status']['message']}
+        else:
+            return content
 
     def status_set(self, status, message='', *, is_app=False):
         """Set a status of a unit or an application.


Test patch:
diff --git a/test/charms/test_main/src/charm.py b/test/charms/test_main/src/charm.py
index 8dd2df7..0442bbf 100755
--- a/test/charms/test_main/src/charm.py
+++ b/test/charms/test_main/src/charm.py
@@ -88,6 +88,7 @@ class Charm(CharmBase):
             self.framework.observe(self.on.start_action, self._on_start_action)
             self.framework.observe(self.on.foo_bar_action, self._on_foo_bar_action)
             self.framework.observe(self.on.get_model_name_action, self._on_get_model_name_action)
+            self.framework.observe(self.on.get_status_action, self._on_get_status_action)
 
         self.framework.observe(self.on.collect_metrics, self._on_collect_metrics)
 
@@ -184,6 +185,11 @@ class Charm(CharmBase):
         self._state['observed_event_types'].append(type(event))
         self._write_state()
 
+    def _on_get_status_action(self, event):
+        self._state['status_name'] = self.unit.status.name
+        self._state['status_message'] = self.unit.status.message
+        self._write_state()
+
     def _on_collect_metrics(self, event):
         self._state['on_collect_metrics'].append(type(event))
         self._state['observed_event_types'].append(type(event))
diff --git a/test/test_lib.py b/test/test_lib.py
index ac8e41f..174293f 100644
--- a/test/test_lib.py
+++ b/test/test_lib.py
@@ -198,7 +198,7 @@ class TestLibParser(TestCase):
 
     def test_too_long(self):
         """Check that if the file is too long, nothing is returned"""
-        m = self._mkmod('foo', '\n'*ops.lib._MAX_LIB_LINES + '''
+        m = self._mkmod('foo', '\n' * ops.lib._MAX_LIB_LINES + '''
         LIBNAME = "foo"
         LIBAPI = 2
         LIBPATCH = 42
diff --git a/test/test_main.py b/test/test_main.py
index e941bf8..99fe66b 100755
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -161,7 +161,9 @@ foo-bar:
 start:
     description: Start the unit.
 get-model-name:
-    description: Return the name of the unit
+    description: Return the name of the model
+get-status:
+    description: Return the Status of the unit
 '''
         actions_dir_name = 'actions'
         actions_meta_file = 'actions.yaml'
@@ -170,7 +172,7 @@ get-model-name:
             f.write(actions_meta)
         actions_dir = self.JUJU_CHARM_DIR / actions_dir_name
         actions_dir.mkdir()
-        for action_name in ('start', 'foo-bar', 'get-model-name'):
+        for action_name in ('start', 'foo-bar', 'get-model-name', 'get-status'):
             self._setup_entry_point(actions_dir, action_name)
 
     def _read_and_clear_state(self):
@@ -513,6 +515,33 @@ log_debug: {}
         self.assertIsNotNone(state)
         self.assertEqual(state['_on_get_model_name_action'], ['test-model-name'])
 
+    def test_has_valid_status(self):
+        self._prepare_actions()
+
+        actions_charm_config = base64.b64encode(pickle.dumps({
+            'STATE_FILE': self._state_file,
+            'USE_ACTIONS': True,
+        }))
+
+        fake_script(self, 'action-get', "echo '{}'")
+        fake_script(self, 'status-get', """echo '{"status": "unknown", "message": ""}'""")
+        state = self._simulate_event(EventSpec(
+            ActionEvent, 'get_status_action',
+            env_var='JUJU_ACTION_NAME',
+            charm_config=actions_charm_config))
+        self.assertIsNotNone(state)
+        self.assertEqual(state['status_name'], 'unknown')
+        self.assertEqual(state['status_message'], '')
+        fake_script(
+            self, 'status-get', """echo '{"status": "blocked", "message": "help meeee"}'""")
+        state = self._simulate_event(EventSpec(
+            ActionEvent, 'get_status_action',
+            env_var='JUJU_ACTION_NAME',
+            charm_config=actions_charm_config))
+        self.assertIsNotNone(state)
+        self.assertEqual(state['status_name'], 'blocked')
+        self.assertEqual(state['status_message'], 'help meeee')
+
 
 class TestMainWithNoDispatch(TestMain, unittest.TestCase):
     has_dispatch = False
diff --git a/test/test_model.py b/test/test_model.py
index ae6c143..cb8189f 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -13,12 +13,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from collections import OrderedDict
+import json
+import ipaddress
 import os
 import pathlib
+from textwrap import dedent
 import unittest
-import json
-import ipaddress
-from collections import OrderedDict
 
 import ops.model
 import ops.charm
@@ -482,6 +483,12 @@ class TestModel(unittest.TestCase):
         with self.assertRaises(TypeError):
             ops.model.StatusBase('test')
 
+        class NoNameStatus(ops.model.StatusBase):
+            pass
+
+        with self.assertRaises(AttributeError):
+            ops.model.StatusBase.register_status(NoNameStatus)
+
     def test_status_repr(self):
         test_cases = {
             "ActiveStatus('Seashell')": ops.model.ActiveStatus('Seashell'),
@@ -948,6 +955,39 @@ class TestModelBackend(unittest.TestCase):
                 run()
             self.assertEqual(fake_script_calls(self, clear=True), calls)
 
+    def test_status_get(self):
+        # taken from actual Juju output
+        content = '{"message": "", "status": "unknown", "status-data": {}}'
+        fake_script(self, 'status-get', "echo '{}'".format(content))
+        s = self.backend.status_get(is_app=False)
+        self.assertEqual(s['status'], "unknown")
+        self.assertEqual(s['message'], "")
+        # taken from actual Juju output
+        content = dedent("""
+            {
+                "application-status": {
+                    "message": "installing",
+                    "status": "maintenance",
+                    "status-data": {},
+                    "units": {
+                        "uo/0": {
+                            "message": "",
+                            "status": "active",
+                            "status-data": {}
+                        }
+                    }
+                }
+            }
+            """)
+        fake_script(self, 'status-get', "echo '{}'".format(content))
+        s = self.backend.status_get(is_app=True)
+        self.assertEqual(s['status'], "maintenance")
+        self.assertEqual(s['message'], "installing")
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['status-get', '--include-data', '--application=False', '--format=json'],
+            ['status-get', '--include-data', '--application=True', '--format=json'],
+        ])
+
     def test_status_is_app_forced_kwargs(self):
         fake_script(self, 'status-get', 'exit 1')
         fake_script(self, 'status-set', 'exit 1')