From 4a94f2928e0cb4b0140f031bcff7d3dc3a62a045 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 3 Oct 2023 15:08:01 +0200 Subject: [PATCH 1/4] removed remote unit ids var from Relation --- README.md | 3 +- scenario/mocking.py | 4 +- scenario/state.py | 80 ++++-------------------------------- tests/test_e2e/test_state.py | 5 +-- 4 files changed, 13 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index 427e56da..97f0c5eb 100644 --- a/README.md +++ b/README.md @@ -370,7 +370,6 @@ To declare a peer relation, you should use `scenario.state.PeerRelation`. The co that peer relations do not have a "remote app" (it's this app, in fact). So unlike `Relation`, a `PeerRelation` does not have `remote_app_name` or `remote_app_data` arguments. Also, it talks in terms of `peers`: -- `Relation.remote_unit_ids` maps to `PeerRelation.peers_ids` - `Relation.remote_units_data` maps to `PeerRelation.peers_data` ```python @@ -488,7 +487,7 @@ remote unit that the event is about. The reason that this parameter is not suppl events, is that the relation already ties 'this app' to some 'remote app' (cfr. the `Relation.remote_app_name` attr), but not to a specific unit. What remote unit this event is about is not a `State` concern but an `Event` one. -The `remote_unit_id` will default to the first ID found in the relation's `remote_unit_ids`, but if the test you are +The `remote_unit_id` will default to the first ID found in the relation's `remote_units_data`, but if the test you are writing is close to that domain, you should probably override it and pass it manually. ```python diff --git a/scenario/mocking.py b/scenario/mocking.py index 768709ca..16ae0893 100644 --- a/scenario/mocking.py +++ b/scenario/mocking.py @@ -177,7 +177,9 @@ def relation_list(self, relation_id: int) -> Tuple[str]: relation = self._get_relation_by_id(relation_id) if isinstance(relation, PeerRelation): - return tuple(f"{self.app_name}/{unit_id}" for unit_id in relation.peers_ids) + return tuple( + f"{self.app_name}/{unit_id}" for unit_id in relation.peers_data + ) return tuple( f"{relation.remote_app_name}/{unit_id}" for unit_id in relation._remote_unit_ids diff --git a/scenario/state.py b/scenario/state.py index 8c97fabb..315160e4 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -318,74 +318,18 @@ def broken_event(self) -> "Event": ) -def unify_ids_and_remote_units_data(ids: List[int], data: Dict[int, Any]): - """Unify and validate a list of unit IDs and a mapping from said ids to databag contents. - - This allows the user to pass equivalently: - ids = [] - data = {1: {}} - - or - - ids = [1] - data = {} - - or - - ids = [1] - data = {1: {}} - - but catch the inconsistent: - - ids = [1] - data = {2: {}} - - or - - ids = [2] - data = {1: {}} - """ - if ids and data: - if not set(ids) == set(data): - raise StateValidationError( - f"{ids} should include any and all IDs from {data}", - ) - elif ids: - data = {x: {} for x in ids} - elif data: - ids = list(data) - else: - ids = [0] - data = {0: {}} - return ids, data - - @dataclasses.dataclass(frozen=True) class Relation(RelationBase): remote_app_name: str = "remote" - # fixme: simplify API by deriving remote_unit_ids from remote_units_data. - remote_unit_ids: List[int] = dataclasses.field(default_factory=list) - # local limit limit: int = 1 remote_app_data: Dict[str, str] = dataclasses.field(default_factory=dict) remote_units_data: Dict[int, Dict[str, str]] = dataclasses.field( - default_factory=dict, + default_factory=lambda: {0: {}}, ) - def __post_init__(self): - super().__post_init__() - - remote_unit_ids, remote_units_data = unify_ids_and_remote_units_data( - self.remote_unit_ids, - self.remote_units_data, - ) - # bypass frozen dataclass - object.__setattr__(self, "remote_unit_ids", remote_unit_ids) - object.__setattr__(self, "remote_units_data", remote_units_data) - @property def _remote_app_name(self) -> str: """Who is on the other end of this relation?""" @@ -394,7 +338,7 @@ def _remote_app_name(self) -> str: @property def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" - return tuple(self.remote_unit_ids) + return tuple(self.remote_units_data) def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: """Return the databag for some remote unit ID.""" @@ -447,10 +391,11 @@ def remote_unit_name(self) -> str: @dataclasses.dataclass(frozen=True) class PeerRelation(RelationBase): - peers_data: Dict[int, Dict[str, str]] = dataclasses.field(default_factory=dict) - - # IDs of the peers. Consistency checks will validate that *this unit*'s ID is not in here. - peers_ids: List[int] = dataclasses.field(default_factory=list) + peers_data: Dict[int, Dict[str, str]] = dataclasses.field( + default_factory=lambda: {0: {}}, + ) + # mapping from peer unit IDs to their databag contents. + # Consistency checks will validate that *this unit*'s ID is not in here. @property def _databags(self): @@ -462,21 +407,12 @@ def _databags(self): @property def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" - return tuple(self.peers_ids) + return tuple(self.peers_data) def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: """Return the databag for some remote unit ID.""" return self.peers_data[unit_id] - def __post_init__(self): - peers_ids, peers_data = unify_ids_and_remote_units_data( - self.peers_ids, - self.peers_data, - ) - # bypass frozen dataclass guards - object.__setattr__(self, "peers_ids", peers_ids) - object.__setattr__(self, "peers_data", peers_data) - def _random_model_name(): import random diff --git a/tests/test_e2e/test_state.py b/tests/test_e2e/test_state.py index f080eafe..f06f7007 100644 --- a/tests/test_e2e/test_state.py +++ b/tests/test_e2e/test_state.py @@ -147,7 +147,6 @@ def pre_event(charm: CharmBase): interface="bar", local_app_data={"a": "because"}, remote_app_name="remote", - remote_unit_ids=[0, 1, 2], remote_app_data={"a": "b"}, local_unit_data={"c": "d"}, remote_units_data={0: {}, 1: {"e": "f"}, 2: {}}, @@ -198,9 +197,7 @@ def pre_event(charm: CharmBase): endpoint="foo", interface="bar", remote_app_name="remote", - remote_unit_ids=[1, 4], - local_app_data={}, - local_unit_data={}, + remote_units_data={1: {}, 4: {}}, ) state = State( leader=True, From 8b2daa12812d1fe30c00150637d9c06ea2950ecc Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 3 Oct 2023 15:38:38 +0200 Subject: [PATCH 2/4] vbump --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 398f63b2..6b647aeb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,7 @@ build-backend = "setuptools.build_meta" [project] name = "ops-scenario" -version = "5.2.2" +version = "5.3" authors = [ { name = "Pietro Pasotti", email = "pietro.pasotti@canonical.com" } From 84effd6b109482d830522e7a914bfe4ace6dfa1d Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 3 Oct 2023 16:19:08 +0200 Subject: [PATCH 3/4] coverage 88%, shed some dead code --- .gitignore | 1 + scenario/__init__.py | 2 - scenario/consistency_checker.py | 1 + scenario/state.py | 21 ----------- tests/test_consistency_checker.py | 62 ++++++++++++++++++++++++++++++- tests/test_context.py | 11 ++++++ tests/test_e2e/test_actions.py | 17 +++++++++ tests/test_e2e/test_secrets.py | 40 ++++++++++++++++++++ tox.ini | 2 +- 9 files changed, 132 insertions(+), 25 deletions(-) diff --git a/.gitignore b/.gitignore index 1be459dd..046b9675 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ __pycache__/ *.egg-info dist/ *.pytest_cache +htmlcov/ \ No newline at end of file diff --git a/scenario/__init__.py b/scenario/__init__.py index 8fa5398c..82b89ad6 100644 --- a/scenario/__init__.py +++ b/scenario/__init__.py @@ -14,7 +14,6 @@ Model, Mount, Network, - ParametrizedEvent, PeerRelation, Port, Relation, @@ -34,7 +33,6 @@ "deferred", "StateValidationError", "Secret", - "ParametrizedEvent", "RelationBase", "Relation", "SubordinateRelation", diff --git a/scenario/consistency_checker.py b/scenario/consistency_checker.py index daa992e5..52d62649 100644 --- a/scenario/consistency_checker.py +++ b/scenario/consistency_checker.py @@ -185,6 +185,7 @@ def _check_action_event( f"action event {event.name} refers to action {action.name} " f"which is not declared in the charm metadata (actions.yaml).", ) + return _check_action_param_types(charm_spec, action, errors, warnings) diff --git a/scenario/state.py b/scenario/state.py index 315160e4..19d60540 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -195,27 +195,6 @@ def normalize_name(s: str): return s.replace("-", "_") -class ParametrizedEvent: - def __init__(self, accept_params: Tuple[str], category: str, *args, **kwargs): - self._accept_params = accept_params - self._category = category - self._args = args - self._kwargs = kwargs - - def __call__(self, remote_unit: Optional[str] = None) -> "Event": - """Construct an Event object using the arguments provided at init and any extra params.""" - if remote_unit and "remote_unit" not in self._accept_params: - raise ValueError( - f"cannot pass param `remote_unit` to a " - f"{self._category} event constructor.", - ) - - return Event(*self._args, *self._kwargs, relation_remote_unit_id=remote_unit) - - def deferred(self, handler: Callable, event_id: int = 1) -> "DeferredEvent": - return self().deferred(handler=handler, event_id=event_id) - - _next_relation_id_counter = 1 diff --git a/tests/test_consistency_checker.py b/tests/test_consistency_checker.py index a1bf26b4..e26fb3ad 100644 --- a/tests/test_consistency_checker.py +++ b/tests/test_consistency_checker.py @@ -142,6 +142,11 @@ def test_bad_config_option_type(): Event("bar"), _CharmSpec(MyCharm, {}, config={"options": {"foo": {"type": "string"}}}), ) + assert_inconsistent( + State(config={"foo": True}), + Event("bar"), + _CharmSpec(MyCharm, {}, config={"options": {"foo": {}}}), + ) assert_consistent( State(config={"foo": True}), Event("bar"), @@ -151,12 +156,26 @@ def test_bad_config_option_type(): @pytest.mark.parametrize("bad_v", ("1.0", "0", "1.2", "2.35.42", "2.99.99", "2.99")) def test_secrets_jujuv_bad(bad_v): + secret = Secret("secret:foo", {0: {"a": "b"}}) assert_inconsistent( - State(secrets=[Secret("secret:foo", {0: {"a": "b"}})]), + State(secrets=[secret]), Event("bar"), _CharmSpec(MyCharm, {}), bad_v, ) + assert_inconsistent( + State(secrets=[secret]), + secret.changed_event, + _CharmSpec(MyCharm, {}), + bad_v, + ) + + assert_inconsistent( + State(), + secret.changed_event, + _CharmSpec(MyCharm, {}), + bad_v, + ) @pytest.mark.parametrize("good_v", ("3.0", "3.1", "3", "3.33", "4", "100")) @@ -182,6 +201,20 @@ def test_peer_relation_consistency(): ) +def test_duplicate_endpoints_inconsistent(): + assert_inconsistent( + State(), + Event("bar"), + _CharmSpec( + MyCharm, + { + "requires": {"foo": {"interface": "bar"}}, + "provides": {"foo": {"interface": "baz"}}, + }, + ), + ) + + def test_sub_relation_consistency(): assert_inconsistent( State(relations=[Relation("foo")]), @@ -191,6 +224,7 @@ def test_sub_relation_consistency(): {"requires": {"foo": {"interface": "bar", "scope": "container"}}}, ), ) + assert_consistent( State(relations=[SubordinateRelation("foo")]), Event("bar"), @@ -226,6 +260,32 @@ def test_container_pebble_evt_consistent(): ) +def test_action_not_in_meta_inconsistent(): + action = Action("foo", params={"bar": "baz"}) + assert_inconsistent( + State(), + action.event, + _CharmSpec(MyCharm, meta={}, actions={}), + ) + + +def test_action_meta_type_inconsistent(): + action = Action("foo", params={"bar": "baz"}) + assert_inconsistent( + State(), + action.event, + _CharmSpec( + MyCharm, meta={}, actions={"foo": {"params": {"bar": {"type": "zabazaba"}}}} + ), + ) + + assert_inconsistent( + State(), + action.event, + _CharmSpec(MyCharm, meta={}, actions={"foo": {"params": {"bar": {}}}}), + ) + + def test_action_name(): action = Action("foo", params={"bar": "baz"}) diff --git a/tests/test_context.py b/tests/test_context.py index 1a1d3e19..76a24eec 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -39,3 +39,14 @@ def test_run_action(): assert isinstance(a, Action) assert a.event.name == "do_foo_action" assert s is state + + +def test_clear(): + ctx = Context(MyCharm, meta={"name": "foo"}) + state = State() + + ctx.run("start", state) + assert ctx.emitted_events + + ctx.clear() + assert not ctx.emitted_events # and others... diff --git a/tests/test_e2e/test_actions.py b/tests/test_e2e/test_actions.py index 3e203bf3..eba28480 100644 --- a/tests/test_e2e/test_actions.py +++ b/tests/test_e2e/test_actions.py @@ -43,6 +43,23 @@ def test_action_event(mycharm, baz_value): assert evt.params["baz"] is baz_value +def test_action_pre_post(mycharm): + ctx = Context( + mycharm, + meta={"name": "foo"}, + actions={ + "foo": {"params": {"bar": {"type": "number"}, "baz": {"type": "boolean"}}} + }, + ) + action = Action("foo", params={"baz": True, "bar": 10}) + ctx.run_action( + action, + State(), + pre_event=lambda charm: None, + post_event=lambda charm: None, + ) + + @pytest.mark.parametrize("res_value", ("one", 1, [2], ["bar"], (1,), {1, 2})) def test_action_event_results_invalid(mycharm, res_value): def handle_evt(charm: CharmBase, evt: ActionEvent): diff --git a/tests/test_e2e/test_secrets.py b/tests/test_e2e/test_secrets.py index 0f074311..2b08cae2 100644 --- a/tests/test_e2e/test_secrets.py +++ b/tests/test_e2e/test_secrets.py @@ -1,3 +1,5 @@ +import datetime + import pytest from ops.charm import CharmBase from ops.framework import Framework @@ -210,6 +212,44 @@ def post_event(charm: CharmBase): assert vals == [{"remote"}] if app else [{"remote/0"}] +def test_update_metadata(mycharm): + exp = datetime.datetime(2050, 12, 12) + + def post_event(charm: CharmBase): + secret = charm.model.get_secret(label="mylabel") + secret.set_info( + label="babbuccia", + description="blu", + expire=exp, + rotate=SecretRotate.DAILY, + ) + + out = trigger( + State( + secrets=[ + Secret( + owner="unit", + id="foo", + label="mylabel", + contents={ + 0: {"a": "b"}, + }, + ) + ], + ), + "update_status", + mycharm, + meta={"name": "local"}, + post_event=post_event, + ) + + secret_out = out.secrets[0] + assert secret_out.label == "babbuccia" + assert secret_out.rotate == SecretRotate.DAILY + assert secret_out.description == "blu" + assert secret_out.expire == exp + + def test_grant_nonowner(mycharm): def post_event(charm: CharmBase): secret = charm.model.get_secret(id="foo") diff --git a/tox.ini b/tox.ini index 8d5b23e2..23a1d22f 100644 --- a/tox.ini +++ b/tox.ini @@ -29,7 +29,7 @@ commands = coverage run \ --source={[vars]src_path} \ -m pytest -v --tb native --log-cli-level=INFO -s {posargs} {[vars]tst_path} - coverage report + coverage html [testenv:lint] description = Format the code base to adhere to our styles, and complain about what we cannot do automatically. From 6dc53f60319a12ef167db2d7df579c2198d354b8 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 3 Oct 2023 16:23:01 +0200 Subject: [PATCH 4/4] explicit types for raw databag contents --- scenario/state.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/scenario/state.py b/scenario/state.py index 19d60540..9b612e20 100644 --- a/scenario/state.py +++ b/scenario/state.py @@ -35,6 +35,8 @@ PathLike = Union[str, Path] AnyRelation = Union["Relation", "PeerRelation", "SubordinateRelation"] AnyJson = Union[str, bool, dict, int, float, list] + RawSecretRevisionContents = RawDataBagContents = Dict[str, str] + UnitID = int logger = scenario_logger.getChild("state") @@ -103,7 +105,7 @@ class Secret(_DCBase): id: str # mapping from revision IDs to each revision's contents - contents: Dict[int, Dict[str, str]] + contents: Dict[int, "RawSecretRevisionContents"] # indicates if the secret is owned by THIS unit, THIS app or some other app/unit. owner: Literal["unit", "application", None] = None @@ -168,7 +170,7 @@ def _set_revision(self, revision: int): def _update_metadata( self, - content: Optional[Dict[str, str]] = None, + content: Optional["RawSecretRevisionContents"] = None, label: Optional[str] = None, description: Optional[str] = None, expire: Optional[datetime.datetime] = None, @@ -216,8 +218,8 @@ class RelationBase(_DCBase): # Every new Relation instance gets a new one, if there's trouble, override. relation_id: int = dataclasses.field(default_factory=next_relation_id) - local_app_data: Dict[str, str] = dataclasses.field(default_factory=dict) - local_unit_data: Dict[str, str] = dataclasses.field(default_factory=dict) + local_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) + local_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) @property def _databags(self): @@ -230,7 +232,10 @@ def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" raise NotImplementedError() - def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: # noqa: U100 + def _get_databag_for_remote( + self, + unit_id: int, # noqa: U100 + ) -> "RawDataBagContents": """Return the databag for some remote unit ID.""" raise NotImplementedError() @@ -304,8 +309,8 @@ class Relation(RelationBase): # local limit limit: int = 1 - remote_app_data: Dict[str, str] = dataclasses.field(default_factory=dict) - remote_units_data: Dict[int, Dict[str, str]] = dataclasses.field( + remote_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) + remote_units_data: Dict["UnitID", "RawDataBagContents"] = dataclasses.field( default_factory=lambda: {0: {}}, ) @@ -319,7 +324,7 @@ def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" return tuple(self.remote_units_data) - def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: + def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents": """Return the databag for some remote unit ID.""" return self.remote_units_data[unit_id] @@ -334,8 +339,8 @@ def _databags(self): @dataclasses.dataclass(frozen=True) class SubordinateRelation(RelationBase): - remote_app_data: Dict[str, str] = dataclasses.field(default_factory=dict) - remote_unit_data: Dict[str, str] = dataclasses.field(default_factory=dict) + remote_app_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) + remote_unit_data: "RawDataBagContents" = dataclasses.field(default_factory=dict) # app name and ID of the remote unit that *this unit* is attached to. remote_app_name: str = "remote" @@ -346,7 +351,7 @@ def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" return (self.remote_unit_id,) - def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: + def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents": """Return the databag for some remote unit ID.""" if unit_id is not self.remote_unit_id: raise ValueError( @@ -370,7 +375,7 @@ def remote_unit_name(self) -> str: @dataclasses.dataclass(frozen=True) class PeerRelation(RelationBase): - peers_data: Dict[int, Dict[str, str]] = dataclasses.field( + peers_data: Dict["UnitID", "RawDataBagContents"] = dataclasses.field( default_factory=lambda: {0: {}}, ) # mapping from peer unit IDs to their databag contents. @@ -388,7 +393,7 @@ def _remote_unit_ids(self) -> Tuple[int]: """Ids of the units on the other end of this relation.""" return tuple(self.peers_data) - def _get_databag_for_remote(self, unit_id: int) -> Dict[str, str]: + def _get_databag_for_remote(self, unit_id: int) -> "RawDataBagContents": """Return the databag for some remote unit ID.""" return self.peers_data[unit_id]