operator icon indicating copy to clipboard operation
operator copied to clipboard

chore: fix error message for non-owned secrets in ops.testing

Open Copilot opened this issue 3 months ago • 2 comments

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 that owner=None indicates 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 &gt; def _check_can_manage_secret( &gt; self, &gt; secret: Secret, &gt; ): &gt; if secret.owner is None: &gt; raise SecretNotFoundError( &gt; 'this secret is not owned by this unit/app or granted to it. ' &gt; 'Did you forget passing it to State.secrets?', &gt; ) &gt; ... &gt; > > 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: https://github.com/dimaqq/operator/pull/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. 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>@dimaqq

quick fix: https://github.com/dimaqq/operator/pull/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. 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

✨ 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 avatar Nov 14 '25 12:11 Copilot

@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?

dimaqq avatar Nov 15 '25 13:11 dimaqq

@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.

Copilot avatar Nov 15 '25 13:11 Copilot