Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: clarify that SecretNotFound may be raised for permission errors #1231

Merged
merged 2 commits into from
May 29, 2024

Conversation

tonyandrewmeyer
Copy link
Contributor

As noticed in #1229, get_secret() may raise SecretNotFoundError if the secret does exist but the caller does not have permission to view it, so make this explicit in the documentation.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the point of #1229 that Ops will currently raise ModelError for permission/grant errors?

@benhoyt
Copy link
Collaborator

benhoyt commented May 28, 2024

Oh, I see -- we're closing #1229, and this PR goes on the assumption that the Juju bug will be fixed.

@tonyandrewmeyer
Copy link
Contributor Author

Oh, I see -- we're closing #1229, and this PR goes on the assumption that the Juju bug will be fixed.

Yes - my assumption is that since @wallyworld agrees that Juju should be consistent that the Juju bug will get fixed.

But also: this is already the case for everything other than user secrets. If we wanted it to reflect the current behaviour, then it should have this change and also a new "raises" item of "ModelError: when the secret is a user secret and the charm does not have permission to view it". My preference would be to not add that now on the assumption that it won't be true soon (for some value of "soon").

@tonyandrewmeyer tonyandrewmeyer requested a review from benhoyt May 28, 2024 04:14
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me -- thanks.

@benhoyt benhoyt merged commit 6640f68 into canonical:main May 29, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants