feat: make it an error to call CollectStatusEvent.add_status with err…

…or 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

@@ -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):

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)