chore: Document and validate settable status values in _ModelBackend.set_status #1354
Conversation
Previously was catching AttributeError, which must have been raised historically for missing `name` attribute on StatusBase subclasses. This, combined with a `type: ignore` directive concealed that the `register_status` method does not exist -- the attempt to access it raises an AttributeError. The correct method is `register`, which raises a TypeError.
A `type: ignore` directive hid that `status_set` was being called with the wrong argument types. The intention was to test that an error is raised on calling with a non-bool argument for `is_app`, however the status argument should be a string, and was instead a `StatusBase` class. The test would have still correctly errored out on the `is_app` argument before this error had an effect at testing time.
_ModelBackend.status_set's status argument was previously type hinted only as str. However, this argument can really only be one of four valid statuses that juju's set-status will accept. This commit documents this, and adjusts the definition of StatusBase to also type hint the six acceptable status names, as well as using these type hints across code that interacts with StatusBase and status_set.
Since we know the valid arguments for status, and expose them in type hints, and since ops already validates arguments here to some extent (is_app being a bool in status_set, status and message being strings in Application.status and Unit.status), let's check that status is valid in status_set and raise a ModelError immediately if not, rather than catching status-set's exit code and only then raising a ModelError.
Since _ModelBackend.set_status validates its status and is_app arguments, let's also validate the type of message, removing redundant checks from Application.status and cleaning up a fixme in Unit.status. Also add a test to cover this, analogous to the existing test for the type of is_app.
james-garner-canonical
changed the title
Given that it's an observability job that fails, I suspect that this change is a breaking change subject to how charmers use our code today. Please check. P.S. I recommend amazing https://github.com/tonyandrewmeyer/charm-analysis to try new code across some 120~160 charms. |
|
This PR breaks a type checking test in canonical/prometheus-k8s-operator where StatusBase.from_name(name, message) is called. The error is because Since Solutions here include:
I'd lean in favour of (1), and will look into modifying this PR to avoid changes to the type signature of the public api at this point. |
Since StatusBase.from_name is part of the public API, and is currently used in several charms, changing the type of the name argument from str to _StatusName will break users' tests. Furthermore, satisfying the type checker as to the contents of the strings used may be painful for users, leading to a proliferation of casts or `type: ignore`s. Therefore, the type or the name argument has been reverted to str.
james-garner-canonical
requested review from
tonyandrewmeyer,
dimaqq and
IronCore864
Thanks, I can see that Charms that use |
|
Looking at loki example, what they do: class CompositeStatus(TypedDict):
"""Per-component status holder."""
# These are going to go into stored state, so we must use marshallable objects.
# They are passed to StatusBase.from_name().
k8s_patch: Tuple[str, str]
config: Tuple[str, str]
rules: Tuple[str, str]
retention: Tuple[str, str]
...
def _on_k8s_patch_failed(self, event: K8sResourcePatchFailedEvent):
self._stored.status["k8s_patch"] = to_tuple(BlockedStatus(cast(str, event.message)))
...
def to_status(tpl: Tuple[str, str]) -> StatusBase:
"""Convert a tuple to a StatusBase, so it could be used natively with ops."""
name, message = tpl
return StatusBase.from_name(name, message)
...
def _on_collect_unit_status(self, event: CollectStatusEvent):
# "Pull" statuses
# TODO refactor _configure to turn the "rules" status into a "pull" status.
# "Push" statuses
for status in self._stored.status.values():
event.add_status(to_status(status))Arguably that code could be refactored, though it's unclear exactly how. One option would be to change str to a union of literals... Another option would be observe that In any case, good call to keep this PR small and leave more extensive changes for later / slower process. |
|
Interesting observation about data persisting across library versions. I definitely wouldn't want to remove any runtime checks when adding type checks (though here it would just fail with a relatively traceable KeyError on dictionary access). The data flow you've pointed out in loki could indeed simply be annotated with I wonder if it would be worth making the |
There was a problem hiding this comment.
Generally looks good - a couple of small notes/queries.
One other thing I noticed while reviewing the docs to see if they fit with this: the add_status method accepts both error and unknown, and the doc lists those. It reads very much like you can add those statuses, but especially for error if you include that then it'll definitely fail because it will "win" the evaluation and then can't be set.
Maybe it makes this PR too big to adjust that, but if so we should probably open a ticket about that too.
|
115 out of 115 (100%) runs passed. 🚀 I would have imagined at least some charm to explicitly set |
Unclear how to narrow type of value given that type checking doesn't excellently support properties with differing types for getter and setter, so sticking with a simple `cast` for now.
Either work, as we're just testing that the type not being bool or str raises an error, but bare object may raise eyebrows for readers.
InvalidStatusError is a subclass of ModelError, so it shouldn't break any user code, and anyway it doesn't look like people were setting invalid statuses since Juju would give them an error at runtime currently.
I've opened an issue to discuss this (#1356). |
|
I'm going to defer work on (#1356 -- i.e. making it an error to call _ReadOnlyStatusName = Literal['error', 'unknown']
_SettableStatusName = Literal['active', 'blocked', 'maintenance', 'waiting']
_StatusName = Union[_SettableStatusName, _ReadOnlyStatusName]
_StatusDict = TypedDict('_StatusDict', {'status': _StatusName, 'message': str})
_SETTABLE_STATUS_NAMES: Tuple[_SettableStatusName, ...] = get_args(_SettableStatusName)Prefixes, but slightly more comprehensible in context? _StatusNameReadOnly = Literal['error', 'unknown']
_StatusNameSettable = Literal['active', 'blocked', 'maintenance', 'waiting']
_StatusName = Union[_StatusNameSettable, _StatusNameReadOnly]
_StatusDict = TypedDict('_StatusDict', {'status': _StatusName, 'message': str})
_SETTABLE_STATUS_NAMES: Tuple[_StatusNameSettable, ...] = get_args(_StatusNameSettable)Are suffixes better here? |
Calling add_status with an ErrorStatus *will* eventually result in an InvalidStatusError. Calling add_status with an UnknownStatus *may* eventually result in an InvalidStatusError.
|
I think this is ready for a final review and merge if anyone wants to take another look and provide the second approval (@tonyandrewmeyer, @benhoyt). |
The plan is to make using ErrorStatus and UnknownStatus here an error
|
Discussed changes have been made, happy to approve and merge @benhoyt ? |
…set_status (canonical#1354) Document the status values that are valid arguments for `_ModelBackend.status_set`, as requested in canonical#1343, via type annotation. The `status` argument was previously type hinted only as `str`. However, this argument can really only be one of four valid statuses that Juju's `status-set` will accept `('active', 'blocked', 'maintenance', 'waiting')`. Since the arguments to `status_set` are already partially validated, (`is_app` being a bool in `status_set`, `status` and `message` being strings in `Application.status` and `Unit.status`), and and the valid values for `status` are now encoded and associated with this method, we can check that `status` is valid in `status_set` and raise an `InvalidStatusError` immediately if not, rather than catching `status-set`'s exit code and only then raising a `ModelError`. Tests are updated to account for this (`test_collect_status_priority` split into `test_collect_status_priority_valid` and `test_collect_status_priority_invalid` since `status-set` isn't called in the invalid case, and `test_local_set_invalid_status` is updated to account for this too). Since `is_app` and `status` are both validated in `status_set`, it makes sense to also validate the type of `message` here, removing redundant checks in `Application.status` and cleaning up a `fixme` in `Unit.status`. A test is added to cover this (`test_status_set_message_not_str_raises`). `StatusBase.name` is also now annotated as only being one of the six valid Juju statuses, and the type alias for this literal (`StatusName`) is exposed in the public api. Finally, a couple of broken unit tests that interact with `StatusBase` and `status_set` (`test_base_status_instance_raises` and `test_status_set_is_app_not_bool_raises` respectively) are now fixed.
Document the status values that are valid arguments for
_ModelBackend.status_set, as requested in #1343, via type annotation. Thestatusargument was previously type hinted only asstr. However, this argument can really only be one of four valid statuses that Juju'sstatus-setwill accept('active', 'blocked', 'maintenance', 'waiting'). This new type information is propagated toStatusBaseand relevant tests, including_TestingModelBackend.Since the arguments to
status_setare already partially validated, (is_appbeing a bool instatus_set,statusandmessagebeing strings inApplication.statusandUnit.status), and and the valid values forstatusare now encoded and associated with this method, we can check thatstatusis valid instatus_setand raise aModelErrorimmediately if not, rather than catchingstatus-set's exit code and only then raising aModelError.Tests are updated to account for this (
test_collect_status_prioritysplit intotest_collect_status_priority_validandtest_collect_status_priority_invalidsincestatus-setisn't called in the invalid case, andtest_local_set_invalid_statusis updated to account for this too).Since
is_appandstatusare both validated instatus_set, it makes sense to also validate the type ofmessagehere, removing redundant checks inApplication.statusand cleaning up afixmeinUnit.status. A test is added to cover this (test_status_set_message_not_str_raises).Finally, fix a couple of broken unit tests that interact with
StatusBaseandstatus_set(test_base_status_instance_raisesandtest_status_set_is_app_not_bool_raisesrespectively).