fix: secret-info-get cannot be provided with both an ID and a label

[tonyandrewmeyer]

Ops currently silently ignores the label if both are provided by the caller, so we need to preserve that behaviour. This is done in the model, because hookcmds is a low-level API that exposes the raw Juju errors when callers make mistakes (but the existing type overloads will alert callers that both should not be provided).

[james-garner-canonical]

Fix looks correct to me, but WDYT about tests asserting on the errors raised in these two cases:

1. Call with both ID and label.
2. Call with neither.

[tonyandrewmeyer]

1. Call with both ID and label.
2. Call with neither.

In test_hookcmds, that would be:

def test_secret_info_get_no_id_or_label(run: Run):
    run.handle(
        ['secret-info-get', '--format=json'],
        returncode=1, stderr='require either a secret URI or label',
    )
    with pytest.raises(hookcmds.Error):
        hookcmds.secret_info_get()  # type: ignore


def test_secret_info_get_id_and_label(run: Run):
    run.handle(
        ['secret-info-get', '--format=json', 'secret:123', '--label', 'lbl'],
        returncode=1, stderr='specifify either a secret URI or label but not both',
    )
    with pytest.raises(hookcmds.Error):
        hookcmds.secret_info_get(id='secret:123', label='lbl')  # type: ignore

I don't feel like this is really testing anything. The Juju behaviour is encoded in the test itself (the Run setup), so the test is really just asserting that if there's an error in the CLI call there will be an error raised, and there's already a test for that. The hookcmds code doesn't have code that deals with neither or both provided, it (by design) lets that fall through to Juju.

If there were tests at the model level, I don't think it's possible to do the neither case, because you can't get a Secret object that doesn't have either an ID or a label (or both). We already have a test for the both case (it's the one that's adjusted in this PR).

[james-garner-canonical]

Thanks for explaining.

I agree that the hookcmds level tests don't add much, and it's better to just pass through the Juju behaviour.

I had kind of imagined tests at the model level, but I see how the 'both' case is already handled, and the 'neither' case doesn't make sense.

So LGTM without reservations now.

[benhoyt]

Couple of comments, but approving optimistically.

[benhoyt]

Presumably it's an error if neither id or label are specified? Should we have an else: raise TypeError(...) case to handle that?

Also, it might be a bit simpler / more type safe to do this (but I don't have strong feelings):

if id is not None:
    with self._wrap_hookcmd('secret-info-get', id=id):
        raw = hookcmds.secret_info_get(id=id)
elif label is not None:
    with self._wrap_hookcmd('secret-info-get', label=label):
        raw = hookcmds.secret_info_get(label=label)

[tonyandrewmeyer]

Presumably it's an error if neither id or label are specified? Should we have an else: raise TypeError(...) case to handle that?

We can't do that in _ModelBackend because it would break backwards compatibility. We could do it in hookcmds (because _ModelBackend would have already filtered out the label argument), but overall the design for hookcmds is that errors are left to be provided by Juju, not duplicated in Python code. A type check will show that it's incorrect (because of the existing overloads). I'd prefer to keep it that way.

[benhoyt]

I'd prefer to keep it that way.

Thanks for clarifying. Yep, that's fine.

[tonyandrewmeyer]

Presumably it's an error if neither id or label are specified? Should we have an else: raise TypeError(...) case to handle that?

Ah, I misread this, sorry. I thought you said both but you said neither. I don't think it's possible to get to that state in _ModelBackend (see earlier comment to James) but I can add an else to be certain, sure.