feat!: simplify testing secret management #168
Merged
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
Co-authored-by: PietroPasotti <[email protected]>
tonyandrewmeyer
changed the title
PietroPasotti
approved these changes
Aug 2, 2024
scenario/mocking.py
Outdated
| secret = Secret( | ||
| id=secret_id, | ||
| contents={0: content}, | ||
| latest=content, |
There was a problem hiding this comment.
and this is why I'd rather more explicit argument names :)
Secret(content=content reads so much better.
dimaqq
reviewed
Aug 2, 2024
Co-authored-by: PietroPasotti <[email protected]>
benhoyt
reviewed
Aug 7, 2024
| @@ -441,48 +442,6 @@ def __init__( | |||
| ): | |||
| """Represents a simulated charm's execution context. | |||
|
|
|||
| It is the main entry point to running a scenario test. | |||
There was a problem hiding this comment.
Just curious why this doc block is being removed in this PR?
There was a problem hiding this comment.
It's being removed because it's duplicated in the 7.0 HEAD - I moved it to Context.__doc__ when doing the autodoc work but must have incorrectly merged that in some branch because it ended up still in Context.__init__.__doc__ too.
It's being removed in this PR because Pietro noticed that it was duplicated in this PR.
I can tidy it up in a separate PR rather than this drive-by if that's preferred.
benhoyt
approved these changes
Aug 7, 2024
tonyandrewmeyer
added a commit
that referenced
this pull request
Sep 2, 2024
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]>
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.
Adjust testing secret management to be simpler - in particular, to avoid needing to manually manage revisions, which should be transparent to charms.
Rather than
Secrets having a dictionary of revision:content-dict, they have two content dictionaries,tracked_content(required) andlatest_content(set to the same value astracked_contentif not provided). This matches what charms can see: only either the tracked revision or the latest revision.A new attribute,
removed_secret_revisionsis added toContextto track removal of secret revisions in thesecret-removeandsecret-expiredhooks. Callingsecret-remove --revisionin those hooks must be done, so should be tested, but don't actually change the state that's visible to the charm (forsecret-removethe whole point is that the secret revision is no longer visible to anyone, so it should be removed). Tests could mock thesecret_removemethod, 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-removeandsecret-expiredhooks, 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.