fix: correct the value of additional_properties in the action meta in Juju 4
#2250
Conversation
tonyandrewmeyer
force-pushed
the
fix-additionalProperties-default-flip
branch
from
dd21cad to
da577e0
Compare
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
I've shared some thoughts in a separate comment, but ultimately I suspect this current implementation might be the right tradeoff, so I'm approving this PR.
| self.required = raw.get('required', []) # [<parameter name>, ...] | ||
| self.additional_properties = raw.get('additionalProperties', True) | ||
| # The default in Juju 4 is False. The default in earlier Juju versions is True. | ||
| juju_version = JujuVersion(os.environ.get('JUJU_VERSION', '0.0.0')) |
There was a problem hiding this comment.
Significantly, it seems like this is the first thing we have under CharmMeta that actually depends on the Juju context, rather than being purely derived from the charm's YAML files.
I wonder if it would be cleaner to update ActionMeta and CharmMeta to take a JujuContext. This would have to be optional, since the constructors are public, and I guess the default behaviour if the context isn't provided would still have to be loading it from the environment. And if we want this to continue to work outside a real Juju context, we'd still need to provide a default value for JUJU_VERSION (among others), since JujuContext.from_environ will error if it isn't set.
This seems like more trouble than it would be worth right now, so I guess just checking the version is the right call, even if it feels a little bit messy.
It does seem like a shame to essentially reimplement the deprecated JujuVersion.from_environ here. WDYT about moving the logic to a private JujuVersion._from_environ method and calling that here? Maybe it's simple enough that it's not worth it, but it could be tidier if we end up needing to do more of this in future.
There was a problem hiding this comment.
This seems like more trouble than it would be worth right now, so I guess just checking the version is the right call, even if it feels a little bit messy.
Yes, that's basically what I said in the PR description 😉.
It does seem like a shame to essentially reimplement the deprecated
JujuVersion.from_environhere. WDYT about moving the logic to a privateJujuVersion._from_environmethod and calling that here? Maybe it's simple enough that it's not worth it, but it could be tidier if we end up needing to do more of this in future.
I'm not sure it's worth it for a single line. I guess there is the JUJU_VERSION constant and the weird "use 0.0.0 if not present" default, but I'm not sure those merit avoiding duplicating it.
There was a problem hiding this comment.
This seems like more trouble than it would be worth right now, so I guess just checking the version is the right call, even if it feels a little bit messy.
Yes, that's basically what I said in the PR description 😉.
Just reproducing your working.
It does seem like a shame to essentially reimplement the deprecated
JujuVersion.from_environhere. WDYT about moving the logic to a privateJujuVersion._from_environmethod and calling that here? Maybe it's simple enough that it's not worth it, but it could be tidier if we end up needing to do more of this in future.I'm not sure it's worth it for a single line. I guess there is the
JUJU_VERSIONconstant and the weird "use 0.0.0 if not present" default, but I'm not sure those merit avoiding duplicating it.
Yeah, I'm on the fence -- I think it's more worth it for the 0.0.0 default than JUJU_VERSION, but probably fine either way.
There was a problem hiding this comment.
Based on a quick scan around the code, it seems that using 0.0.0 by default is an established pattern. So keeping the logic here seems fine to me (but I'd also be fine either way).
There was a problem hiding this comment.
Pull request overview
This PR fixes the default value of additional_properties in ActionMeta to align with Juju 4's behavior change, where the default value of additionalProperties flips from True to False.
Key Changes:
- Added version-based logic to determine the correct default for
additional_propertiesbased on JUJU_VERSION - Added parametrized tests to verify correct behavior across Juju versions (2.9, 3.6.12, 4.0.0, 4.1)
- Used JujuVersion comparison to determine whether to default to True (Juju < 4) or False (Juju >= 4)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ops/charm.py | Added JujuVersion import and version-based logic in ActionMeta.init to set the correct default for additional_properties based on whether Juju version is < 4.0.0 |
| test/test_charm.py | Added parametrized test to verify additional_properties defaults correctly for Juju versions 2.9, 3.6.12, 4.0.0, and 4.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
I left a comment in the Juju bug.
juju/juju#21294 (comment)
TL;DR: I'm unclear if the default value change is slated for Juju 3.6 too.
4 participants
Juju 4 flips the default value of additionalProperties from true to false (see juju/juju#21294).
We expose this value in
ActionMeta.additional_properties. The PR adjusts this so that the default value (ifadditionalPropertiesis not in the YAML) is correct in both Juju 4 and earlier Juju.This loads the version from the environment, which we try to avoid (using the existing
juju_contexton the model instead). However,ActionMetadoesn't have access to the model object, and nor doesCharmMeta, so piping in access would be somewhat messy. I've gone with the simpler solution, but please consider this in the review.Fixes #2188