fix: adjust Harness secret behaviour to align with Juju #1248
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
…ets. Juju may change secret-info-get so that it returns 'permission denied' when the charm doesn't have the required permissions, which will require an update here (and a tricky situation where the exception raised will depend on the Juju version) but let's deal with that when the time comes.
benhoyt
approved these changes
Jun 6, 2024
benhoyt
approved these changes
Jun 7, 2024
|
@IronCore864 would you mind reviewing this when you have time? Thanks! |
dimaqq
reviewed
Jun 20, 2024
dimaqq
approved these changes
Jun 20, 2024
IronCore864
approved these changes
Jun 25, 2024
amandahla
pushed a commit
to amandahla/operator
that referenced
this pull request
Jun 26, 2024
Adjust Harness so that the errors match what Juju does in practice (I tested each case - the results are in a table in a comment in canonical#1229). * When Juju responds with a message including "not found", we raise `SecretNotFoundError` * When Juju responds with any other message (such as "permission denied"), we raise `ModelError` * When Juju succeeds and only fails at the completion of the hook, we raise `RuntimeError` - in production, the charm cannot catch this, but we do want to surface this error, so `RuntimeError` seems most approprate In addition: * Update the documentation to clarify when secret content is cached (this was a request from the discussion with the data platform team in Madrid) * Correct the documentation regarding when `ModelError` and when `SecretNotFoundError` will be raised with the secret methods * Don't clear the `Secret` object local cache of the secret content on `set_contents`. The contents won't change until `refresh` is used, so there's no point forcing the next call to get the content from Juju. Juju may change the responses so that there is increased consistency (probably via [this bug](https://bugs.launchpad.net/juju/+bug/2067336)) but it seems best if we fix the behaviour to match now, and then if Juju changes in the future, we can update ops then (also deciding at that time how to handle the issue that different Juju versions will have different behaviour). Fixes canonical#1229.
tonyandrewmeyer
added a commit
to tonyandrewmeyer/operator
that referenced
this pull request
Jun 26, 2024
Adjust Harness so that the errors match what Juju does in practice (I tested each case - the results are in a table in a comment in canonical#1229). * When Juju responds with a message including "not found", we raise `SecretNotFoundError` * When Juju responds with any other message (such as "permission denied"), we raise `ModelError` * When Juju succeeds and only fails at the completion of the hook, we raise `RuntimeError` - in production, the charm cannot catch this, but we do want to surface this error, so `RuntimeError` seems most approprate In addition: * Update the documentation to clarify when secret content is cached (this was a request from the discussion with the data platform team in Madrid) * Correct the documentation regarding when `ModelError` and when `SecretNotFoundError` will be raised with the secret methods * Don't clear the `Secret` object local cache of the secret content on `set_contents`. The contents won't change until `refresh` is used, so there's no point forcing the next call to get the content from Juju. Juju may change the responses so that there is increased consistency (probably via [this bug](https://bugs.launchpad.net/juju/+bug/2067336)) but it seems best if we fix the behaviour to match now, and then if Juju changes in the future, we can update ops then (also deciding at that time how to handle the issue that different Juju versions will have different behaviour). Fixes canonical#1229.
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 Harness so that the errors match what Juju does in practice (I tested each case - the results are in a table in a comment in #1229).
SecretNotFoundErrorModelErrorRuntimeError- in production, the charm cannot catch this, but we do want to surface this error, soRuntimeErrorseems most approprateIn addition:
ModelErrorand whenSecretNotFoundErrorwill be raised with the secret methodsSecretobject local cache of the secret content onset_contents. The contents won't change untilrefreshis used, so there's no point forcing the next call to get the content from Juju.Juju may change the responses so that there is increased consistency (probably via this bug) but it seems best if we fix the behaviour to match now, and then if Juju changes in the future, we can update ops then (also deciding at that time how to handle the issue that different Juju versions will have different behaviour).
Fixes #1229.