From f3c28775615319ea64470f4b20c0b8803ec200d2 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 4 Mar 2024 15:37:46 +1300 Subject: [PATCH 1/9] Adjust the imports so that both ruff and isort are ok. (#1143) --- test/charms/test_main/src/charm.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/charms/test_main/src/charm.py b/test/charms/test_main/src/charm.py index ae845c1e6..ccab5d62a 100755 --- a/test/charms/test_main/src/charm.py +++ b/test/charms/test_main/src/charm.py @@ -18,9 +18,10 @@ import sys import typing +import ops + sys.path.append('lib') -import ops logger = logging.getLogger() From d0cae7d847e10f9d0acc88703594533fe5d08fb1 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 8 Mar 2024 17:22:00 +1300 Subject: [PATCH 2/9] docs: use 'integrate with' rather than 'relate to' (#1145) --- ops/charm.py | 17 +++++++++-------- ops/model.py | 16 ++++++++-------- ops/testing.py | 6 +++--- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index db02e13d9..59ad4c5e7 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -507,9 +507,10 @@ def restore(self, snapshot: Dict[str, Any]): class RelationCreatedEvent(RelationEvent): """Event triggered when a new relation is created. - This is triggered when a new relation to another app is added in Juju. This + This is triggered when a new integration with another app is added in Juju. This can occur before units for those applications have started. All existing - relations should be established before start. + relations will trigger `RelationCreatedEvent` before :class:`StartEvent` is + emitted. """ unit: None # pyright: ignore[reportIncompatibleVariableOverride] """Always ``None``.""" @@ -521,7 +522,7 @@ class RelationJoinedEvent(RelationEvent): This event is triggered whenever a new unit of a related application joins the relation. The event fires only when that remote unit is first observed by the unit. Callback methods bound - to this event may set any local unit settings that can be + to this event may set any local unit data that can be determined using no more than the name of the joining unit and the remote ``private-address`` setting, which is always available when the relation is created and is by convention not deleted. @@ -539,13 +540,13 @@ class RelationChangedEvent(RelationEvent): the callback method bound to this event. This event always fires once, after :class:`RelationJoinedEvent`, and - will subsequently fire whenever that remote unit changes its settings for + will subsequently fire whenever that remote unit changes its data for the relation. Callback methods bound to this event should be the only ones - that rely on remote relation settings. They should not error if the settings - are incomplete, since it can be guaranteed that when the remote unit or - application changes its settings, the event will fire again. + that rely on remote relation data. They should not error if the data + is incomplete, since it can be guaranteed that when the remote unit or + application changes its data, the event will fire again. - The settings that may be queried, or set, are determined by the relation's + The data that may be queried, or set, are determined by the relation's interface. """ diff --git a/ops/model.py b/ops/model.py index cf1f065d2..92eac06d3 100644 --- a/ops/model.py +++ b/ops/model.py @@ -147,7 +147,7 @@ def app(self) -> 'Application': def relations(self) -> 'RelationMapping': """Mapping of endpoint to list of :class:`Relation`. - Answers the question "what am I currently related to". + Answers the question "what am I currently integrated with". See also :meth:`.get_relation`. In a ``relation-broken`` event, the broken relation is excluded from @@ -236,7 +236,7 @@ def get_relation( given application has more than one relation on a given endpoint. Raises: - TooManyRelatedAppsError: is raised if there is more than one relation to the + TooManyRelatedAppsError: is raised if there is more than one integration with the supplied relation_name and no relation_id was supplied """ return self.relations._get_unique(relation_name, relation_id) @@ -315,8 +315,8 @@ def get(self, entity_type: 'UnitOrApplicationType', name: str): class Application: """Represents a named application in the model. - This might be this charm's application, or might be an application this charm is related - to. Charmers should not instantiate Application objects directly, but should use + This might be this charm's application, or might be an application this charm is integrated + with. Charmers should not instantiate Application objects directly, but should use :attr:`Model.app` to get the application this unit is part of, or :meth:`Model.get_app` if they need a reference to a given application. """ @@ -472,7 +472,7 @@ class Unit: """Represents a named unit in the model. This might be the current unit, another unit of the charm's application, or a unit of - another application that the charm is related to. + another application that the charm is integrated with. """ name: str @@ -1865,8 +1865,8 @@ class MaintenanceStatus(StatusBase): class WaitingStatus(StatusBase): """A unit is unable to progress. - The unit is unable to progress to an active state because an application to which - it is related is not running. + The unit is unable to progress to an active state because an application with which + it is integrated is not running. """ name = 'waiting' @@ -2856,7 +2856,7 @@ class ModelError(Exception): class TooManyRelatedAppsError(ModelError): - """Raised by :meth:`Model.get_relation` if there is more than one related application.""" + """Raised by :meth:`Model.get_relation` if there is more than one integrated application.""" def __init__(self, relation_name: str, num_related: int, max_supported: int): super().__init__('Too many remote applications on {} ({} > {})'.format( diff --git a/ops/testing.py b/ops/testing.py index 5014ef516..3bb040ff2 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -819,8 +819,8 @@ def add_relation(self, relation_name: str, remote_app: str, *, }) Args: - relation_name: The relation on the charm that is being related to. - remote_app: The name of the application that is being related to. + relation_name: The relation on the charm that is being integrated with. + remote_app: The name of the application that is being integrated with. To add a peer relation, set to the name of *this* application. app_data: If provided, also add a new unit to the relation (triggering relation-joined) and set the *application* relation data @@ -1339,7 +1339,7 @@ def set_leader(self, is_leader: bool = True) -> None: If this charm becomes a leader then `leader_elected` will be triggered. If :meth:`begin` has already been called, then the charm's peer relation should usually be added *prior* to - calling this method (with :meth:`add_relation`) to properly initialize and make + calling this method (with :meth:`add_relation`) to properly initialise and make available relation data that leader elected hooks may want to access. Args: From bc0bf8feb1b4795f6ceb8d6d550e48c45607a5fa Mon Sep 17 00:00:00 2001 From: PietroPasotti Date: Sun, 10 Mar 2024 21:17:49 +0100 Subject: [PATCH 3/9] fix(framework): add warning on lost observer weakref (#1142) --- ops/framework.py | 7 +++++++ test/test_charm.py | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/ops/framework.py b/ops/framework.py index cc84436ac..645abdce7 100644 --- a/ops/framework.py +++ b/ops/framework.py @@ -954,6 +954,13 @@ def _reemit(self, single_event_path: Optional[str] = None): # Regular call to the registered method. custom_handler(event) + else: + logger.warning( + f"Reference to ops.Object at path {observer_path} has been garbage collected " + "between when the charm was initialised and when the event was emitted. " + "Make sure sure you store a reference to the observer." + ) + if event.deferred: deferred = True else: diff --git a/test/test_charm.py b/test/test_charm.py index 627d2d52c..7ebbce298 100644 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -128,6 +128,30 @@ def _on_start(self, event: ops.EventBase): # check that the event has been seen by the observer self.assertIsInstance(charm.seen, ops.StartEvent) + def test_observer_not_referenced_warning(self): + class MyObj(ops.Object): + def __init__(self, charm: ops.CharmBase): + super().__init__(charm, "obj") + framework.observe(charm.on.start, self._on_start) + + def _on_start(self, _: ops.StartEvent): + raise RuntimeError() # never reached! + + class MyCharm(ops.CharmBase): + def __init__(self, *args: typing.Any): + super().__init__(*args) + MyObj(self) # not assigned! + framework.observe(self.on.start, self._on_start) + + def _on_start(self, _: ops.StartEvent): + pass # is reached + + framework = self.create_framework() + c = MyCharm(framework) + with self.assertLogs() as logs: + c.on.start.emit() + assert any('Reference to ops.Object' in log for log in logs.output) + def test_empty_action(self): meta = ops.CharmMeta.from_yaml('name: my-charm', '') self.assertEqual(meta.actions, {}) From c886aadd865e2241d59df3201633d67fa116a290 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Wed, 13 Mar 2024 04:43:31 +0800 Subject: [PATCH 4/9] fix(pebble)!: change select=all to users=all for pebble get_notices (#1146) Pebble added support for notices, aggregated events that are recorded with a type and key. The `/v1/notices` API had a parameter "select=all" to show notices for all users, and the parameter name is changed to "users=all" in recent [release v1.9.0](https://github.com/canonical/pebble/releases/tag/v1.9.0). This PR is the corresponding changes to the ops framework. Related doc: [OP042 - User ID features for Pebble Notices ](https://docs.google.com/document/d/1tQwUxz-rV-NjH-UodDbSDhGcMJGfD3OSoTnBLI9aoGU/edit#heading=h.kavgppauvkj5) See here: https://github.com/canonical/operator/issues/1132 --- CHANGES.md | 4 ++++ ops/model.py | 4 ++-- ops/pebble.py | 14 +++++++------- ops/testing.py | 6 +++--- test/test_model.py | 4 ++-- test/test_pebble.py | 4 ++-- 6 files changed, 20 insertions(+), 16 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 6fd7d6c78..e2d5768cb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,7 @@ +# 2.12.0 + +* Updated Pebble Notices `get_notices` parameter name to `users=all` (previously `select=all`). + # 2.11.0 * `StopEvent`, `RemoveEvent`, and all `LifeCycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object. diff --git a/ops/model.py b/ops/model.py index 92eac06d3..c2a42b291 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2757,7 +2757,7 @@ def get_notice(self, id: str) -> pebble.Notice: def get_notices( self, *, - select: Optional[pebble.NoticesSelect] = None, + users: Optional[pebble.NoticesUsers] = None, user_id: Optional[int] = None, types: Optional[Iterable[Union[pebble.NoticeType, str]]] = None, keys: Optional[Iterable[str]] = None, @@ -2768,7 +2768,7 @@ def get_notices( parameters. """ return self._pebble.get_notices( - select=select, + users=users, user_id=user_id, types=types, keys=keys, diff --git a/ops/pebble.py b/ops/pebble.py index 0b96d596a..60bb9d7b5 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -1316,11 +1316,11 @@ class NoticeType(enum.Enum): CUSTOM = 'custom' -class NoticesSelect(enum.Enum): - """Enum of :meth:`Client.get_notices` ``select`` values.""" +class NoticesUsers(enum.Enum): + """Enum of :meth:`Client.get_notices` ``users`` values.""" ALL = 'all' - """Select notices from all users (any user ID, including public notices). + """Return notices from all users (any user ID, including public notices). This only works for Pebble admins (for example, root). """ @@ -2803,7 +2803,7 @@ def get_notice(self, id: str) -> Notice: def get_notices( self, *, - select: Optional[NoticesSelect] = None, + users: Optional[NoticesUsers] = None, user_id: Optional[int] = None, types: Optional[Iterable[Union[NoticeType, str]]] = None, keys: Optional[Iterable[str]] = None, @@ -2824,7 +2824,7 @@ def get_notices( type has nanosecond precision). Args: - select: Select which notices to return (instead of returning + users: Change which users' notices to return (instead of returning notices for the current user). user_id: Filter for notices for the specified user, including public notices (only works for Pebble admins). @@ -2832,8 +2832,8 @@ def get_notices( keys: Filter for notices with any of the specified keys. """ query: Dict[str, Union[str, List[str]]] = {} - if select is not None: - query['select'] = select.value + if users is not None: + query['users'] = users.value if user_id is not None: query['user-id'] = str(user_id) if types is not None: diff --git a/ops/testing.py b/ops/testing.py index 3bb040ff2..d8e821d02 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -3361,7 +3361,7 @@ def get_notice(self, id: str) -> pebble.Notice: def get_notices( self, *, - select: Optional[pebble.NoticesSelect] = None, + users: Optional[pebble.NoticesUsers] = None, user_id: Optional[int] = None, types: Optional[Iterable[Union[pebble.NoticeType, str]]] = None, keys: Optional[Iterable[str]] = None, @@ -3371,9 +3371,9 @@ def get_notices( filter_user_id = 0 # default is to filter by request UID (root) if user_id is not None: filter_user_id = user_id - if select is not None: + if users is not None: if user_id is not None: - raise self._api_error(400, 'cannot use both "select" and "user_id"') + raise self._api_error(400, 'cannot use both "users" and "user_id"') filter_user_id = None if types is not None: diff --git a/test/test_model.py b/test/test_model.py index 588b3e273..9155217d8 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -1977,7 +1977,7 @@ def test_get_notices(self): notices = self.container.get_notices( user_id=1000, - select=pebble.NoticesSelect.ALL, + users=pebble.NoticesUsers.ALL, types=[pebble.NoticeType.CUSTOM], keys=['example.com/a', 'example.com/b'], ) @@ -1988,7 +1988,7 @@ def test_get_notices(self): self.assertEqual(self.pebble.requests, [('get_notices', dict( user_id=1000, - select=pebble.NoticesSelect.ALL, + users=pebble.NoticesUsers.ALL, types=[pebble.NoticeType.CUSTOM], keys=['example.com/a', 'example.com/b'], ))]) diff --git a/test/test_pebble.py b/test/test_pebble.py index 7c7f42990..ccd96d00c 100644 --- a/test/test_pebble.py +++ b/test/test_pebble.py @@ -3032,7 +3032,7 @@ def test_get_notices_filters(self): notices = self.client.get_notices( user_id=1000, - select=pebble.NoticesSelect.ALL, + users=pebble.NoticesUsers.ALL, types=[pebble.NoticeType.CUSTOM], keys=['example.com/a', 'example.com/b'], ) @@ -3042,7 +3042,7 @@ def test_get_notices_filters(self): query = { 'user-id': '1000', - 'select': 'all', + 'users': 'all', 'types': ['custom'], 'keys': ['example.com/a', 'example.com/b'], } From 13a178afccdd7c2167749e113956e54cc7dae614 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 15 Mar 2024 10:04:36 +1300 Subject: [PATCH 5/9] fix: inspect the correct signature when validating observe arguments (#1147) When validating the `observe` arguments, inspect the signature of the method that we'll call, rather than digging down into any wrapped version, so that wrappers can adjust the signature as required, as long as the framework can still call them with the expected arguments. This also fixes a bug where the check would fail to properly identify `*` and `**` as not required, so a signature like `def _on_x(self, event, *args, **kwargs)` would fail, even though the framework can use it to emit events. Also remove the code that checks for `framework.observe(self.on.x, self)` style calls, which were possible in some old version of ops, and raises a specific error just for that (the general error is clear enough, and we don't believe anyone is still doing this). When checking the type of the event handler, raise `TypeError` for errors, rather than a mixture of `RuntimeError` and `TypeError`. Fixes #1129 --- ops/framework.py | 35 +++++--------- test/test_charm.py | 2 +- test/test_framework.py | 104 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 28 deletions(-) diff --git a/ops/framework.py b/ops/framework.py index 645abdce7..07294ed7a 100644 --- a/ops/framework.py +++ b/ops/framework.py @@ -782,18 +782,11 @@ class SomeObject: RuntimeError: if bound_event or observer are the wrong type. """ if not isinstance(bound_event, BoundEvent): - raise RuntimeError( + raise TypeError( f'Framework.observe requires a BoundEvent as second parameter, got {bound_event}') if not isinstance(observer, types.MethodType): - # help users of older versions of the framework - if isinstance(observer, charm.CharmBase): - raise TypeError( - 'observer methods must now be explicitly provided;' - ' please replace observe(self.on.{0}, self)' - ' with e.g. observe(self.on.{0}, self._on_{0})'.format( - bound_event.event_kind)) - raise RuntimeError( - f'Framework.observe requires a method as third parameter, got {observer}') + raise TypeError( + f"Framework.observe requires a method as the 'observer' parameter, got {observer}") event_type = bound_event.event_type event_kind = bound_event.event_kind @@ -804,27 +797,23 @@ class SomeObject: if hasattr(emitter, "handle"): emitter_path = emitter.handle.path else: - raise RuntimeError( + raise TypeError( f'event emitter {type(emitter).__name__} must have a "handle" attribute') - # Validate that the method has an acceptable call signature. - sig = inspect.signature(observer) - # Self isn't included in the params list, so the first arg will be the event. - extra_params = list(sig.parameters.values())[1:] - method_name = observer.__name__ assert isinstance(observer.__self__, Object), "can't register observers " \ "that aren't `Object`s" observer_obj = observer.__self__ - if not sig.parameters: - raise TypeError( - f'{type(observer_obj).__name__}.{method_name} must accept event parameter') - elif any(param.default is inspect.Parameter.empty for param in extra_params): - # Allow for additional optional params, since there's no reason to exclude them, but - # required params will break. + + # Validate that the method has an acceptable call signature. + sig = inspect.signature(observer, follow_wrapped=False) + try: + sig.bind(EventBase(None)) # type: ignore + except TypeError as e: raise TypeError( - f'{type(observer_obj).__name__}.{method_name} has extra required parameter') + f'{type(observer_obj).__name__}.{method_name} must be callable with ' + "only 'self' and the 'event'") from e # TODO Prevent the exact same parameters from being registered more than once. diff --git a/test/test_charm.py b/test/test_charm.py index 7ebbce298..1a9ac52c5 100644 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -89,7 +89,7 @@ def _on_start(self, event: ops.EventBase): self.assertEqual(charm.started, True) - with self.assertRaisesRegex(TypeError, "observer methods must now be explicitly provided"): + with self.assertRaises(TypeError): framework.observe(charm.on.start, charm) # type: ignore def test_observe_decorated_method(self): diff --git a/test/test_framework.py b/test/test_framework.py index 7a2a7783b..bea874520 100644 --- a/test/test_framework.py +++ b/test/test_framework.py @@ -13,6 +13,7 @@ # limitations under the License. import datetime +import functools import gc import inspect import io @@ -166,7 +167,7 @@ def on_foo(self, event: ops.EventBase): framework.observe(pub.foo, obs.on_any) framework.observe(pub.bar, obs.on_any) - with self.assertRaisesRegex(RuntimeError, "^Framework.observe requires a method"): + with self.assertRaisesRegex(TypeError, "^Framework.observe requires a method"): framework.observe(pub.baz, obs) # type: ignore pub.foo.emit() @@ -178,6 +179,61 @@ def on_foo(self, event: ops.EventBase): "", ]) + def test_event_observer_more_args(self): + framework = self.create_framework() + + class MyEvent(ops.EventBase): + pass + + class MyNotifier(ops.Object): + foo = ops.EventSource(MyEvent) + bar = ops.EventSource(MyEvent) + baz = ops.EventSource(MyEvent) + qux = ops.EventSource(MyEvent) + + class MyObserver(ops.Object): + def __init__(self, parent: ops.Object, key: str): + super().__init__(parent, key) + self.seen: typing.List[str] = [] + self.reprs: typing.List[str] = [] + + def on_foo(self, event: ops.EventBase): + self.seen.append(f"on_foo:{event.handle.kind}") + self.reprs.append(repr(event)) + + def on_bar(self, event: ops.EventBase, _: int = 1): + self.seen.append(f"on_bar:{event.handle.kind}") + self.reprs.append(repr(event)) + + def on_baz(self, event: ops.EventBase, *, _: int = 1): + self.seen.append(f"on_baz:{event.handle.kind}") + self.reprs.append(repr(event)) + + def on_qux(self, event: ops.EventBase, *args, **kwargs): # type: ignore + self.seen.append(f"on_qux:{event.handle.kind}") + self.reprs.append(repr(event)) + + pub = MyNotifier(framework, "1") + obs = MyObserver(framework, "1") + + framework.observe(pub.foo, obs.on_foo) + framework.observe(pub.bar, obs.on_bar) + framework.observe(pub.baz, obs.on_baz) + framework.observe(pub.qux, obs.on_qux) # type: ignore + + pub.foo.emit() + pub.bar.emit() + pub.baz.emit() + pub.qux.emit() + + self.assertEqual(obs.seen, ['on_foo:foo', 'on_bar:bar', 'on_baz:baz', 'on_qux:qux']) + self.assertEqual(obs.reprs, [ + "", + "", + "", + "", + ]) + def test_bad_sig_observer(self): class MyEvent(ops.EventBase): @@ -210,11 +266,11 @@ def _on_qux(self, event: ops.EventBase, extra: typing.Optional[typing.Any] = Non pub = MyNotifier(framework, "pub") obs = MyObserver(framework, "obs") - with self.assertRaisesRegex(TypeError, "must accept event parameter"): + with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"): framework.observe(pub.foo, obs._on_foo) # type: ignore - with self.assertRaisesRegex(TypeError, "has extra required parameter"): + with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"): framework.observe(pub.bar, obs._on_bar) # type: ignore - with self.assertRaisesRegex(TypeError, "has extra required parameter"): + with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"): framework.observe(pub.baz, obs._on_baz) # type: ignore framework.observe(pub.qux, obs._on_qux) @@ -886,6 +942,46 @@ def _on_event(self, event: ops.EventBase): 'ObjectWithStorage[obj]/StoredStateData[_stored]', 'ObjectWithStorage[obj]/on/event[1]'])) + def test_wrapped_handler(self): + # It's fine to wrap the event handler, as long as the framework can + # still call it with just the `event` argument. + def add_arg(func: typing.Callable[..., None]) -> typing.Callable[..., None]: + @functools.wraps(func) + def wrapper(charm: ops.CharmBase, event: ops.EventBase): + return func(charm, event, "extra-arg") + + return wrapper + + class MyCharm(ops.CharmBase): + @add_arg + def _on_event(self, _, another_arg: str): + assert another_arg == "extra-arg" + + framework = self.create_framework() + charm = MyCharm(framework) + framework.observe(charm.on.start, charm._on_event) + charm.on.start.emit() + + # If the event handler is wrapped, and then needs an additional argument + # that's the same problem as if we didn't wrap it and required an extra + # argument, so check that also correctly fails. + def bad_arg(func: typing.Callable[..., None]) -> typing.Callable[..., None]: + @functools.wraps(func) + def wrapper(charm: ops.CharmBase, event: ops.EventBase, _: str): + return func(charm, event) + + return wrapper + + class BadCharm(ops.CharmBase): + @bad_arg + def _on_event(self, _: ops.EventBase): + assert False, 'should not get to here' + + framework = self.create_framework() + charm = BadCharm(framework) + with self.assertRaisesRegex(TypeError, "only 'self' and the 'event'"): + framework.observe(charm.on.start, charm._on_event) + MutableTypesTestCase = typing.Tuple[ typing.Callable[[], typing.Any], # Called to get operand A. From 82cadf2f3717dfd01b80b22138344cf199453f3c Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Fri, 15 Mar 2024 09:10:26 +0800 Subject: [PATCH 6/9] fix(model): change model.relation.app type from optional to mandatory (#1151) fix(model): change model.relation.app type from optional to mandatory --- ops/model.py | 18 +++++++++--------- ops/testing.py | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ops/model.py b/ops/model.py index c2a42b291..bc87b3bdb 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1448,7 +1448,7 @@ class Relation: id: int """The identifier for a particular relation.""" - app: Optional[Application] + app: Application """Represents the remote application of this relation. For peer relations, this will be the local application. @@ -1478,32 +1478,32 @@ def __init__( backend: '_ModelBackend', cache: '_ModelCache', active: bool = True): self.name = relation_name self.id = relation_id - self.app: Optional[Application] = None self.units: Set[Unit] = set() self.active = active - if is_peer: - # For peer relations, both the remote and the local app are the same. - self.app = our_unit.app + # For peer relations, both the remote and the local app are the same. + app = our_unit.app if is_peer else None try: for unit_name in backend.relation_list(self.id): unit = cache.get(Unit, unit_name) self.units.add(unit) - if self.app is None: + if app is None: # Use the app of one of the units if available. - self.app = unit.app + app = unit.app except RelationNotFoundError: # If the relation is dead, just treat it as if it has no remote units. self.active = False # If we didn't get the remote app via our_unit.app or the units list, # look it up via JUJU_REMOTE_APP or "relation-list --app". - if self.app is None: + if app is None: app_name = backend.relation_remote_app_name(relation_id) if app_name is not None: - self.app = cache.get(Application, app_name) + app = cache.get(Application, app_name) + # self.app will not be None and always be set because of the fallback mechanism above. + self.app = typing.cast(Application, app) self.data = RelationData(self, our_unit, backend) def __repr__(self): diff --git a/ops/testing.py b/ops/testing.py index d8e821d02..9e144cb32 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -958,7 +958,7 @@ def add_relation_unit(self, relation_id: int, remote_unit_name: str) -> None: 'but no relation matching that name was found.') self._backend._relation_data_raw[relation_id][remote_unit_name] = {} - app = cast(model.Application, relation.app) # should not be None since we're testing + app = relation.app if not remote_unit_name.startswith(app.name): warnings.warn( 'Remote unit name invalid: the remote application of {} is called {!r}; ' From 5804652253926fea5c2aae5952d3032cea12ca5f Mon Sep 17 00:00:00 2001 From: PietroPasotti Date: Mon, 18 Mar 2024 09:25:14 +0100 Subject: [PATCH 7/9] fix: add_relation consistency check and default network (#1138) This PR makes two changes in the harness: - forbids doing add_relation on endpoints that haven't previously been defined in the charm metadata (something that scenario handles via the consistency checker) - automatically adds a default network, if not found already, for each newly added relation, to match juju's behaviour. Fixes #1137 Fixes #1136 --------- Co-authored-by: Tony Meyer Co-authored-by: Ben Hoyt --- ops/testing.py | 17 ++++++++++++++++ test/test_testing.py | 47 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index 9e144cb32..e3c9556e8 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -799,6 +799,9 @@ def add_relation(self, relation_name: str, remote_app: str, *, This function creates a relation with an application and triggers a :class:`RelationCreatedEvent `. + To match Juju's behaviour, it also creates a default network binding on this endpoint. + If you want to associate a custom network to this binding (or a global default network), + provide one using :meth:`add_network` before calling this function. If `app_data` or `unit_data` are provided, also add a new unit (``/0``) to the relation and trigger @@ -832,6 +835,11 @@ def add_relation(self, relation_name: str, remote_app: str, *, Return: The ID of the relation created. """ + if not (relation_name in self._meta.provides + or relation_name in self._meta.requires + or relation_name in self._meta.peers): + raise RelationNotFoundError(f'relation {relation_name!r} not declared in metadata') + relation_id = self._next_relation_id() self._backend._relation_ids_map.setdefault( relation_name, []).append(relation_id) @@ -859,6 +867,15 @@ def add_relation(self, relation_name: str, remote_app: str, *, if unit_data is not None: self.update_relation_data(relation_id, remote_unit, unit_data) + # If we have a default network binding configured, respect it. + if not self._backend._networks.get((None, None)): + # If we don't already have a network binding for this relation id, create one. + if not self._backend._networks.get((relation_name, relation_id)): + self.add_network("10.0.0.10", endpoint=relation_name, relation_id=relation_id) + # If we don't already have a default network binding for this endpoint, create one. + if not self._backend._networks.get((relation_name, None)): + self.add_network("192.0.2.0", endpoint=relation_name) + return relation_id def remove_relation(self, relation_id: int) -> None: diff --git a/test/test_testing.py b/test/test_testing.py index 47dd36e16..fcb33694f 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -92,6 +92,11 @@ def on_storage_changed(self, event: ops.EventBase): class TestHarness(unittest.TestCase): + def test_add_relation_no_meta_fails(self): + harness = ops.testing.Harness(ops.CharmBase, meta="name: mycharm") + self.addCleanup(harness.cleanup) + with self.assertRaises(ops.RelationNotFoundError): + harness.add_relation('db', 'postgresql') def test_add_relation(self): harness = ops.testing.Harness(ops.CharmBase, meta=''' @@ -3048,6 +3053,12 @@ def test_network_get_relation_not_found(self): assert binding is not None binding.network + def test_add_relation_network_get(self): + self.harness.add_relation('db', 'remote') + binding = self.harness.model.get_binding('db') + assert binding is not None + assert binding.network + def test_add_network_endpoint_not_in_meta(self): with self.assertRaises(ops.ModelError): self.harness.add_network('35.0.0.1', endpoint='xyz') @@ -4741,7 +4752,9 @@ def test_storage_mount(self): class TestSecrets(unittest.TestCase): def test_add_model_secret_by_app_name_str(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4753,7 +4766,9 @@ def test_add_model_secret_by_app_name_str(self): self.assertEqual(secret.get_content(), {'password': 'hunter2'}) def test_add_model_secret_by_app_instance(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4766,7 +4781,9 @@ def test_add_model_secret_by_app_instance(self): self.assertEqual(secret.get_content(), {'password': 'hunter3'}) def test_add_model_secret_by_unit_instance(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4779,7 +4796,9 @@ def test_add_model_secret_by_unit_instance(self): self.assertEqual(secret.get_content(), {'password': 'hunter4'}) def test_get_secret_as_owner(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) harness.begin() # App secret. @@ -4839,7 +4858,9 @@ def test_add_model_secret_invalid_content(self): harness.add_model_secret('database', {'x': 'y'}) # key too short def test_set_secret_content(self): - harness = ops.testing.Harness(EventRecorder, meta='name: webapp') + harness = ops.testing.Harness(EventRecorder, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4885,7 +4906,9 @@ def test_set_secret_content_invalid_content(self): harness.set_secret_content(secret_id, {'x': 'y'}) def test_grant_secret_and_revoke_secret(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4901,7 +4924,9 @@ def test_grant_secret_and_revoke_secret(self): harness.model.get_secret(id=secret_id) def test_grant_secret_wrong_app(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4912,7 +4937,9 @@ def test_grant_secret_wrong_app(self): harness.model.get_secret(id=secret_id) def test_grant_secret_wrong_unit(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: webapp') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'webapp', 'requires': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'database') harness.add_relation_unit(relation_id, 'database/0') @@ -4931,7 +4958,9 @@ def test_grant_secret_no_relation(self): harness.grant_secret(secret_id, 'webapp') def test_get_secret_grants(self): - harness = ops.testing.Harness(ops.CharmBase, meta='name: database') + harness = ops.testing.Harness(ops.CharmBase, meta=yaml.safe_dump( + {'name': 'database', 'provides': {'db': {'interface': 'pgsql'}}} + )) self.addCleanup(harness.cleanup) relation_id = harness.add_relation('db', 'webapp') From 9fcb04a9a0519c7d0acbc54cf53c027c202df377 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 18 Mar 2024 21:30:33 +1300 Subject: [PATCH 8/9] fix(harness): don't error out when attempting attaching storage before begin (#1150) One minor fix to the `storage` metadata: [charmcraft.yaml](https://juju.is/docs/sdk/charmcraft-yaml#heading--storage) and [metadata.yaml](https://juju.is/docs/sdk/metadata-yaml#heading--storage) support a `multiple.range` value in the form `+` - this would previously error out when we attempted to read the metadata. We now support this (as an alias for `-`, which is an open-ended range rather than "n or less", and have a test for it. Adds a minor clarification in the API docs when `Harness.add_storage` will emit a storage-attached event. In `begin_with_initial_hooks`, if the storage has already been attached, then don't try to attach it a second time, but *do* emit the storage-attached event, since that will not have been done previously. Make the `_storage_attach` method safe to call multiple times. This appears to have been the intention based on comments in the code, but, in practice, attempting to symlink the mount would fail. Now, if the symlink already exists (and has the same target) then we continue. Adds a variety of tests for Harness `add_storage` and `attach_storage` behaviour. Fixes #1127 --------- Co-authored-by: Ben Hoyt --- ops/charm.py | 4 +- ops/testing.py | 28 ++++++++++---- test/test_charm.py | 5 +++ test/test_testing.py | 90 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 8 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 59ad4c5e7..eae71a2f6 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1540,7 +1540,9 @@ def __init__(self, name: str, raw: '_StorageMetaDict'): self.multiple_range = None if 'multiple' in raw: range = raw['multiple']['range'] - if '-' not in range: + if range[-1] == '+': + self.multiple_range = (int(range[:-1]), None) + elif '-' not in range: self.multiple_range = (int(range), int(range)) else: range = range.split('-') diff --git a/ops/testing.py b/ops/testing.py index e3c9556e8..cb395be51 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -405,7 +405,11 @@ def begin_with_initial_hooks(self) -> None: for storage_name in self._meta.storages: for storage_index in self._backend.storage_list(storage_name, include_detached=True): s = model.Storage(storage_name, storage_index, self._backend) - self.attach_storage(s.full_id) + if self._backend._storage_is_attached(storage_name, storage_index): + # Attaching was done already, but we still need the event to be emitted. + self.charm.on[storage_name].storage_attached.emit(s) + else: + self.attach_storage(s.full_id) # Storage done, emit install event charm.on.install.emit() @@ -690,8 +694,8 @@ def add_storage(self, storage_name: str, count: int = 1, Args: storage_name: The storage backend name on the Charm count: Number of disks being added - attach: True to also attach the storage mount and emit storage-attached if - harness.begin() has been called. + attach: True to also attach the storage mount; if :meth:`begin` + has been called a True value will also emit storage-attached Return: A list of storage IDs, e.g. ["my-storage/1", "my-storage/2"]. @@ -739,12 +743,12 @@ def attach_storage(self, storage_id: str) -> None: """Attach a storage device. The intent of this function is to simulate a ``juju attach-storage`` call. - It will trigger a storage-attached hook if the storage unit in question exists + If called after :meth:`begin` and hooks are not disabled, it will trigger + a storage-attached hook if the storage unit in question exists and is presently marked as detached. The test harness uses symbolic links to imitate storage mounts, which may lead to some - inconsistencies compared to the actual charm. Users should be cognizant of - this potential discrepancy. + inconsistencies compared to the actual charm. Args: storage_id: The full storage ID of the storage unit being attached, including the @@ -2339,7 +2343,17 @@ def _storage_attach(self, storage_id: str): mounting_dir.parent.mkdir(parents=True, exist_ok=True) target_dir = pathlib.Path(store["location"]) target_dir.mkdir(parents=True, exist_ok=True) - mounting_dir.symlink_to(target_dir) + try: + mounting_dir.symlink_to(target_dir, target_is_directory=True) + except FileExistsError: + # If the symlink is already the one we want, then we + # don't need to do anything here. + # NOTE: In Python 3.9, this can use `mounting_dir.readlink()` + if ( + not mounting_dir.is_symlink() + or os.readlink(mounting_dir) != str(target_dir) + ): + raise index = int(index) if not self._storage_is_attached(name, index): diff --git a/test/test_charm.py b/test/test_charm.py index 1a9ac52c5..9767588a2 100644 --- a/test/test_charm.py +++ b/test/test_charm.py @@ -282,6 +282,10 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent): multiple: range: 2- type: filesystem + stor-plus: + multiple: + range: 10+ + type: filesystem ''') fake_script( @@ -329,6 +333,7 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent): self.assertEqual(self.meta.storages['stor2'].multiple_range, (2, 2)) self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None)) self.assertEqual(self.meta.storages['stor-4'].multiple_range, (2, 4)) + self.assertEqual(self.meta.storages['stor-plus'].multiple_range, (10, None)) charm = MyCharm(self.create_framework()) diff --git a/test/test_testing.py b/test/test_testing.py index fcb33694f..d531e08d5 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -4749,6 +4749,96 @@ def test_storage_mount(self): self.harness.attach_storage(storage_id) self.assertTrue((self.root / "mounts/foo/bar").read_text(), "foobar") + def _make_storage_attach_harness(self, meta: typing.Optional[str] = None): + class MyCharm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + self.attached: typing.List[str] = [] + self.locations: typing.List[pathlib.Path] = [] + framework.observe(self.on['test-storage'].storage_attached, self._on_attach) + + def _on_attach(self, event: ops.StorageAttachedEvent): + self.attached.append(event.storage.full_id) + self.locations.append(event.storage.location) + + if meta is None: + meta = ''' + name: test + containers: + test-container: + mounts: + - storage: test-storage + location: /mounts/foo + storage: + test-storage: + type: filesystem + ''' + harness = ops.testing.Harness(MyCharm, meta=meta) + self.addCleanup(harness.cleanup) + return harness + + def test_storage_attach_begin_no_emit(self): + """If `begin()` hasn't been called, `attach` does not emit storage-attached.""" + harness = self._make_storage_attach_harness() + harness.add_storage('test-storage', attach=True) + harness.begin() + self.assertNotIn('test-storage/0', harness.charm.attached) + + def test_storage_attach_begin_with_hooks_emits(self): + """`attach` doesn't emit storage-attached before `begin_with_initial_hooks`.""" + harness = self._make_storage_attach_harness() + harness.add_storage('test-storage', attach=True) + harness.begin_with_initial_hooks() + self.assertIn('test-storage/0', harness.charm.attached) + self.assertTrue(harness.charm.locations[0]) + + def test_storage_add_with_later_attach(self): + harness = self._make_storage_attach_harness() + harness.begin() + storage_ids = harness.add_storage('test-storage', attach=False) + self.assertNotIn('test-storage/0', harness.charm.attached) + for s_id in storage_ids: + harness.attach_storage(s_id) + # It's safe to call `attach_storage` more than once, and this will + # only trigger the event once - this is the same as executing + # `juju attach-storage ` more than once. + harness.attach_storage(s_id) + self.assertEqual(harness.charm.attached.count('test-storage/0'), 1) + + def test_storage_machine_charm_metadata(self): + meta = ''' + name: test + storage: + test-storage: + type: filesystem + mount: /mounts/foo + ''' + harness = self._make_storage_attach_harness(meta) + harness.begin() + harness.add_storage('test-storage', attach=True) + self.assertIn('test-storage/0', harness.charm.attached) + + def test_storage_multiple_storage_instances(self): + meta = ''' + name: test + storage: + test-storage: + type: filesystem + mount: /mounts/foo + multiple: + range: 2-4 + ''' + harness = self._make_storage_attach_harness(meta) + harness.begin() + harness.add_storage('test-storage', 2, attach=True) + self.assertEqual(harness.charm.attached, ['test-storage/0', 'test-storage/1']) + self.assertNotEqual(harness.charm.locations[0], harness.charm.locations[1]) + harness.add_storage('test-storage', 2, attach=True) + self.assertEqual( + harness.charm.attached, [ + 'test-storage/0', 'test-storage/1', 'test-storage/2', 'test-storage/3']) + self.assertEqual(len(set(harness.charm.locations)), 4) + class TestSecrets(unittest.TestCase): def test_add_model_secret_by_app_name_str(self): From 8097cca861eae31727636f92c932738d14d0ac81 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Thu, 21 Mar 2024 10:56:47 +0800 Subject: [PATCH 9/9] feat(Model): support credential-get hook tool in both model and harness (#1152) Added `Model.get_cloud_spec` which uses the `credential-get` hook tool to get details of the cloud where the model is deployed. --- CHANGES.md | 1 + ops/__init__.py | 4 ++ ops/model.py | 105 ++++++++++++++++++++++++++++++++++++ ops/testing.py | 50 +++++++++++++++++ test/test_model.py | 126 +++++++++++++++++++++++++++++++++++++++++++ test/test_testing.py | 38 +++++++++++++ 6 files changed, 324 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index e2d5768cb..5faa20943 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,7 @@ # 2.12.0 * Updated Pebble Notices `get_notices` parameter name to `users=all` (previously `select=all`). +* Added `Model.get_cloud_spec` which uses the `credential-get` hook tool to get details of the cloud where the model is deployed. # 2.11.0 diff --git a/ops/__init__.py b/ops/__init__.py index 2f359c2b0..60de7ad4a 100644 --- a/ops/__init__.py +++ b/ops/__init__.py @@ -129,6 +129,8 @@ 'BindingMapping', 'BlockedStatus', 'CheckInfoMapping', + 'CloudCredential', + 'CloudSpec', 'ConfigData', 'Container', 'ContainerMapping', @@ -270,6 +272,8 @@ BindingMapping, BlockedStatus, CheckInfoMapping, + CloudCredential, + CloudSpec, ConfigData, Container, ContainerMapping, diff --git a/ops/model.py b/ops/model.py index bc87b3bdb..9ccadcd82 100644 --- a/ops/model.py +++ b/ops/model.py @@ -281,6 +281,21 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) - content = self._backend.secret_get(id=id, label=label) return Secret(self._backend, id=id, label=label, content=content) + def get_cloud_spec(self) -> 'CloudSpec': + """Get details of the cloud in which the model is deployed. + + Note: This information is only available for machine charms, + not Kubernetes sidecar charms. + + Returns: + a specification for the cloud in which the model is deployed, + including credential information. + + Raises: + :class:`ModelError`: if called in a Kubernetes model. + """ + return self._backend.credential_get() + if typing.TYPE_CHECKING: # (entity type, name): instance. @@ -3507,6 +3522,14 @@ def reboot(self, now: bool = False): else: self._run("juju-reboot") + def credential_get(self) -> 'CloudSpec': + """Access cloud credentials by running the credential-get hook tool. + + Returns the cloud specification used by the model. + """ + result = self._run('credential-get', return_output=True, use_json=True) + return CloudSpec.from_dict(typing.cast(Dict[str, Any], result)) + class _ModelBackendValidator: """Provides facilities for validating inputs and formatting them for model backends.""" @@ -3596,3 +3619,85 @@ def _ensure_loaded(self): self._notice = self._container.get_notice(self.id) assert self._notice.type == self.type assert self._notice.key == self.key + + +@dataclasses.dataclass(frozen=True) +class CloudCredential: + """Credentials for cloud. + + Used as the type of attribute `credential` in :class:`CloudSpec`. + """ + + auth_type: str + """Authentication type.""" + + attributes: Dict[str, str] = dataclasses.field(default_factory=dict) + """A dictionary containing cloud credentials. + + For example, for AWS, it contains `access-key` and `secret-key`; + for Azure, `application-id`, `application-password` and `subscription-id` + can be found here. + """ + + redacted: List[str] = dataclasses.field(default_factory=list) + """A list of redacted secrets.""" + + @classmethod + def from_dict(cls, d: Dict[str, Any]) -> 'CloudCredential': + """Create a new CloudCredential object from a dictionary.""" + return cls( + auth_type=d['auth-type'], + attributes=d.get('attrs') or {}, + redacted=d.get('redacted') or [], + ) + + +@dataclasses.dataclass(frozen=True) +class CloudSpec: + """Cloud specification information (metadata) including credentials.""" + + type: str + """Type of the cloud.""" + + name: str + """Juju cloud name.""" + + region: Optional[str] = None + """Region of the cloud.""" + + endpoint: Optional[str] = None + """Endpoint of the cloud.""" + + identity_endpoint: Optional[str] = None + """Identity endpoint of the cloud.""" + + storage_endpoint: Optional[str] = None + """Storage endpoint of the cloud.""" + + credential: Optional[CloudCredential] = None + """Cloud credentials with key-value attributes.""" + + ca_certificates: List[str] = dataclasses.field(default_factory=list) + """A list of CA certificates.""" + + skip_tls_verify: bool = False + """Whether to skip TLS verfication.""" + + is_controller_cloud: bool = False + """If this is the cloud used by the controller, defaults to False.""" + + @classmethod + def from_dict(cls, d: Dict[str, Any]) -> 'CloudSpec': + """Create a new CloudSpec object from a dict parsed from JSON.""" + return cls( + type=d['type'], + name=d['name'], + region=d.get('region') or None, + endpoint=d.get('endpoint') or None, + identity_endpoint=d.get('identity-endpoint') or None, + storage_endpoint=d.get('storage-endpoint') or None, + credential=CloudCredential.from_dict(d['credential']) if d.get('credential') else None, + ca_certificates=d.get('cacertificates') or [], + skip_tls_verify=d.get('skip-tls-verify') or False, + is_controller_cloud=d.get('is-controller-cloud') or False, + ) diff --git a/ops/testing.py b/ops/testing.py index cb395be51..5d8cc838c 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -1911,6 +1911,49 @@ def run_action(self, action_name: str, output=action_under_test.output) return action_under_test.output + def set_cloud_spec(self, spec: 'model.CloudSpec'): + """Set cloud specification (metadata) including credentials. + + Call this method before the charm calls :meth:`ops.Model.get_cloud_spec`. + + Example usage:: + + class MyVMCharm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + framework.observe(self.on.start, self._on_start) + + def _on_start(self, event: ops.StartEvent): + self.cloud_spec = self.model.get_cloud_spec() + + class TestCharm(unittest.TestCase): + def setUp(self): + self.harness = ops.testing.Harness(MyVMCharm) + self.addCleanup(self.harness.cleanup) + + def test_start(self): + cloud_spec_dict = { + 'name': 'localhost', + 'type': 'lxd', + 'endpoint': 'https://127.0.0.1:8443', + 'credential': { + 'authtype': 'certificate', + 'attrs': { + 'client-cert': 'foo', + 'client-key': 'bar', + 'server-cert': 'baz' + }, + }, + } + self.harness.set_cloud_spec(ops.CloudSpec.from_dict(cloud_spec_dict)) + self.harness.begin() + self.harness.charm.on.start.emit() + expected = ops.CloudSpec.from_dict(cloud_spec_dict) + self.assertEqual(harness.charm.cloud_spec, expected) + + """ + self._backend._cloud_spec = spec + def _get_app_or_unit_name(app_or_unit: AppUnitOrName) -> str: """Return name of given application or unit (return strings directly).""" @@ -2126,6 +2169,7 @@ def __init__(self, unit_name: str, meta: charm.CharmMeta, config: '_RawConfig'): self._networks: Dict[Tuple[Optional[str], Optional[int]], _NetworkDict] = {} self._reboot_count = 0 self._running_action: Optional[_RunningAction] = None + self._cloud_spec: Optional[model.CloudSpec] = None def _validate_relation_access(self, relation_name: str, relations: List[model.Relation]): """Ensures that the named relation exists/has been added. @@ -2709,6 +2753,12 @@ def reboot(self, now: bool = False): # to handle everything after the exit. raise SystemExit() + def credential_get(self) -> model.CloudSpec: + if not self._cloud_spec: + raise model.ModelError( + 'ERROR cloud spec is empty, set it with `Harness.set_cloud_spec()` first') + return self._cloud_spec + @_copy_docstrings(pebble.ExecProcess) class _TestingExecProcess: diff --git a/test/test_model.py b/test/test_model.py index 9155217d8..c1f990c65 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -3791,5 +3791,131 @@ def test_repr(self): ) +class TestCloudCredential(unittest.TestCase): + def test_from_dict(self): + d = { + 'auth-type': 'certificate', + } + cloud_cred = ops.CloudCredential.from_dict(d) + self.assertEqual(cloud_cred.auth_type, d['auth-type']) + self.assertEqual(cloud_cred.attributes, {}) + self.assertEqual(cloud_cred.redacted, []) + + def test_from_dict_full(self): + d = { + 'auth-type': 'certificate', + 'attrs': { + 'client-cert': 'foo', + 'client-key': 'bar', + 'server-cert': 'baz' + }, + 'redacted': ['foo'] + } + cloud_cred = ops.CloudCredential.from_dict(d) + self.assertEqual(cloud_cred.auth_type, d['auth-type']) + self.assertEqual(cloud_cred.attributes, d['attrs']) + self.assertEqual(cloud_cred.redacted, d['redacted']) + + +class TestCloudSpec(unittest.TestCase): + def test_from_dict(self): + cloud_spec = ops.CloudSpec.from_dict( + { + 'type': 'lxd', + 'name': 'localhost', + } + ) + self.assertEqual(cloud_spec.type, 'lxd') + self.assertEqual(cloud_spec.name, 'localhost') + self.assertEqual(cloud_spec.region, None) + self.assertEqual(cloud_spec.endpoint, None) + self.assertEqual(cloud_spec.identity_endpoint, None) + self.assertEqual(cloud_spec.storage_endpoint, None) + self.assertIsNone(cloud_spec.credential) + self.assertEqual(cloud_spec.ca_certificates, []) + self.assertEqual(cloud_spec.skip_tls_verify, False) + self.assertEqual(cloud_spec.is_controller_cloud, False) + + def test_from_dict_full(self): + cred = { + 'auth-type': 'certificate', + 'attrs': { + 'client-cert': 'foo', + 'client-key': 'bar', + 'server-cert': 'baz' + }, + 'redacted': ['foo'] + } + d = { + 'type': 'lxd', + 'name': 'localhost', + 'region': 'localhost', + 'endpoint': 'https://10.76.251.1:8443', + 'credential': cred, + 'identity-endpoint': 'foo', + 'storage-endpoint': 'bar', + 'cacertificates': ['foo', 'bar'], + 'skip-tls-verify': False, + 'is-controller-cloud': True, + } + cloud_spec = ops.CloudSpec.from_dict(d) + self.assertEqual(cloud_spec.type, d['type']) + self.assertEqual(cloud_spec.name, d['name']) + self.assertEqual(cloud_spec.region, d['region']) + self.assertEqual(cloud_spec.endpoint, d['endpoint']) + self.assertEqual(cloud_spec.credential, ops.CloudCredential.from_dict(cred)) + self.assertEqual(cloud_spec.identity_endpoint, d['identity-endpoint']) + self.assertEqual(cloud_spec.storage_endpoint, d['storage-endpoint']) + self.assertEqual(cloud_spec.ca_certificates, d['cacertificates']) + self.assertEqual(cloud_spec.skip_tls_verify, False) + self.assertEqual(cloud_spec.is_controller_cloud, True) + + def test_from_dict_no_credential(self): + d = { + 'type': 'lxd', + 'name': 'localhost', + 'region': 'localhost', + 'endpoint': 'https://10.76.251.1:8443', + 'identity-endpoint': 'foo', + 'storage-endpoint': 'bar', + 'cacertificates': ['foo', 'bar'], + 'skip-tls-verify': False, + 'is-controller-cloud': True, + } + cloud_spec = ops.CloudSpec.from_dict(d) + self.assertEqual(cloud_spec.type, d['type']) + self.assertEqual(cloud_spec.name, d['name']) + self.assertEqual(cloud_spec.region, d['region']) + self.assertEqual(cloud_spec.endpoint, d['endpoint']) + self.assertIsNone(cloud_spec.credential) + self.assertEqual(cloud_spec.identity_endpoint, d['identity-endpoint']) + self.assertEqual(cloud_spec.storage_endpoint, d['storage-endpoint']) + self.assertEqual(cloud_spec.ca_certificates, d['cacertificates']) + self.assertEqual(cloud_spec.skip_tls_verify, False) + self.assertEqual(cloud_spec.is_controller_cloud, True) + + +class TestGetCloudSpec(unittest.TestCase): + def setUp(self): + self.model = ops.Model(ops.CharmMeta(), _ModelBackend('myapp/0')) + + def test_success(self): + fake_script(self, 'credential-get', """echo '{"type": "lxd", "name": "localhost"}'""") + cloud_spec = self.model.get_cloud_spec() + self.assertEqual(cloud_spec.type, 'lxd') + self.assertEqual(cloud_spec.name, 'localhost') + self.assertEqual(fake_script_calls(self, clear=True), + [['credential-get', '--format=json']]) + + def test_error(self): + fake_script( + self, + 'credential-get', + """echo 'ERROR cannot access cloud credentials' >&2; exit 1""") + with self.assertRaises(ops.ModelError) as cm: + self.model.get_cloud_spec() + self.assertEqual(str(cm.exception), 'ERROR cannot access cloud credentials\n') + + if __name__ == "__main__": unittest.main() diff --git a/test/test_testing.py b/test/test_testing.py index d531e08d5..504a851e2 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -5854,3 +5854,41 @@ def test_get_notices(self): class TestNotices(unittest.TestCase, _TestingPebbleClientMixin, PebbleNoticesMixin): def setUp(self): self.client = self.get_testing_client() + + +class TestCloudSpec(unittest.TestCase): + def test_set_cloud_spec(self): + class TestCharm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + framework.observe(self.on.start, self._on_start) + + def _on_start(self, event: ops.StartEvent): + self.cloud_spec = self.model.get_cloud_spec() + + harness = ops.testing.Harness(TestCharm) + self.addCleanup(harness.cleanup) + cloud_spec_dict = { + 'name': 'localhost', + 'type': 'lxd', + 'endpoint': 'https://127.0.0.1:8443', + 'credential': { + 'auth-type': 'certificate', + 'attrs': { + 'client-cert': 'foo', + 'client-key': 'bar', + 'server-cert': 'baz' + }, + }, + } + harness.set_cloud_spec(ops.CloudSpec.from_dict(cloud_spec_dict)) + harness.begin() + harness.charm.on.start.emit() + self.assertEqual(harness.charm.cloud_spec, ops.CloudSpec.from_dict(cloud_spec_dict)) + + def test_get_cloud_spec_without_set_error(self): + harness = ops.testing.Harness(ops.CharmBase) + self.addCleanup(harness.cleanup) + harness.begin() + with self.assertRaises(ops.ModelError): + harness.model.get_cloud_spec()