fix: don't include private-address in default testing.Relation databags on Juju 4#2618
Conversation
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
Main comment is about (lack of) JujuVersion
| if major < 4: | ||
| return | ||
| for relation in state.relations: | ||
| for databag in relation._databags: |
There was a problem hiding this comment.
My review agent tells me that this would remove private-address from local_app_data and remote_app_data - if someone has set private-address to the default value in those databags. I feel like we're safe to assume nobody would ever do that... What do you think?
There was a problem hiding this comment.
I did wonder about this. There's some chance a charm could do this to make their Juju 4 databag look like a Juju 3 one. I'd argue that it's not a good idea, but it's not impossible.
If someone does that their tests would break and they wouldn't be able to get around that.
The only ideas I have for avoiding that are ugly (like a special object we set instead, which we can recognise later, but pretends to be the normal one when people work with their state).
I think this comes down to a decision about whether we want to have a fix in Scenario 8 so the Juju 4 behaviour is surfaced, only have a fix with something opt-in (where we could do this more cleanly), or only fix it in 9.0 (definitely could do it clean, but people's tests may need updates). Or someone comes up with an idea I haven't thought of, of course.
I lean towards what's in the PR, but I can definitely be convinced we should go another way.
There was a problem hiding this comment.
I'm OK with what's in this PR. My knowledge is a bit limited here, so no objection if you and James decide to go in a different direction.
I think it would be great to quantify the difference in breaky-ness so we can make an informed choice about whether this is the right fix for now. PS: An idea for making this PR's approach safer -- track if the default factory was called with a private marker on the relation object, and only replace in that case. But maybe that falls apart as soon as we hit |
In terms of running Hyrum? Or in my opinion given what code changes?
This feels objectively more break-y to me, because it impacts Juju 3 as well as Juju 4, and also the keys that are still injected in both. But I could implement it and run it over the fleet tests to get actual numbers.
Yeah, that sort of thing (as well as using a dict-subclass or something) is the kind of thing I was thinking of when I was saying there are "ugly" solutions. My preference would be to avoid those, and go with a clean solution (that I can't think of), the one in this PR, or an opt-in fix that becomes the default in 9.0. |
You're probably right, but with how cheap implementation is I'd love to see the Hyrum numbers for the different approaches. |
3 participants
At exec time, remove the
private-addresskey from the databags, if it's set to our default value and the mock Juju version is 4+.I don't love this solution, since we're editing the State after the user has finished with it, but it's only when we have the Context that the version is available, so there doesn't seem to be a backwards-compatible alternative. It would probably be better if the runtime injected the automated keys and they weren't there when the user had the State, or maybe if you had to provide a version when getting a fresh Relation, but we can't do either of those in 8.x. This change at least gives the Juju behaviour, even if it's a bit quirky.
Regarding the comment in the issue, what I am proposing is that we do this workaround so that the Scenario 8.x behaviour matches Juju 3 and 4, and we do something more elaborate and backwards incompatible in 9.0 (I added to the list in #2350). We could instead reject this change and opt in to new behaviour with a env flag or similar, but I feel even if we do that we would want this change so that the default behaviour is the Juju one.
Fixes #2185