search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
244 stars 119 forks source link

feat: expand the secret ID out to the full URI when only given the ID #1358

Closed tonyandrewmeyer closed 3 weeks ago

tonyandrewmeyer commented 3 weeks ago

Adjust the rules for getting the 'canonical' form of the Secret URI:

Also adjust the code that calls the canonicalization [sic] method so that the model UUID is always available in regular ops use. If someone is creating SecretInfo objects themselves then a None value for the UUID is the default, for backwards compatibility (but a warning is issued in that case).

Fixes #1312.

tonyandrewmeyer commented 3 weeks ago

Do you envision this breaking anyone's tests?

Good question. I don't see it breaking anyone's charms, since it's already the case that the id might be in this format, and the Juju tools all accept this form, and I couldn't find anyone using id for e.g. constructing strings. However, tests might indeed be expecting specific strings.

Hopefully not, but would be worth a super tox run.

Good call: 112 charms' tests passed with main, 110 with this branch. One of the failures was charm-simple-streams, which is because my branch is a little behind main and so is missing the fix in 2.16.1 (I checked there was nothing else). The other is temporal-worker-k8s-operator:

FAILED tests/unit/test_charm.py::TestCharm::test_valid_environment_config - AssertionError: {'ser[55 chars]r', 'startup': 'enabled', 'override': 'replace[867 chars...

And this is a legitimate bug :disappointed:. Harness isn't properly getting the secret because it can't handle the different types of URI like Juju can. I'll fix that and re-run.

Could we also run this on a real charm on Juju, for an end-to-end test?

Sorry, I always forget to mention I've done that. Juju 3.6b2, LXD (I don't think either of those should matter). With main:

unit-secret-full-id-1: 11:53:12 INFO unit.secret-full-id/1.juju-log id: secret:crf3hi1tvhl39c5lur5g, identifier crf3hi1tvhl39c5lur5g

With this branch:

unit-secret-full-id-1: 11:57:06 INFO unit.secret-full-id/1.juju-log id: secret://7e7c1d98-03ed-46de-804e-8275c52d5fe3/crf3hi1tvhl39c5lur5g, identifier crf3hi1tvhl39c5lur5g
tonyandrewmeyer commented 3 weeks ago

And this is a legitimate bug 😞. Harness isn't properly getting the secret because it can't handle the different types of URI like Juju can. I'll fix that and re-run.

They pass now, so this doesn't break the unit tests of at least those 112 charms, which makes me fairly confident it won't break for anyone.

tonyandrewmeyer commented 3 weeks ago

pls remove coverage.xml from the PR

Thanks! Guess which PR I worked on immediately prior to this :laughing:.