-
Notifications
You must be signed in to change notification settings - Fork 125
feat: support for generating charmcraft YAML from Python classes #1975
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
4ee7643
da8d265
7f143cd
739c20c
2c9bb9f
a1d4f74
fac6c7c
f155bf5
ea10a4d
bd2a2df
eb2d6e1
35b643a
441c68a
0966028
29875b1
f13ed9d
4320d97
84ba899
b13c742
30573f9
172c692
3ee1807
5f8db50
d932a97
80dfa63
197431a
d0b7e2e
1c09153
b716878
97cd70a
b3c4941
3949ca7
0b4a84d
7f203a1
734b292
5b865f2
ea24f94
dc61cc9
bf3b0bd
703938c
9d614ea
b929496
832a6d1
4294b29
f4ebc54
6248c2c
86a85d9
d1f1113
ead796e
dac0582
48352d6
b8986d6
8659adb
8263595
d228ac9
76dd056
9c72564
36c3b64
28b7a47
92880d4
951d91c
bb9e760
3a94b7d
db5e7c8
8db8ee8
eaa7476
b901a0a
335c01c
71d14d6
a5b5dc5
b2efed5
b485a50
47e425b
f888063
36aa0e0
9044d61
89cac58
851956c
181ce73
3d26c4b
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 |
|---|---|---|
|
|
@@ -10,5 +10,5 @@ pebble | |
| ops-testing | ||
| ops-testing-harness | ||
| ops-tracing | ||
| ops-tools | ||
| ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .. _ops_tools: | ||
|
|
||
| `ops_tools` | ||
| =========== | ||
|
|
||
| .. automodule:: ops_tools | ||
| :exclude-members: main |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1975,6 +1975,10 @@ def autoload(charm_type: type[CharmBase]) -> _CharmSpec[CharmType]: | |
| # try to load using legacy metadata.yaml/actions.yaml/config.yaml files | ||
| meta, config, actions = _CharmSpec._load_metadata_legacy(charm_root) | ||
|
|
||
| # TODO: ideally, we would look for ConfigBase classes in the charm | ||
| # module and autoload from there at this point. Leaving this until the | ||
| # conversation about if & how the generation is done is resolved. | ||
|
|
||
|
Comment on lines
+1978
to
+1981
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. My feeling at the moment is that this should be left for future work, rather than baking anything in to Scenario at this time. There is a simple example of using this functionality in tests for both config and actions in the Scenario tests - I think those show that it's not too arduous to use at the moment, without anything else.
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 not getting the point of this comment. Could future work be tracked in GitHub issues or Jira tickets or roadmap items instead? |
||
| if not meta: | ||
| # still no metadata? bug out | ||
| raise MetadataNotFoundError( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,24 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import dataclasses | ||
|
|
||
| import pytest | ||
| from ops.charm import CharmBase | ||
| from ops.framework import Framework | ||
|
|
||
| import ops | ||
| import ops_tools | ||
|
|
||
| from scenario.context import Context | ||
| from scenario.state import State | ||
| from ..helpers import trigger | ||
|
|
||
|
|
||
| @pytest.fixture(scope='function') | ||
| def mycharm(): | ||
| class MyCharm(CharmBase): | ||
| def __init__(self, framework: Framework): | ||
| class MyCharm(ops.CharmBase): | ||
| def __init__(self, framework: ops.Framework): | ||
| super().__init__(framework) | ||
| for evt in self.on.events().values(): | ||
| self.framework.observe(evt, self._on_event) | ||
| framework.observe(evt, self._on_event) | ||
|
|
||
| def _on_event(self, event): | ||
| pass | ||
|
|
@@ -23,7 +27,7 @@ def _on_event(self, event): | |
|
|
||
|
|
||
| def test_config_get(mycharm): | ||
| def check_cfg(charm: CharmBase): | ||
| def check_cfg(charm: ops.CharmBase): | ||
| assert charm.config['foo'] == 'bar' | ||
| assert charm.config['baz'] == 1 | ||
|
|
||
|
|
@@ -40,7 +44,7 @@ def check_cfg(charm: CharmBase): | |
|
|
||
|
|
||
| def test_config_get_default_from_meta(mycharm): | ||
| def check_cfg(charm: CharmBase): | ||
| def check_cfg(charm: ops.CharmBase): | ||
| assert charm.config['foo'] == 'bar' | ||
| assert charm.config['baz'] == 2 | ||
| assert charm.config['qux'] is False | ||
|
|
@@ -72,11 +76,11 @@ def check_cfg(charm: CharmBase): | |
| ), | ||
| ) | ||
| def test_config_in_not_mutated(mycharm, cfg_in): | ||
| class MyCharm(CharmBase): | ||
| def __init__(self, framework: Framework): | ||
| class MyCharm(ops.CharmBase): | ||
| def __init__(self, framework: ops.Framework): | ||
| super().__init__(framework) | ||
| for evt in self.on.events().values(): | ||
| self.framework.observe(evt, self._on_event) | ||
| framework.observe(evt, self._on_event) | ||
|
|
||
| def _on_event(self, event): | ||
| # access the config to trigger a config-get | ||
|
|
@@ -101,3 +105,27 @@ def _on_event(self, event): | |
| ) | ||
| # check config was not mutated by scenario | ||
| assert state_out.config == cfg_in | ||
|
|
||
|
|
||
| def test_config_using_generated_config(): | ||
| @dataclasses.dataclass | ||
| class Config: | ||
| a: int | ||
| b: float | ||
| c: str | ||
|
|
||
| class Charm(ops.CharmBase): | ||
| def __init__(self, framework: ops.Framework): | ||
| super().__init__(framework) | ||
| framework.observe(self.on.config_changed, self._on_config_changed) | ||
|
|
||
| def _on_config_changed(self, event: ops.ConfigChangedEvent): | ||
| self.typed_config = self.load_config(Config, 10, c='foo') | ||
|
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. Question this is unexpected to me. I would have thought that default are specified in a model, not in the load config call site. The defaults can be included in both dataclasses and pydantic models, so why add the extra arguments feature in the P.S. maybe I was lazy reading the spec, but somehow I don't recall this API 🙈 P.P.S. I guess it's because Juju action parameters don't have defaults like Juju config does, isn't it? That also means that we should be testing both |
||
|
|
||
| schema = ops_tools.config_to_juju_schema(Config) | ||
| ctx = Context(Charm, meta={'name': 'foo'}, config=schema) | ||
| with ctx(ctx.on.config_changed(), State(config={'b': 3.14})) as mgr: | ||
| mgr.run() | ||
| assert mgr.charm.typed_config.a == 10 | ||
| assert mgr.charm.typed_config.b == 3.14 | ||
| assert mgr.charm.typed_config.c == 'foo' | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit could be perhaps written as "test expected to fail if uncommented".