From 64d9565a7b762d1a8cabc4080905e67bfbf5c59a Mon Sep 17 00:00:00 2001
From: James Garner <james.garner@canonical.com>
Date: Wed, 25 Sep 2024 15:48:35 +1200
Subject: [PATCH] feat: make it an error to call CollectStatusEvent.add_status
 with error or unknown (#1386)

Add a runtime check to immediately raise an InvalidStatusError if add_status is
called with ErrorStatus or UnknownStatus. Previously these statuses would (in
the case of ErrorStatus) or might (in the case of UnknownStatus) lead to an
InvalidStatusError when it came time to set the status after the collect-status
observer returned. It was never valid to set either status, this just means that
the user will get an immediate exception that points to the line adding the bad
status.
---
 ops/charm.py       | 9 +++++++--
 test/test_charm.py | 4 ++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/ops/charm.py b/ops/charm.py
index edef8db32..0e6861850 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -1105,12 +1105,13 @@ class CollectStatusEvent(LifecycleEvent):
 
     The order of priorities is as follows, from highest to lowest:
 
-    * error
     * blocked
     * maintenance
     * waiting
     * active
-    * unknown
+
+    It is an error to call :meth:`add_status` with an instance of
+    :class:`ErrorStatus` or :class:`UnknownStatus`.
 
     If there are multiple statuses with the same priority, the first one added
     wins (and if an event is observed multiple times, the handlers are called
@@ -1151,6 +1152,10 @@ def add_status(self, status: model.StatusBase):
         """
         if not isinstance(status, model.StatusBase):
             raise TypeError(f'status should be a StatusBase, not {type(status).__name__}')
+        if status.name not in model._SETTABLE_STATUS_NAMES:
+            raise model.InvalidStatusError(
+                f'status.name must be in {model._SETTABLE_STATUS_NAMES}, not {status.name!r}'
+            )
         model_ = self.framework.model
         if self.handle.kind == 'collect_app_status':
             if not isinstance(status, model.ActiveStatus):
diff --git a/test/test_charm.py b/test/test_charm.py
index 0939fbb6b..5872a4eca 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -959,7 +959,6 @@ def _on_collect_status(self, event: ops.CollectStatusEvent):
         (['waiting', 'blocked'], 'blocked'),
         (['waiting', 'maintenance'], 'maintenance'),
         (['active', 'waiting'], 'waiting'),
-        (['active', 'unknown'], 'active'),
     ],
 )
 def test_collect_status_priority_valid(
@@ -994,6 +993,7 @@ def _on_collect_status(self, event: ops.CollectStatusEvent):
     [
         ['blocked', 'error'],
         ['unknown'],
+        ['active', 'unknown'],
     ],
 )
 def test_collect_status_priority_invalid(
@@ -1015,7 +1015,7 @@ def _on_collect_status(self, event: ops.CollectStatusEvent):
 
     framework = create_framework(request)
     charm = MyCharm(framework, statuses=statuses)
-    with pytest.raises(ModelError):
+    with pytest.raises(ops.InvalidStatusError):
         ops.charm._evaluate_status(charm)