search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
247 stars 119 forks source link

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

Closed james-garner-canonical closed 1 month ago

james-garner-canonical commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I've also opened an issue in ops-scenario to make its status_set do the same thing.

james-garner-canonical commented 1 month ago

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).