Repository: canonical/operator
Pull request #312 (2020-06-01 14:21:12)

ops.main: only do _init_dispatch() if dispatch exists
This avoids the situation where Juju is dispatch-aware but the charm
is not resulting in no symlinks getting created.

fixes: #310


Patch:
diff --git a/ops/main.py b/ops/main.py
index dca8da0..c387271 100755
--- a/ops/main.py
+++ b/ops/main.py
@@ -176,7 +176,7 @@ class _Dispatcher:
         self._charm_dir = charm_dir
         self._exec_path = Path(sys.argv[0])
 
-        if 'JUJU_DISPATCH_PATH' in os.environ:
+        if 'JUJU_DISPATCH_PATH' in os.environ and (charm_dir / 'dispatch').exists():
             self._init_dispatch()
         else:
             self._init_legacy()
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..7829c80 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
@@ -573,6 +602,12 @@ class TestMainWithNoDispatch(TestMain, unittest.TestCase):
         self.assertTrue(action_hook.exists())
 
 
+class TestMainWithNoDispatchButJujuIsDispatchAware(TestMainWithNoDispatch):
+    def _call_event(self, rel_path, env):
+        env["JUJU_DISPATCH_PATH"] = str(rel_path)
+        super()._call_event(rel_path, env)
+
+
 class TestMainWithNoDispatchButScriptsAreCopies(TestMainWithNoDispatch):
     hooks_are_symlinks = 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')