chore: fix error message for non-owned secrets in ops.testing
The error message when attempting to manage a secret with owner=None was contradictory. The documentation states that owner=None means 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:
-
Updated error message in
_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' or owner='app') for management operations, not merely granted, while keeping the original hint about passing the secret to State.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)
>
>None means 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>
>
- Fixes canonical/operator#2077
Original prompt
This section details on the original issue you should resolve
<issue_title>Fix docs/impl inconsistency in Secret ownership in ops[testing]</issue_title> <issue_description>Code:
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)
@dimaqq cc @tonyandrewmeyer@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>
quick fix: https://github.com/dimaqq/operator/pull/72@dimaqq I want to think a bit more about a more comprehensive fix... maybe.</comment_new> <comment_new>
I don't think the code or doc is wrong.@tonyandrewmeyer 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>
- Fixes canonical/operator#2077
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
@copilot there's no need for the new test; existing tests already cover this.
the exception text should be this secret is not owned by this unit/app. Did you forget passing it to State.secrets?
@copilot there's no need for the new test; existing tests already cover this. the exception text should be
this secret is not owned by this unit/app. Did you forget passing it to State.secrets?
Done. Removed the new test and updated the error message to keep the original State.secrets hint. The error message now reads: 'this secret is not owned by this unit/app. Did you forget passing it to State.secrets?'
Changes in commit ab4e79e.