-
Notifications
You must be signed in to change notification settings - Fork 125
chore: fix error message for non-owned secrets in ops.testing #2177
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: dimaqq <662249+dimaqq@users.noreply.github.com>
Co-authored-by: dimaqq <662249+dimaqq@users.noreply.github.com>
|
@copilot there's no need for the new test; existing tests already cover this. |
Co-authored-by: dimaqq <662249+dimaqq@users.noreply.github.com>
Done. Removed the new test and updated the error message to keep the original Changes in commit ab4e79e. |
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping "or granted to it" is a good improvement, but we should also fix the recommendation as well, since it's also wrong.
Also: the PR description is quite messy with all the Copilot stuff. Let's make sure to either clear that out now or remove it when doing the merge.
| raise SecretNotFoundError( | ||
| 'this secret is not owned by this unit/app or granted to it. ' | ||
| 'this secret is not owned by this unit/app. ' | ||
| 'Did you forget passing it to State.secrets?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion is not correct - any time we get into _check_can_manage_secret we have retrieved the secret from the state, so we know that it's there.
What we should suggest is that they forgot to set the owner attribute on the testing.Secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered that based on your comment on the issue. Sadly it looks like my comment along those lines got eaten (I probably left it in 'preview' mode before I submitted my review). Thanks for catching this.
Error message suggested fix isn't correct per @tonyandrewmyer's comment.
The error message when attempting to manage a secret with
owner=Nonewas contradictory. The documentation states thatowner=Nonemeans the secret has been granted for read access, but the error claimed the secret was "not owned by this unit/app or granted to it".Changes:
_check_can_manage_secret: Changed from "not owned by this unit/app or granted to it. Did you forget passing it to State.secrets?" to "not owned by this unit/app. Did you forget passing it to State.secrets?" to align with the documented behavior thatowner=Noneindicates a granted (read-only) secret.The error now correctly conveys that secrets need to be owned (
owner='unit'orowner='app') for management operations, not merely granted, while keeping the original hint about passing the secret toState.secrets.<issue_title>Fix docs/impl inconsistency in Secret ownership in ops[testing]</issue_title>
><issue_description>Code:
>
>
py > def _check_can_manage_secret( > self, > secret: Secret, > ): > if secret.owner is None: > raise SecretNotFoundError( > 'this secret is not owned by this unit/app or granted to it. ' > 'Did you forget passing it to State.secrets?', > ) > ... >>
> Docs:
>
> > owner: Literal['unit', 'app', None] = None
> > Indicates if the secret is owned by this unit, this application, or another application/unit.
> >
> > If None, the implication is that read access to the secret has been granted to this unit.</issue_description>
>
><agent_instructions>Make a PR with a minimal change to fix this.</agent_instructions>
>
> ## Comments on the Issue (you are @copilot in this section)
>
>
><comment_new>@dimaqq
> cc @tonyandrewmeyer </comment_new>
><comment_new>@dimaqq
> In term of logic the above is actually correct (this unit can't manage granted secrets).
>
> In term of the error message though, this is confusing, if None really means granted.</comment_new>
><comment_new>@dimaqq
> quick fix: dimaqq#72
>
> I want to think a bit more about a more comprehensive fix... maybe.</comment_new>
><comment_new>@tonyandrewmeyer
> I don't think the code or doc is wrong.
Nonemeans that it's a secret that the unit only has view access to, so the unit can't do management of the secret, but can view it.>
> I suspect maybe that error text was wrongly copied from somewhere else. It should instead say that the secret is not owned by the unit/app, did you forget to set the owner.</comment_new>
>
>
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.