status_set: document (or link to) the list of valid values
#1343
Labels
Comments
|
Hey Leon, this is the docstring of a method on a private (underscore-prefixed |
|
Same question about why it's worthwhile (given that if someone is building something on top of ops so has a legitimate reason to use a private class, they ought to know enough about Juju to know the possible values), but for what it's worth, I'd suggest we would do this by just type annotating |
james-garner-canonical
added a commit
that referenced
this issue
Sep 18, 2024
…set_status (#1354) Document the status values that are valid arguments for `_ModelBackend.status_set`, as requested in #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.
|
This was fixed in #1354. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, the docstring doesn't mention that "status" is limited to a set of valid values.
operator/ops/model.py
Lines 3405 to 3410 in 7969187
The text was updated successfully, but these errors were encountered: