-
Notifications
You must be signed in to change notification settings - Fork 135
fix: don't include private-address in default testing.Relation databags on Juju 4 #2618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
64bf9f6
13038a0
9e6a02b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -716,12 +716,35 @@ def _validate_databag(self, databag: dict[str, str]): | |
|
|
||
|
|
||
| _DEFAULT_IP = '192.0.2.0' | ||
| # Juju 3.x populates `private-address` in every relation databag, but Juju 4 | ||
| # does not. The dataclass `default_factory` runs at `Relation()` construction | ||
| # time, before the test's `juju_version` is known, so we always include | ||
| # `private-address` here and strip it later in `_strip_juju4_defaults` if the | ||
| # `Context.juju_version` is 4.0 or later. | ||
| _DEFAULT_JUJU_DATABAG = { | ||
| 'egress-subnets': _DEFAULT_IP, | ||
| 'ingress-address': _DEFAULT_IP, | ||
| 'private-address': _DEFAULT_IP, | ||
| } | ||
|
|
||
| # Keys in `_DEFAULT_JUJU_DATABAG` that Juju 4 no longer auto-populates. | ||
| _JUJU_4_REMOVED_DATABAG_KEYS = ('private-address',) | ||
|
|
||
|
|
||
| def _strip_juju4_defaults(state: State, juju_version: str) -> None: | ||
| """Strip Juju-3-only default databag keys from `state` for Juju 4+. | ||
|
|
||
| Mutates the databag dicts in place; only keys still set to the default | ||
| sentinel IP are removed, so explicit user-set values are preserved. | ||
| """ | ||
| if ops.JujuVersion(juju_version).major < 4: | ||
| return | ||
| for relation in state.relations: | ||
| for databag in relation._databags: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My review agent tells me that this would remove
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| for key in _JUJU_4_REMOVED_DATABAG_KEYS: | ||
| if databag.get(key) == _DEFAULT_IP: | ||
| del databag[key] | ||
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True, kw_only=True) | ||
| class Relation(RelationBase): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.