7.0 - API adjustments, Pebble check events, and container exec improvements #183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'll rebase to fix the conflicts maybe later today or Monday. |
Update scenario/consistency_checker.py Co-authored-by: PietroPasotti <[email protected]> Fix broken files. Update scenario/mocking.py
Fix typo in comment. Remove support for directly running custom events. Update tests and docs to match final (hopefully\!) API decision. Style fixes. Fix tests. The failing test are (a) ones that need to be rewritten for the new system, (b) ones to do with custom events. Remove the old shortcuts on state components. Remove old event properties from Secret. Style fixes. Move the checks that were on binding to the consistency checker. Update rubbish event tests. These are not as valuable without support for emitting custom events directly, but it seems worthwhile to leave them in so that if we add custom event emitting back in the future they're here to build off. Update tests now that emitting custom events is not possible. Minor clean-up. Fix typo found in review. Fix tests for relation.unit.
Update tests and docs to match final (hopefully\!) API decision. Fix tests. The failing test are (a) ones that need to be rewritten for the new system, (b) ones to do with custom events. Various README updates. Typo Style fixes.
Most particularly, the relation_id to id change was merged in main (mistakenly) and then unmerged, so the rebasing undid it, so that is redone. Also, the basic changes for Pebble notices and Cloud Configuration are done in here, although those need to be double-checked to make sure they make sense with the updated API.
What do you think?
* Remove _DCBase. * Adjust the consistency checker and expose the Resource class. * Finish the conversion (all tests pass). * Don't add __eq__ for now. * Update scenario/mocking.py * Allow getting components by passing in the old entity. * Revert back to the simpler get_ methods. * Fix merges. * Remove unused method (was used in the old binding, not generally useful). * Add a basic test for resources. * Add a basic test for resources. * Make networks a set as well.
Adds classes that match the ops status classes: * UnknownStatus * ActiveStatus * WaitingStatus * MaintenanceStatus * BlockedStatus * ErrorStatus
… hashable objects can be passed, but still convert to frozenset underneath. (#160)
* Add support for Pebble checks. Also update the support for Pebble notices to be aligned with the 7.x approach.
This moves the default Network into the `Network` initialisation, rather
than as a separate classmethod that provides a `Network` object. This is
more consistent with the rest of the Scenario objects, which try to have
useful defaults where possible.
For the most simple case:
```python
# Previously
network = Network.default()
# Now
network = Network("default") # The name is needed because of the `State` change elsewhere
```
To override elements of the default is a little bit more work,
particularly if it's in the nested `Address` object, but it doesn't seem
too bad:
```python
# Previously
network = Network.default(private_address="129.0.2.1")
# Now
network = Network("foo", [BindAddress([Address("129.0.2.1")])])
```
Adjust testing secret management to be simpler - in particular, to avoid needing to manually manage revisions, which should be transparent to charms. Rather than `Secret`s having a dictionary of revision:content-dict, they have two content dictionaries, `tracked_content` (required) and `latest_content` (set to the same value as `tracked_content` if not provided). This matches what charms can see: only either the tracked revision or the latest revision. A new attribute, `removed_secret_revisions` is added to `Context` to track removal of secret revisions in the `secret-remove` and `secret-expired` hooks. Calling `secret-remove --revision` in those hooks must be done, so should be tested, but don't actually change the state that's visible to the charm (for `secret-remove` the whole point is that the secret revision is no longer visible to anyone, so it should be removed). Tests could mock the `secret_remove` method, but it seems cleaner to provide a mechanism, given that this should be part of any charm that uses secrets. Charms should only remove specific revisions in the `secret-remove` and `secret-expired` hooks, and only remove the revision that's provided, but it is possible to remove arbitrary revisions. Modelling this is complicated (the state of the Juju secret is a mess afterwards) and it is always a mistake, so rather than trying to make the model fit the bad code, an exception is raised instead. A warning is logged if a secret revision is created that is the same as the existing revision - in the latest Juju this is a no-op, but in earlier version it's a problem, and either way it's something that the charm should avoid if possible. --------- Co-authored-by: PietroPasotti <[email protected]>
The `run_action()` method (both standalone and in the context manager)
are removed. This means that all events, including action events, are
emitted with the same `run()` call, and both return the output state.
To get access to the results of actions, a new `action_output` attribute
is added to the `Context`. This is a simplified representation of the
Juju `operation` object (and the `task` objects in them), which are
displayed with `juju run`, but also available via `juju show-operation`.
The class has the same name as the Harness `ActionOutput` and will be
consolidated into a single class when Scenario is added to ops.
For example:
```python
out = ctx.run_action(Action("backup", params={"output": "data.tar.gz"}), State())
assert out.results == {"size": 1234}
assert out.logs = [..., ...]
assert out.state...
state = ctx.run(ctx.on.action("backup", params={"output": "data.tar.gz"}), State())
assert ctx.action_output.results == {"size": 1234}
assert ctx.action_output.logs = [..., ...]
assert state...
```
When the charm calls `event.fail()`, this raises an exception, in the
same way that Harness does. For example:
```python
out = ctx.run_action("bad-action", State())
assert out.failure == "couldn't do the thing"
with pytest.raises(ActionFailed) as exc_info:
ctx.run(ctx.on.action("bad-action"), State())
assert exc_info.value.message == "couldn't do the thing"
```
The advantage of this is that tests that the action is successful do not
need to have `assert ctx.action_output.status != "failed"`, which is
easy to miss.
In addition, the `Context.manager` and `Context.action_manager` methods
are replaced by the ability to use the `Context` object itself as a
context manager.
For example:
```python
ctx = Context(MyCharm)
with ctx(ctx.on.start(), State()) as event:
event.charm.prepare()
state = event.run()
assert event.charm...
```
The same code is also used (with `ctx.on.action()`) for action events.
Advantages:
* Slightly shorter code (no ".manager" or ".action_manager")
* Avoids naming complications with "manager", "action_manager" and the
various alternatives proposed in #115.
The `.output` property of the context manager is also removed. The
context manager will still handle running the event if it's not done
explicitly, but if that's the case then the output is not available. We
want to encourage people to explicitly run the event, not rely on the
automated behaviour - although I think it does make sense that it does
run, rather than raise or end in a weird situation where the event never
ran.
This replaces #115 and #118, being a combination of ideas/discussion
from both, plus incorporating the unification of run/run_action
discussed here, and the "action fail raises" discussed elsewhere.
Also, as drive-by changes while names are being made consistent:
* `_Port` becomes `Port`
* `_RelationBase` becomes (again) `RelationBase`
---------
Co-authored-by: PietroPasotti <[email protected]>
When we get a secret and provide both ID and label, the label should update to the provided one. This was previously missing in Scenario. Fixes #95
A couple of changes to simplify setting up the queue of deferred events: * Adjusted the docs for `DeferredEvent` to strongly suggest that it's for internal use only and people should use the `.deferred()` method of an `_Event` instead. It's still required internally (to go to/from the ops state) and might be needed in obscure cases, but this reduces the number of ways people need to be aware of by one. * The `scenario.deferred()` helper is removed. When using this method for events that link to components (relations, containers, etc) it was possible to not have the event set up correctly, and it didn't really seem to offer much over `.deferred()`. Removing it leaves one normal way to create deferred events. Finally, setting up the `DeferredEvent` snapshot data only handled workload events and relation events (and wasn't always correct for all relation events). It should now cover all current events with the right snapshot.
…mocking (#124) Firstly, some simple renaming: * `Container.exec_mocks` becomes `Container.execs` * `Container.service_status` becomes `Container.service_statuses` * `ExecOutput` becomes `Exec` A behaviour change that is a bugfix: * When a process exits non-zero, the `ExecError` should have the stdout/stderr if `wait_output` was used (it's not readable afterwards by the charm, although the Scenario code doesn't enforce that). Some more substantial changes: * Provide the ability to get the exact command, the stdin, and all the other args that the charm used with the process (everything from os.testing's `ExecArgs`), via a new context attribute `exec_history`, which is a (default) dict where the key is the container name and the value is a list of Pebble exec's that have been run. * Support the same "find the closest match to this prefix" system for commands as Harness does We could add more of the functionality that Harness has, but I think this is a solid subset (I've wanted to be able to get to the stdin in writing tests myself, and the simple mock matching seems handy). It should be easy enough to extend in the future without needing API changes, I think, since this now has both input and output. The key parts that are missing are properly supporting binary IO and the execution context. --------- Co-authored-by: PietroPasotti <[email protected]>
A collection of small changes that generally fall into "remove x from the public API": * Remove the `with_can_connect`, `with_leadership`, and `with_unit_status` convenience methods from `State`. * Makes `next_relation_id`, `next_action_id`, `next_storage_index`, and `next_notice_id` private. * Removes `Context.output_state`. * Makes all the *_SUFFIX constants private. * Makes all the *_EVENTS constants private, except `META_EVENTS`, which is removed. * Makes `capture_events` private (and consolidates capture_events.py into runtime.py). * Makes both `hook_tool_output_fmt` methods private. * Makes `normalize_name` private. * Moves all of the Scenario error exception classes (the ones that no-one should be catching) to a scenario.errors namespace/module. * Renames the consistency checker module to be private. * Makes `DEFAULT_JUJU_VERSION` and `DEFAULT_JUJU_DATABAG` private. * Adds various classes/types to the top-level scenario namespace for use in type annotations: * Removes `AnyRelation` in favour of using `RelationBase` * Removes `PathLike` in favour of `str|Path`. Fixes #175.
Since 7.x has so many breaking changes, add an `UPGRADING.md` doc that lists them all, with before/after examples. Note that this is *not* the full release notes for 7.x - it doesn't cover any of the non-breaking changes, other than when they are incidentally used as part of the examples. We can write release notes for the release fairly shortly, which can be more comprehensive (but probably not have as many examples).
Expands the docstrings so that when used with Sphinx's autodoc they will fit in with the other ops docs on ops.rtd but also still work standalone. The Sphinx autodoc system uses the `__new__` signature in preference to the `__init__` one, which means that by default all classes that are using the `_MaxPositionalArgs` class have a `*args, **kwargs` signature, which is not informative. custom_conf.py monkeypatches Sphinx to work around this, including tweaking the signature so that the `*` appears in the correct place for the maximum number of positional arguments. Also bumps the Sphinx version to align with ops. --------- Co-authored-by: PietroPasotti <[email protected]>
* Remove `Manager.output` as noticed by @PietroPasotti * If `Context.run_action` is called, raise an error but point them towards the solution. * If `Context.run` is called with a string or callable event, raise an error but point them towards the solution * If `Relation.relation_id` is used, raise an error but point towards `.id`
|
ready to merge! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A variety of API changes with the goal of adopting Scenario as the new "ops.testing". A lot of breaking changes, but with the intention that these are rare going forward (a similar cadence with ops).
tox test-readmeto check the code blocks in the READMEMore detailed notes in each of the individual PRs/commits to the branch.