Skip to content

fix: correct the value of additional_properties in the action meta in Juju 4 #2250

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.

Sign up for GitHub

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

Open
tonyandrewmeyer wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: correct the value of additional_properties in the action meta in Juju 4 #2250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da577e0
fix: correct the value of additional_parameters in Juju 4.
tonyandrewmeyer Dec 18, 2025
19a6902
Add tests for missing envvar and both explicit values.
tonyandrewmeyer Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ops/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import dataclasses
import enum
import logging
import os
import pathlib
import warnings
from collections.abc import Mapping
Expand All @@ -45,6 +46,7 @@
Object,
ObjectEvents,
)
from .jujuversion import JujuVersion

if TYPE_CHECKING:
from typing_extensions import Required
Expand Down Expand Up @@ -2209,7 +2211,9 @@ def __init__(self, name: str, raw: dict[str, Any] | None = None):
self.description = raw.get('description', '')
self.parameters = raw.get('params', {}) # {<parameter name>: <JSON Schema definition>}
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'))
Copy link
Contributor

@james-garner-canonical james-garner-canonical Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@tonyandrewmeyer tonyandrewmeyer Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_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.

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.

Copy link
Contributor

@james-garner-canonical james-garner-canonical Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_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.

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.

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.

Copy link
Contributor

@dwilding dwilding Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

self.additional_properties = raw.get('additionalProperties', juju_version < '4.0.0')


@dataclasses.dataclass(frozen=True)
Expand Down
27 changes: 24 additions & 3 deletions test/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,11 +627,17 @@ def test_config_from_raw():
assert meta.config['sssh'].description == 'a user secret'


def test_actions_from_charm_root():
@pytest.mark.parametrize('additional_properties', [False, True])
def test_actions_from_charm_root(additional_properties: bool):
with tempfile.TemporaryDirectory() as d:
td = pathlib.Path(d)
(td / 'actions.yaml').write_text(
yaml.safe_dump({'foo': {'description': 'foos the bar', 'additionalProperties': False}})
yaml.safe_dump({
'foo': {
'description': 'foos the bar',
'additionalProperties': additional_properties,
}
})
)
(td / 'metadata.yaml').write_text(
yaml.safe_dump({'name': 'bob', 'requires': {'foo': {'interface': 'bar'}}})
Expand All @@ -640,10 +646,25 @@ def test_actions_from_charm_root():
meta = ops.CharmMeta.from_charm_root(td)
assert meta.name == 'bob'
assert meta.requires['foo'].interface_name == 'bar'
assert not meta.actions['foo'].additional_properties
assert meta.actions['foo'].additional_properties == additional_properties
assert meta.actions['foo'].description == 'foos the bar'


@pytest.mark.parametrize(
'juju_version,additional_properties',
[('2.9', True), ('3.6.12', True), ('4.0.0', False), ('4.1', False), (None, True)],
)
def test_actions_additional_properties(
monkeypatch: pytest.MonkeyPatch, juju_version: str | None, additional_properties: bool
):
if juju_version is None:
monkeypatch.delenv('JUJU_VERSION', raising=False)
else:
monkeypatch.setenv('JUJU_VERSION', juju_version)
action = ops.ActionMeta('foo', {})
assert action.additional_properties == additional_properties


def _setup_test_action(fake_script: FakeScript):
fake_script.write('action-get', """echo '{"foo-name": "name", "silent": true}'""")
fake_script.write('action-set', '')
Expand Down