feat: make it an error to call CollectStatusEvent.add_status with error or unknown

[james-garner-canonical]

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.

[tonyandrewmeyer]

I think the overloads are fine - yes, there are 18 lines, but it's readable and clear what it's doing.

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.

It would be good to check for this (e.g. super-tox -e with the various names charms use for static checking). I'm generally ok with going to the stricter types both for the ability to have more static checking and for any autocomplete improvements it brings (probably not much here, since these objects are practically identical), but I don't think we want to annoy charmers if there isn't a clear win.

[benhoyt]

Sorry to be a wet blanket, but I'm not a fan of this. I think it's a lot of implementation noise for the sake of something that's likely not an issue in practice. I'd much prefer we keep the types simple and do the runtime checks.

However, I suggest we do that a different way. For one thing, this only covers CollectStatusEvent.add_status, not app.status = ... or unit.status = ..., and we'd want the same check for those.

If we put the check in status_set, we'd mirror the check that Juju's status-set hook tool already does. In fact, for the _TestingModelBackend used by harness, we already do this:

    def status_set(self, status: '_StatusName', message: str = '', *, is_app: bool = False):
        if status in [model.ErrorStatus.name, model.UnknownStatus.name]:
            raise model.ModelError(
                f'ERROR invalid status "{status}", expected one of'
                ' [maintenance blocked waiting active]'
            )

(It would seem better to do "not in allowed" rather than "in disallowed", but that's a minor detail.)

In other words, as long as someone has a unit test with the code path in question, Harness will already raise an error and flag this.

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.

Overall it's a trade-off: we want reasonable types, but they come at a cost, and I think we need to be careful here.

[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 good point -- thanks.

[benhoyt]

Thanks, I'm happy with this much more limited change.

[benhoyt]

I've also opened an issue in ops-scenario 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).

[tonyandrewmeyer]

Looks good to me, thanks!

One small doc suggestion.

[benhoyt]

Fine with me, thanks!