Repository: canonical/operator
Pull request #1386 (2024-09-23 00:43:01)

feat: make it an error to call CollectStatusEvent.add_status with error or unknown
It seems like it should immediately be an error to add an `ErrorStatus` or `UnknownStatus` with `add_status`, since these can never be set as the status, and will cause an error at the end of the event if they are the highest priority status added (always the case with `ErrorStatus` as it has the highest priority, while with `UnknownStatus` only if it is the only status type added, as it has the lowest priority). See also issue #1356.

This draft PR makes this relatively simple runtime change, and then investigates the changes needed to support this via type checking.

There ends up being more type checking code than runtime code, with two new public type aliases (`SettableStatus` and `SettableStatusName` -- better names might be better?).

The biggest offender is `from_name`, which previously took a `str` argument and (implicitly) returned an `UnknownStatus` or another `StatusBase`, but which we now need to know whether the status returned will be valid for use with `add_status` or not. This PR uses `@overload` to accomplish this, but possibly there's a better way (a `TypedDict` for `StatusBase._statuses` perhaps?).

One concern I have is if people are using loosely typed strings to call `from_name`, or loosely typed collections of statuses when calling `add_status`, they'll now have a lot of type checking errors.

One possible solution would be to just make the runtime changes and leave the typing alone for now.

james-garner-canonical: > I think it's a lot of implementation noise for the sake of something that's likely not an issue in practice

Yeah I think it's a lot of typing ceremony for not much value added. I've taken out the type checking changes in favour of just runtime checks.

`status_set` is actually already checked at runtime, but I think it's still worth checking in `add_status` to make it easier to get to the the offending line of code from the exception, since an error on the automatic `status_set` call after the `collect-status` event won't point directly to the bad `add_status` call.
tonyandrewmeyer: > It looks like Scenario doesn't do this check (@tonyandrewmeyer can correct me if I'm wrong), but that could easily be added, and I'd be in support of that.

It doesn't, and I'd be happy have that added too (it would be in mocking.py's `set_status` I imagine). Feel free to open a PR!
benhoyt: > status_set is actually already checked at runtime, but I think it's still worth checking in add_status to make it easier to get to the the offending line of code from the exception, since an error on the automatic status_set call after the collect-status event won't point directly to the bad add_status call.

Ah, that's a goo point -- thanks.
benhoyt: I've also opened an [issue in ops-scenario](https://github.com/canonical/ops-scenario/issues/202) to make its `status_set` do the same thing.
james-garner-canonical: Are we happy to merge the changes in as is? The current changes (1 approval) are an update to the docstring for `CollectStatusEvent`, and making it an immediate `InvalidStatusError` to call `add_status` with `ErrorStatus` or `UnknownStatus`, rather than waiting for an `InvalidStatusError` to be raised when the status is set at the end of the `collect-status` event (after the charm's observer for `CollectStatusEvent` has returned).

Patch:
diff --git a/ops/charm.py b/ops/charm.py
index edef8db..0e68618 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 @@ class CollectStatusEvent(LifecycleEvent):
         """
         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 patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index 0939fbb..5872a4e 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -959,7 +959,6 @@ def test_add_status_type_error(request: pytest.FixtureRequest, fake_script: Fake
         (['waiting', 'blocked'], 'blocked'),
         (['waiting', 'maintenance'], 'maintenance'),
         (['active', 'waiting'], 'waiting'),
-        (['active', 'unknown'], 'active'),
     ],
 )
 def test_collect_status_priority_valid(
@@ -994,6 +993,7 @@ def test_collect_status_priority_valid(
     [
         ['blocked', 'error'],
         ['unknown'],
+        ['active', 'unknown'],
     ],
 )
 def test_collect_status_priority_invalid(
@@ -1015,7 +1015,7 @@ def test_collect_status_priority_invalid(
 
     framework = create_framework(request)
     charm = MyCharm(framework, statuses=statuses)
-    with pytest.raises(ModelError):
+    with pytest.raises(ops.InvalidStatusError):
         ops.charm._evaluate_status(charm)