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
| _JUJU_4_REMOVED_DATABAG_KEYS = ('private-address',) | ||
|
|
||
|
|
||
| def _strip_juju4_defaults(state: State, juju_version: str) -> None: |
There was a problem hiding this comment.
Couldn't we use JujuVersion instead of custom logic to parse the version? (As in mocking.py)
There was a problem hiding this comment.
I feel like the value of JujuVersion is the properties, not parsing a very simple string. But sure, I can switch to that.
| 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.
2 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