From 16836a25e745b2dd24feafff1a8fc24f8810b790 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 15:19:27 +1200 Subject: [PATCH 01/20] Add type for ActionEvent.params --- ops/charm.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 11228b0de..15d924128 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -103,13 +103,13 @@ class ActionEvent(EventBase): To respond with the result of the action, call :meth:`set_results`. To add progress messages that are visible as the action is progressing use :meth:`log`. - - Attributes: - params: The parameters passed to the action. """ + params: Dict[str, Any] + """The parameters passed to the action.""" + def defer(self): - """Action events are not deferable like other events. + """Action events are not deferrable like other events. This is because an action runs synchronously and the administrator is waiting for the result. From d2a195a001277b6b1a41f02b53ceae6d768c48a6 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 15:19:53 +1200 Subject: [PATCH 02/20] Avoid extra indent on ActionEvent.set_results bulleted list --- ops/charm.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 15d924128..ba985e125 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -155,11 +155,11 @@ def set_results(self, results: Dict[str, Any]): results object. Thus, the results object might contain the following keys, additionally to those specified by the charm code: - - Stdout - - Stderr - - Stdout-encoding - - Stderr-encoding - - ReturnCode + - Stdout + - Stderr + - Stdout-encoding + - Stderr-encoding + - ReturnCode Args: results: The result of the action as a Dict From 34606344a18f346b50f7268fda1a3d312c395219 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 15:20:45 +1200 Subject: [PATCH 03/20] Fix docs for StatusBase subclasses (was using __new__ signature) --- ops/model.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ops/model.py b/ops/model.py index 51d03883c..3b82412a1 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1517,13 +1517,9 @@ class StatusBase: name = NotImplemented def __init__(self, message: str = ''): - self.message = message - - def __new__(cls, *args: Any, **kwargs: Dict[Any, Any]): - """Forbid the usage of StatusBase directly.""" - if cls is StatusBase: + if self.__class__ is StatusBase: raise TypeError("cannot instantiate a base class") - return super().__new__(cls) + self.message = message def __eq__(self, other: 'StatusBase') -> bool: if not isinstance(self, type(other)): From 9e9ded490965a1abd7fa42c0ccbabf96c39c4fbe Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 15:20:55 +1200 Subject: [PATCH 04/20] Update action_get signature --- ops/model.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ops/model.py b/ops/model.py index 3b82412a1..a11a4b67e 100644 --- a/ops/model.py +++ b/ops/model.py @@ -2883,9 +2883,9 @@ def storage_add(self, name: str, count: int = 1) -> None: raise TypeError(f'storage count must be integer, got: {count} ({type(count)})') self._run('storage-add', f'{name}={count}') - def action_get(self) -> Dict[str, str]: # todo: what do we know about this dict? + def action_get(self) -> Dict[str, Any]: out = self._run('action-get', return_output=True, use_json=True) - return typing.cast(Dict[str, str], out) + return typing.cast(Dict[str, Any], out) def action_set(self, results: Dict[str, Any]) -> None: # The Juju action-set hook tool cannot interpret nested dicts, so we use a helper to From 60e64cb6410097ad9eeb1b8c03814da5573539f6 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 15:50:36 +1200 Subject: [PATCH 05/20] Type annotate a couple of attributes --- ops/model.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/ops/model.py b/ops/model.py index a11a4b67e..f79241185 100644 --- a/ops/model.py +++ b/ops/model.py @@ -293,11 +293,13 @@ class Application: This might be your application, or might be an application that you are related to. 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. + """ - Attributes: - name: The name of this application (eg, 'mysql'). This name may differ from the name of - the charm, if the user has deployed it to a different name. + name: str + """The name of this application (eg, 'mysql'). This name may differ from the name of + the charm, if the user has deployed it to a different name. """ def __init__(self, name: str, meta: 'ops.charm.CharmMeta', @@ -777,11 +779,10 @@ def __len__(self) -> int: class Binding: - """Binding to a network space. + """Binding to a network space.""" - Attributes: - name: The name of the endpoint this binding represents (eg, 'db') - """ + name: str + """The name of the endpoint this binding represents (eg, 'db').""" def __init__(self, name: str, relation_id: Optional[int], backend: '_ModelBackend'): self.name = name From b2f7cd971cc469d2d6a28741d0c39422abd4b85f Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 14 Jun 2023 16:01:22 +1200 Subject: [PATCH 06/20] Fixes to docstrings of CharmBase and .on property --- ops/charm.py | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index ba985e125..0f3f02bfb 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -870,30 +870,24 @@ class CharmEvents(ObjectEvents): class CharmBase(Object): """Base class that represents the charm overall. - :class:`CharmBase` is used to create a charm. This is done by inheriting - from :class:`CharmBase` and customising the sub class as required. So to + :code:`CharmBase` is used to create a charm. This is done by inheriting + from :code:`CharmBase` and customising the subclass as required. So to create your own charm, say ``MyCharm``, define a charm class and set up the required event handlers (“hooks”) in its constructor:: import logging - from ops.charm import CharmBase - from ops.main import main + import ops - logger = logging.getLogger(__name__) - - def MyCharm(CharmBase): + def MyCharm(ops.CharmBase): def __init__(self, *args): - logger.debug('Initializing Charm') - super().__init__(*args) - self.framework.observe(self.on.config_changed, self._on_config_changed) self.framework.observe(self.on.stop, self._on_stop) # ... if __name__ == "__main__": - main(MyCharm) + ops.main(MyCharm) As shown in the example above, a charm class is instantiated by :code:`ops.main` rather than charm authors directly instantiating a @@ -904,10 +898,11 @@ def __init__(self, *args): charm. """ - # note that without the #: below, sphinx will copy the whole of CharmEvents - # docstring inline which is less than ideal. - # Used to set up event handlers; see :class:`CharmEvents`. - on = CharmEvents() # type: ignore + on: CharmEvents = CharmEvents() # type: ignore + """This property is used to create an event handler using :meth:`Framework.observe`, + and can be one of the events listed at :class:`CharmEvents`. + """ + if TYPE_CHECKING: # to help the type checker and IDEs: @property From 0a7b2cff2cafd388a7db6eb029c86d32fa7abc56 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 15 Jun 2023 15:00:14 +1200 Subject: [PATCH 07/20] Significantly improve CharmEvents docstring and event list --- ops/charm.py | 75 ++++++++++++++++++++++++++++++++---------------- ops/framework.py | 7 +++-- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 0f3f02bfb..9e5bc6a11 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -714,7 +714,7 @@ def restore(self, snapshot: Dict[str, Any]): class SecretChangedEvent(SecretEvent): - """Event raised by Juju on the observer when the secret owner changes its contents. + """Event triggered by Juju on the observer when the secret owner changes its contents. When the owner of a secret changes the secret's contents, Juju will create a new secret revision, and all applications or units that are tracking this @@ -727,7 +727,7 @@ class SecretChangedEvent(SecretEvent): class SecretRotateEvent(SecretEvent): - """Event raised by Juju on the owner when the secret's rotation policy elapses. + """Event triggered by Juju on the owner when the secret's rotation policy elapses. This event is fired on the secret owner to inform it that the secret must be rotated. The event will keep firing until the owner creates a new @@ -742,7 +742,7 @@ def defer(self): class SecretRemoveEvent(SecretEvent): - """Event raised by Juju on the owner when a secret revision can be removed. + """Event triggered by Juju on the owner when a secret revision can be removed. When the owner of a secret creates a new revision, and after all observers have updated to that new revision, this event will be fired to @@ -780,7 +780,7 @@ def restore(self, snapshot: Dict[str, Any]): class SecretExpiredEvent(SecretEvent): - """Event raised by Juju on the owner when a secret's expiration time elapses. + """Event triggered by Juju on the owner when a secret's expiration time elapses. This event is fired on the secret owner to inform it that the secret revision must be removed. The event will keep firing until the owner removes the @@ -823,48 +823,75 @@ def defer(self): class CharmEvents(ObjectEvents): """Events generated by Juju pertaining to application lifecycle. - This class is used to create an event descriptor (``self.on``) attribute for + This class is used to create an event descriptor (``self.on``) for a charm class that inherits from :class:`CharmBase`. The event descriptor may be used to set up event handlers for corresponding events. - By default the following events will be provided through - :class:`CharmBase`:: - - self.on.install - self.on.start - self.on.remove - self.on.update_status - self.on.config_changed - self.on.upgrade_charm - self.on.pre_series_upgrade - self.on.post_series_upgrade - self.on.leader_elected - self.on.collect_metrics - - - In addition to these, depending on the charm's metadata (``metadata.yaml``), - named relation and storage events may also be defined. These named events - are created by :class:`CharmBase` using charm metadata. The named events may be - accessed as ``self.on[].`` + By default, the events listed below will be provided via the + :attr:`CharmBase.on` attribute. For example:: + + self.framework.observe(self.on.config_changed, self._on_config_changed) + + In addition to the events listed below, dynamically-named events will also + be defined based on the charm's metadata (``metadata.yaml``) for relations, + storage, actions, and containers. These named events may be accessed as + ``self.on[].`` or using a prefix like ``self.on._``, + for example:: + + self.framework.observe(self.on["db"].relation_created, self._on_db_relation_created) + self.framework.observe(self.on.workload_pebble_ready, self._on_workload_pebble_ready) """ + # NOTE: The one-line docstrings below are copied from the first line of + # each event class's docstring. Please keep in sync. + install = EventSource(InstallEvent) + """Triggered when a charm is installed (see :class:`InstallEvent`).""" + start = EventSource(StartEvent) + """Triggered immediately after first configuration change (see :class:`StartEvent`).""" + stop = EventSource(StopEvent) + """Triggered when a charm is shut down (see :class:`StopEvent`).""" + remove = EventSource(RemoveEvent) + """Triggered when a unit is about to be terminated (see :class:`RemoveEvent`).""" + update_status = EventSource(UpdateStatusEvent) + """Triggered periodically by a status update request from Juju (see :class:`UpdateStatusEvent`).""" + config_changed = EventSource(ConfigChangedEvent) + """Triggered when a configuration change occurs (see :class:`ConfigChangedEvent`).""" + upgrade_charm = EventSource(UpgradeCharmEvent) + """Triggered by request to upgrade the charm (see :class:`UpgradeCharmEvent`).""" + pre_series_upgrade = EventSource(PreSeriesUpgradeEvent) + """Triggered to prepare a unit for series upgrade (see :class:`PreSeriesUpgradeEvent`).""" + post_series_upgrade = EventSource(PostSeriesUpgradeEvent) + """Triggered after a series upgrade (see :class:`PostSeriesUpgradeEvent`).""" + leader_elected = EventSource(LeaderElectedEvent) + """Triggered when a new leader has been elected (see :class:`LeaderElectedEvent`).""" + leader_settings_changed = EventSource(LeaderSettingsChangedEvent) + """DEPRECATED. Triggered when leader changes any settings (see :class:`LeaderSettingsChangedEvent`).""" + collect_metrics = EventSource(CollectMetricsEvent) + """Triggered by Juju to collect metrics (see :class:`CollectMetricsEvent`).""" secret_changed = EventSource(SecretChangedEvent) + """Triggered by Juju on the observer when the secret owner changes its contents (see :class:`SecretChangedEvent`).""" + secret_expired = EventSource(SecretExpiredEvent) + """Triggered by Juju on the owner when a secret's expiration time elapses (see :class:`SecretExpiredEvent`).""" + secret_rotate = EventSource(SecretRotateEvent) + """Triggered by Juju on the owner when the secret's rotation policy elapses (see :class:`SecretRotateEvent`).""" + secret_remove = EventSource(SecretRemoveEvent) + """Triggered by Juju on the owner when a secret revision can be removed (see :class:`SecretRemoveEvent`).""" class CharmBase(Object): diff --git a/ops/framework.py b/ops/framework.py index e85330509..1415fe9ce 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -263,7 +263,7 @@ def restore(self, snapshot: Dict[str, Any]): class EventSource: """EventSource wraps an event type with a descriptor to facilitate observing and emitting. - It is generally used as: + It is generally used as:: class SomethingHappened(EventBase): pass @@ -271,8 +271,9 @@ class SomethingHappened(EventBase): class SomeObject(Object): something_happened = EventSource(SomethingHappened) - With that, instances of that type will offer the someobj.something_happened - attribute which is a BoundEvent and may be used to emit and observe the event. + With that, instances of that type will offer the ``someobj.something_happened`` + attribute which is a :class:`BoundEvent`, and may be used to emit and observe + the event. """ def __init__(self, event_type: 'Type[EventBase]'): From b2cd31f56e85f030574cec0d476b8740de34cbc9 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 15 Jun 2023 15:24:50 +1200 Subject: [PATCH 08/20] Significantly improve CharmMeta docstring and attribute type info --- ops/charm.py | 114 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 69 insertions(+), 45 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 9e5bc6a11..a0a97a104 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -988,57 +988,81 @@ def config(self) -> model.ConfigData: class CharmMeta: """Object containing the metadata for the charm. - This is read from ``metadata.yaml`` and/or ``actions.yaml``. Generally + This is read from ``metadata.yaml`` and ``actions.yaml``. Generally charms will define this information, rather than reading it at runtime. This class is mostly for the framework to understand what the charm has defined. - The :attr:`maintainers`, :attr:`tags`, :attr:`terms`, :attr:`series`, and - :attr:`extra_bindings` attributes are all lists of strings. The :attr:`containers`, - :attr:`requires`, :attr:`provides`, :attr:`peers`, :attr:`relations`, - :attr:`storages`, :attr:`resources`, and :attr:`payloads` attributes are all - mappings of names to instances of the respective :class:`RelationMeta`, - :class:`StorageMeta`, :class:`ResourceMeta`, or :class:`PayloadMeta`. - - The :attr:`relations` attribute is a convenience accessor which includes all - of the ``requires``, ``provides``, and ``peers`` :class:`RelationMeta` - items. If needed, the role of the relation definition can be obtained from - its :attr:`role ` attribute. - - Attributes: - name: The name of this charm - summary: Short description of what this charm does - description: Long description for this charm - maintainers: A list of strings of the email addresses of the maintainers - of this charm. - tags: Charm store tag metadata for categories associated with this charm. - terms: Charm store terms that should be agreed to before this charm can - be deployed. (Used for things like licensing issues.) - series: The list of supported OS series that this charm can support. - The first entry in the list is the default series that will be - used by deploy if no other series is requested by the user. - subordinate: True/False whether this charm is intended to be used as a - subordinate charm. - min_juju_version: If supplied, indicates this charm needs features that - are not available in older versions of Juju. - containers: A dict of {name: :class:`ContainerMeta` } for each of the 'containers' - declared by this charm in the `matadata.yaml` file. - requires: A dict of {name: :class:`RelationMeta` } for each 'requires' relation. - provides: A dict of {name: :class:`RelationMeta` } for each 'provides' relation. - peers: A dict of {name: :class:`RelationMeta` } for each 'peer' relation. - relations: A dict containing all :class:`RelationMeta` attributes (merged from other - sections) - storages: A dict of {name: :class:`StorageMeta`} for each defined storage. - resources: A dict of {name: :class:`ResourceMeta`} for each defined resource. - payloads: A dict of {name: :class:`PayloadMeta`} for each defined payload. - extra_bindings: A dict of additional named bindings that a charm can use - for network configuration. - actions: A dict of {name: :class:`ActionMeta`} for actions that the charm has defined. Args: raw: a mapping containing the contents of metadata.yaml actions_raw: a mapping containing the contents of actions.yaml + """ + + name: str + """Name of this charm.""" + + summary: str + """Short description of what this charm does.""" + description: str + """Long description for this charm.""" + + maintainers: List[str] + """List of email addresses of charm maintainers.""" + + tags: List[str] + """Charmhub tag metadata for categories associated with this charm.""" + + terms: List[str] + """Charmhub terms that should be agreed to before this charm can be deployed.""" + + series: List[str] + """List of supported OS series that this charm can support. + + The first entry in the list is the default series that will be used by + deploy if no other series is requested by the user. """ + subordinate: bool + """Whether this charm is intended to be used as a subordinate charm.""" + + min_juju_version: Optional[str] + """Indicates the minimum Juju version this charm requires.""" + + containers: Dict[str, 'ContainerMeta'] + """Container metadata for each defined container.""" + + requires: Dict[str, 'RelationMeta'] + """Relations this charm requires.""" + + provides: Dict[str, 'RelationMeta'] + """Relations this charm provides.""" + + peers: Dict[str, 'RelationMeta'] + """Peer relations.""" + + relations: Dict[str, 'RelationMeta'] + """All :class:`RelationMeta` instances. + + This is merged from ``requires``, ``provides``, and ``peers``. If needed, + the role of the relation definition can be obtained from its + :attr:`role ` attribute. + """ + + storages: Dict[str, 'StorageMeta'] + """Storage metadata for each defined storage.""" + + resources: Dict[str, 'ResourceMeta'] + """Resource metadata for each defined resource.""" + + payloads: Dict[str, 'PayloadMeta'] + """Payload metadata for each defined payload.""" + + extra_bindings: Dict[str, None] + """Additional named bindings that a charm can use for network configuration.""" + + actions: Dict[str, 'ActionMeta'] + """Actions the charm has defined.""" + def __init__(self, raw: Optional[Dict[str, Any]] = None, actions_raw: Optional[Dict[str, Any]] = None): raw_: Dict[str, Any] = raw or {} @@ -1085,12 +1109,12 @@ def __init__(self, raw: Optional[Dict[str, Any]] = None, def from_yaml( cls, metadata: Union[str, TextIO], actions: Optional[Union[str, TextIO]] = None) -> 'CharmMeta': - """Instantiate a CharmMeta from a YAML description of metadata.yaml. + """Instantiate a :class:`CharmMeta` from a YAML description of ``metadata.yaml``. Args: metadata: A YAML description of charm metadata (name, relations, etc.) - This can be a simple string, or a file-like object. (passed to `yaml.safe_load`). - actions: YAML description of Actions for this charm (eg actions.yaml) + This can be a simple string, or a file-like object (passed to ``yaml.safe_load``). + actions: YAML description of Actions for this charm (e.g., actions.yaml) """ meta = yaml.safe_load(metadata) raw_actions = {} From dea55e5b971d8be9465a8ccf6ff374cf6deeb287 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 15 Jun 2023 15:34:08 +1200 Subject: [PATCH 09/20] Fix linting --- ops/charm.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index a0a97a104..5000f7ce0 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -858,7 +858,9 @@ class CharmEvents(ObjectEvents): """Triggered when a unit is about to be terminated (see :class:`RemoveEvent`).""" update_status = EventSource(UpdateStatusEvent) - """Triggered periodically by a status update request from Juju (see :class:`UpdateStatusEvent`).""" + """Triggered periodically by a status update request from Juju (see + :class:`UpdateStatusEvent`). + """ config_changed = EventSource(ConfigChangedEvent) """Triggered when a configuration change occurs (see :class:`ConfigChangedEvent`).""" @@ -876,22 +878,27 @@ class CharmEvents(ObjectEvents): """Triggered when a new leader has been elected (see :class:`LeaderElectedEvent`).""" leader_settings_changed = EventSource(LeaderSettingsChangedEvent) - """DEPRECATED. Triggered when leader changes any settings (see :class:`LeaderSettingsChangedEvent`).""" + """DEPRECATED. Triggered when leader changes any settings (see + :class:`LeaderSettingsChangedEvent`).""" collect_metrics = EventSource(CollectMetricsEvent) """Triggered by Juju to collect metrics (see :class:`CollectMetricsEvent`).""" secret_changed = EventSource(SecretChangedEvent) - """Triggered by Juju on the observer when the secret owner changes its contents (see :class:`SecretChangedEvent`).""" + """Triggered by Juju on the observer when the secret owner changes its contents (see + :class:`SecretChangedEvent`).""" secret_expired = EventSource(SecretExpiredEvent) - """Triggered by Juju on the owner when a secret's expiration time elapses (see :class:`SecretExpiredEvent`).""" + """Triggered by Juju on the owner when a secret's expiration time elapses (see + :class:`SecretExpiredEvent`).""" secret_rotate = EventSource(SecretRotateEvent) - """Triggered by Juju on the owner when the secret's rotation policy elapses (see :class:`SecretRotateEvent`).""" + """Triggered by Juju on the owner when the secret's rotation policy elapses (see + :class:`SecretRotateEvent`).""" secret_remove = EventSource(SecretRemoveEvent) - """Triggered by Juju on the owner when a secret revision can be removed (see :class:`SecretRemoveEvent`).""" + """Triggered by Juju on the owner when a secret revision can be removed (see + :class:`SecretRemoveEvent`).""" class CharmBase(Object): @@ -1017,7 +1024,7 @@ class is mostly for the framework to understand what the charm has defined. series: List[str] """List of supported OS series that this charm can support. - + The first entry in the list is the default series that will be used by deploy if no other series is requested by the user. """ @@ -1042,7 +1049,7 @@ class is mostly for the framework to understand what the charm has defined. relations: Dict[str, 'RelationMeta'] """All :class:`RelationMeta` instances. - + This is merged from ``requires``, ``provides``, and ``peers``. If needed, the role of the relation definition can be obtained from its :attr:`role ` attribute. From ee910f1790905fcd6224c5b836b26d10f1fb4f57 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Fri, 16 Jun 2023 17:06:34 +1200 Subject: [PATCH 10/20] Bit more docstring and type cleanup, particularly model.Container --- ops/charm.py | 26 ++++++------- ops/framework.py | 4 +- ops/model.py | 96 ++++++++++++++++++++++++------------------------ ops/pebble.py | 11 ++++-- 4 files changed, 69 insertions(+), 68 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 5000f7ce0..d93f22721 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -225,21 +225,21 @@ class RemoveEvent(HookEvent): class ConfigChangedEvent(HookEvent): """Event triggered when a configuration change occurs. - This event can fire in several situations: + This event will fire in several situations: + - When the admin reconfigures the charm using the Juju CLI, for example + ``juju config mycharm foo=bar``. This event notifies the charm of + its new configuration. (The event itself, however, is not aware of *what* + specifically has changed in the config). - Right after the unit starts up for the first time. This event notifies the charm of its initial configuration. - Typically, this event will fire between a :class:`install ` - and a :class:`starts ` during the startup sequence - (when you first deploy a unit), but more in general it will fire whenever - the unit is (re)started, e.g. after pod churn on kubernetes, on unit - rescheduling, on unit upgrade/refresh, etc... + Typically, this event will fire between an :class:`install ` + and a :class:`start ` during the startup sequence + (when you first deploy a unit), but in general it will fire whenever + the unit is (re)started, for example after pod churn on Kubernetes, on unit + rescheduling, on unit upgrade or refresh, and so on. - As a specific instance of the above point: when networking changes (if the machine reboots and comes up with a different IP). - - When the cloud admin reconfigures the charm via the Juju CLI, i.e. - `juju config my-charm foo=bar`. This event notifies the charm of - its new configuration. (The event itself, however, is not aware of *what* - specifically has changed in the config). Any callback method bound to this event cannot assume that the software has already been started; it should not start stopped @@ -347,10 +347,8 @@ def add_metrics(self, metrics: Mapping[str, Union[int, float]], """Record metrics that have been gathered by the charm for this unit. Args: - metrics: A collection of {key: float} pairs that contains the - metrics that have been gathered - labels: {key:value} strings that can be applied to the - metrics that are being gathered + metrics: Key-value mapping of metrics that have been gathered. + labels: Key-value labels applied to the metrics. """ self.framework.model._backend.add_metrics(metrics, labels) # type:ignore diff --git a/ops/framework.py b/ops/framework.py index 1415fe9ce..07767ce6a 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -506,11 +506,11 @@ class LifecycleEvent(EventBase): class PreCommitEvent(LifecycleEvent): - """Events that will be emitted first on commit.""" + """Event that will be emitted first on commit.""" class CommitEvent(LifecycleEvent): - """Events that will be emitted second on commit.""" + """Event that will be emitted second on commit.""" class FrameworkEvents(ObjectEvents): diff --git a/ops/model.py b/ops/model.py index f79241185..37b2b3987 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1294,7 +1294,7 @@ class RelationData(Mapping[Union['Unit', 'Application'], 'RelationDataContent']) they can read and write their application data. They are allowed to read remote unit and application data. - This class should not be created directly. It should be accessed via + This class should not be instantiated directly, instead use :attr:`Relation.data` """ @@ -1495,7 +1495,7 @@ def __repr__(self): class ConfigData(LazyMapping): """Configuration data. - This class should not be created directly. It should be accessed via :attr:`Model.config`. + This class should not be instantiated directly. It should be accessed via :attr:`Model.config`. """ def __init__(self, backend: '_ModelBackend'): @@ -1791,11 +1791,11 @@ class Container: This class should not be instantiated directly, instead use :meth:`Unit.get_container` or :attr:`Unit.containers`. - - Attributes: - name: The name of the container from metadata.yaml (eg, 'postgres'). """ + name: str + """The name of the container from ``metadata.yaml``, for example "postgres".""" + def __init__(self, name: str, backend: '_ModelBackend', pebble_client: Optional[pebble.Client] = None): self.name = name @@ -1808,14 +1808,11 @@ def __init__(self, name: str, backend: '_ModelBackend', def can_connect(self) -> bool: """Report whether the Pebble API is reachable in the container. - :meth:`can_connect` returns a bool that indicates whether the Pebble API is available at + This method returns a bool that indicates whether the Pebble API is available at the time the method is called. It does not guard against the Pebble API becoming - unavailable, and should be treated as a 'point in time' status only. - - If the Pebble API later fails, serious consideration should be given as to the reason for - this. + unavailable, and should be treated as a "point in time" status only. - Example:: + For example:: container = self.unit.get_container("example") if container.can_connect(): @@ -1827,9 +1824,6 @@ def can_connect(self) -> bool: event.defer() """ try: - # TODO: This call to `get_system_info` should be replaced with a call to a more - # appropriate endpoint that has stronger connotations of what constitutes a Pebble - # instance that is in fact 'ready'. self._pebble.get_system_info() except pebble.ConnectionError as e: logger.debug("Pebble API is not ready; ConnectionError: %s", e) @@ -1847,7 +1841,7 @@ def can_connect(self) -> bool: return True def autostart(self): - """Autostart all services marked as startup: enabled.""" + """Autostart all services marked as ``startup: enabled``.""" self._pebble.autostart_services() def replan(self): @@ -1943,7 +1937,7 @@ def get_checks( check_names: Optional check names to query for. If no check names are specified, return checks with any name. level: Optional check level to query for. If not specified, fetch - checks with any level. + all checks. """ checks = self._pebble.get_checks(names=check_names or None, level=level) return CheckInfoMapping(checks) @@ -1951,7 +1945,7 @@ def get_checks( def get_check(self, check_name: str) -> pebble.CheckInfo: """Get check information for a single named check. - Raises :class:`ModelError` if check_name is not found. + Raises :class:`ModelError` if ``check_name`` is not found. """ checks = self.get_checks(check_name) if not checks: @@ -1966,13 +1960,13 @@ def pull(self, path: Union[str, PurePath], *, Args: path: Path of the file to read from the remote system. - encoding: Encoding to use for decoding the file's bytes to str, - or None to specify no decoding. + encoding: Encoding to use for decoding the file's bytes to string, + or ``None`` to specify no decoding. Returns: - A readable file-like object, whose read() method will return str - objects decoded according to the specified encoding, or bytes if - encoding is None. + A readable file-like object, whose ``read()`` method will return + strings decoded according to the specified encoding, or bytes if + encoding is ``None``. Raises: pebble.PathError: If there was an error reading the file at path, @@ -2042,13 +2036,13 @@ def push_path(self, Only regular files and directories are copied; symbolic links, device files, etc. are skipped. Pushing is attempted to completion even if errors occur during the process. All errors are collected incrementally. After copying has completed, if any errors occurred, a - single MultiPushPullError is raised containing details for each error. + single :class:`MultiPushPullError` is raised containing details for each error. Assuming the following files exist locally: - * /foo/bar/baz.txt - * /foo/foobar.txt - * /quux.txt + * /foo/bar/baz.txt + * /foo/foobar.txt + * /quux.txt You could push the following ways:: @@ -2073,13 +2067,15 @@ def push_path(self, # Destination results: /dst/bar/baz.txt, /dst/quux.txt Args: - source_path: A single path or list of paths to push to the remote system. The - paths can be either a file or a directory. If source_path is a directory, the - directory base name is attached to the destination directory - i.e. the source path - directory is placed inside the destination directory. If a source path ends with a - trailing "/*" it will have its *contents* placed inside the destination directory. - dest_dir: Remote destination directory inside which the source dir/files will be - placed. This must be an absolute path. + source_path: A single path or list of paths to push to the remote + system. The paths can be either a file or a directory. If + ``source_path`` is a directory, the directory base name is + attached to the destination directory -- that is, the source + path directory is placed inside the destination directory. If + a source path ends with a trailing ``/*`` it will have its + *contents* placed inside the destination directory. + dest_dir: Remote destination directory inside which the source + dir/files will be placed. This must be an absolute path. """ if hasattr(source_path, '__iter__') and not isinstance(source_path, str): source_paths = typing.cast(Iterable[Union[str, Path]], source_path) @@ -2121,13 +2117,13 @@ def pull_path(self, Only regular files and directories are copied; symbolic links, device files, etc. are skipped. Pulling is attempted to completion even if errors occur during the process. All errors are collected incrementally. After copying has completed, if any errors occurred, a - single MultiPushPullError is raised containing details for each error. + single :class:`MultiPushPullError` is raised containing details for each error. Assuming the following files exist remotely: - * /foo/bar/baz.txt - * /foo/foobar.txt - * /quux.txt + * /foo/bar/baz.txt + * /foo/foobar.txt + * /quux.txt You could pull the following ways:: @@ -2152,14 +2148,16 @@ def pull_path(self, # Destination results: /dst/bar/baz.txt, /dst/quux.txt Args: - source_path: A single path or list of paths to pull from the remote system. The - paths can be either a file or a directory but must be absolute paths. If - source_path is a directory, the directory base name is attached to the destination - directory - i.e. the source path directory is placed inside the destination - directory. If a source path ends with a trailing "/*" it will have its *contents* - placed inside the destination directory. - dest_dir: Local destination directory inside which the source dir/files will be - placed. + source_path: A single path or list of paths to pull from the + remote system. The paths can be either a file or a directory + but must be absolute paths. If ``source_path`` is a directory, + the directory base name is attached to the destination + directory -- that is, the source path directory is placed + inside the destination directory. If a source path ends with a + trailing ``/*`` it will have its *contents* placed inside the + destination directory. + dest_dir: Local destination directory inside which the source + dir/files will be placed. """ if hasattr(source_path, '__iter__') and not isinstance(source_path, str): source_paths = typing.cast(Iterable[Union[str, Path]], source_path) @@ -2260,7 +2258,7 @@ def _build_destpath( return dest_dir / path_suffix def exists(self, path: Union[str, PurePath]) -> bool: - """Return true if the path exists on the container filesystem.""" + """Report whether a path exists on the container filesystem.""" try: self._pebble.list_files(str(path), itself=True) except pebble.APIError as err: @@ -2270,7 +2268,7 @@ def exists(self, path: Union[str, PurePath]) -> bool: return True def isdir(self, path: Union[str, PurePath]) -> bool: - """Return true if a directory exists at the given path on the container filesystem.""" + """Report whether a directory exists at the given path on the container filesystem.""" try: files = self._pebble.list_files(str(path), itself=True) except pebble.APIError as err: @@ -2401,8 +2399,8 @@ def send_signal(self, sig: Union[int, str], *service_names: str): """Send the given signal to one or more services. Args: - sig: Name or number of signal to send, e.g., "SIGHUP", 1, or - signal.SIGHUP. + sig: Name or number of signal to send, for example ``"SIGHUP"``, ``1``, or + ``signal.SIGHUP``. service_names: Name(s) of the service(s) to send the signal to. Raises: diff --git a/ops/pebble.py b/ops/pebble.py index 21ad199b1..f16dd0afe 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -2106,6 +2106,11 @@ def exec( ) -> ExecProcess[Any]: r"""Execute the given command on the remote system. + Two method signatures are shown because this method returns an + :class:`ExecProcess` that deals with strings if ``encoding`` is + specified (the default ), or one that deals with bytes if ``encoding`` + is set to ``None``. + Most of the parameters are explained in the "Parameters" section below, however, input/output handling is a bit more complex. Some examples are shown below:: @@ -2344,8 +2349,8 @@ def send_signal(self, sig: Union[int, str], services: Iterable[str]): """Send the given signal to the list of services named. Args: - sig: Name or number of signal to send, e.g., "SIGHUP", 1, or - signal.SIGHUP. + sig: Name or number of signal to send, for example ``"SIGHUP"``, ``1``, or + ``signal.SIGHUP``. services: Non-empty list of service names to send the signal to. Raises: @@ -2378,7 +2383,7 @@ def get_checks( level: Optional check level to query for (default is to fetch checks with any level). names: Optional list of check names to query for (default is to - fetch checks with any name). + fetch all checks). Returns: List of :class:`CheckInfo` objects. From f73dcb1b05fbacc26cbcb7f078fd6d856a5d8681 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Mon, 19 Jun 2023 11:31:10 +1200 Subject: [PATCH 11/20] Bunch more improvements; up to PebbleReadyEvent --- ops/charm.py | 42 +++++++++++++++------------- ops/framework.py | 39 ++++++++++++++++---------- ops/jujuversion.py | 23 +++++++-------- ops/model.py | 70 +++++++++++++++++++++++++++++++--------------- 4 files changed, 105 insertions(+), 69 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index d93f22721..9a71affb1 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -321,9 +321,7 @@ class LeaderElectedEvent(HookEvent): class LeaderSettingsChangedEvent(HookEvent): - """Event triggered when leader changes any settings. - - DEPRECATED NOTICE + """DEPRECATED. Event triggered when leader changes any settings. This event has been deprecated in favor of using a Peer relation, and having the leader set a value in the Application data bag for @@ -1234,12 +1232,13 @@ def __init__(self, name: str, raw: '_ResourceMetaDict'): class PayloadMeta: - """Object containing metadata about a payload definition. + """Object containing metadata about a payload definition.""" - Attributes: - payload_name: Name of payload - type: Payload type - """ + payload_name: str + """Name of the payload.""" + + type: str + """Payload type.""" def __init__(self, name: str, raw: Dict[str, Any]): self.payload_name = name @@ -1263,11 +1262,11 @@ class ContainerMeta: NOTE: this is extremely lightweight right now, and just includes the fields we need for Pebble interaction. - - Attributes: - name: Name of container (key in the YAML) """ + name: str + """Name of the container (key in the YAML).""" + def __init__(self, name: str, raw: Dict[str, Any]): self.name = name self._mounts: Dict[str, ContainerStorageMeta] = {} @@ -1319,22 +1318,25 @@ def _populate_mounts(self, mounts: List['_MountDict']): class ContainerStorageMeta: """Metadata about storage for an individual container. - Attributes: - storage: a name for the mountpoint, which should exist the keys for :class:`StorageMeta` - for the charm - location: the location `storage` is mounted at - If multiple locations are specified for the same storage, such as Kubernetes subPath mounts, - `location` will not be an accessible attribute, as it would not be possible to determine - which mount point was desired, and `locations` should be iterated over. + ``location`` will not be an accessible attribute, as it would not be possible to determine + which mount point was desired, and ``locations`` should be iterated over. """ + storage: str + """Name for the mount point, which should exist in the keys of the charm's + :class:`StorageMeta`. + """ + + location: str + """The location the storage is mounted at.""" + def __init__(self, storage: str, location: str): self.storage = storage self._locations: List[str] = [location] def add_location(self, location: str): - """Add an additional mountpoint to a known storage.""" + """Add an additional mount point to a known storage.""" self._locations.append(location) @property @@ -1349,7 +1351,7 @@ def __getattr__(self, name: str): return self._locations[0] else: raise RuntimeError( - "container has more than one mountpoint with the same backing storage. " + "container has more than one mount point with the same backing storage. " "Request .locations to see a list" ) else: diff --git a/ops/framework.py b/ops/framework.py index 07767ce6a..23395d3d8 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -174,9 +174,10 @@ def from_path(cls, path: str) -> 'Handle': class EventBase: - """The base for all the different Events. + """The base class for all events. - Inherit this and override 'snapshot' and 'restore' methods to build a custom event. + Inherit this and override the ``snapshot`` and ``restore`` methods to + create a custom event. """ # gets patched in by `Framework.restore()`, if this event is being re-emitted @@ -184,6 +185,7 @@ class EventBase: # event is being fired for the first time. # TODO this is hard to debug, this should be refactored framework: 'Framework' = None # type: ignore + """The :class:`Framework` instance (set by the framework itself).""" def __init__(self, handle: Handle): self.handle = handle @@ -408,7 +410,7 @@ def model(self) -> 'Model': class ObjectEvents(Object): - """Convenience type to allow defining .on attributes at class level.""" + """Convenience type to allow defining ``.on`` attributes at class level.""" handle_kind = "on" @@ -432,23 +434,21 @@ def __get__(self, emitter: Object, emitter_type: 'Type[Object]'): def define_event(cls, event_kind: str, event_type: 'Type[EventBase]'): """Define an event on this type at runtime. - cls: a type to define an event on. - - event_kind: an attribute name that will be used to access the - event. Must be a valid python identifier, not be a keyword - or an existing attribute. - - event_type: a type of the event to define. - Note that attempting to define the same event kind more than once will - raise a 'overlaps with existing type' runtime error. Ops uses a + raise an "overlaps with existing type" runtime error. Ops uses a labeling system to track and reconstruct events between hook executions (each time a hook runs, the Juju Agent invokes a fresh instance of ops; there is no ops process that persists on the host between hooks). Having duplicate Python objects creates duplicate labels. Overwriting a previously created label means that only the latter code path will be - run when the current event, if it does get deferred, is reemitted. This - is usually not what is desired, and is error-prone and ambigous. + run when the current event, if it does get deferred, is re-emitted. This + is usually not what is desired, and is error-prone and ambiguous. + + Args: + event_kind: An attribute name that will be used to access the + event. Must be a valid Python identifier, not be a keyword + or an existing attribute. + event_type: A type of the event to define. """ prefix = 'unable to define an event with event_kind that ' if not event_kind.isidentifier(): @@ -515,8 +515,12 @@ class CommitEvent(LifecycleEvent): class FrameworkEvents(ObjectEvents): """Manager of all framework events.""" + pre_commit = EventSource(PreCommitEvent) + """Triggered before the :attr:`commit` event.""" + commit = EventSource(CommitEvent) + """Triggered before event data is committed to storage.""" class NoTypeError(Exception): @@ -545,14 +549,19 @@ class Framework(Object): """Main interface from the Charm to the Operator Framework internals.""" on = FrameworkEvents() # type: ignore + """Used for :meth:`observe`-ing framework-specific events.""" # Override properties from Object so that we can set them in __init__. model: 'Model' = None # type: ignore + """The :class:`Model` instance for this charm.""" + meta: 'CharmMeta' = None # type: ignore + """The charm's metadata.""" + charm_dir: 'pathlib.Path' = None # type: ignore + """The top-level directory of the charm.""" # to help the type checker and IDEs: - if TYPE_CHECKING: _stored: 'StoredStateData' = None # type: ignore @property diff --git a/ops/jujuversion.py b/ops/jujuversion.py index a042222d8..c5d075c4a 100755 --- a/ops/jujuversion.py +++ b/ops/jujuversion.py @@ -24,19 +24,20 @@ class JujuVersion: """Helper to work with the Juju version. - It knows how to parse the ``JUJU_VERSION`` environment variable, and exposes different - capabilities according to the specific version, allowing also to compare with other - versions. + It knows how to parse the ``JUJU_VERSION`` environment variable, and + exposes different capabilities according to the specific version. It also + allows users to compare ``JujuVersion`` instances with ``<`` and ``>`` + operators. """ - PATTERN = r'''^ + _pattern_re = re.compile(r'''^ (?P\d{1,9})\.(?P\d{1,9}) # and numbers are always there ((?:\.|-(?P[a-z]+))(?P\d{1,9}))? # sometimes with . or - (\.(?P\d{1,9}))?$ # and sometimes with a number. - ''' + ''', re.VERBOSE) def __init__(self, version: str): - m = re.match(self.PATTERN, version, re.VERBOSE) + m = self._pattern_re.match(version) if not m: raise RuntimeError(f'"{version}" is not a valid Juju version string') @@ -95,27 +96,27 @@ def __lt__(self, other: 'JujuVersion') -> bool: @classmethod def from_environ(cls) -> 'JujuVersion': - """Build a JujuVersion from JUJU_VERSION.""" + """Build a version from the ``JUJU_VERSION`` environment variable.""" v = os.environ.get('JUJU_VERSION') if v is None: v = '0.0.0' return cls(v) def has_app_data(self) -> bool: - """Determine whether this Juju version knows about app data.""" + """Report whether this Juju version supports app data.""" return (self.major, self.minor, self.patch) >= (2, 7, 0) def is_dispatch_aware(self) -> bool: - """Determine whether this Juju version knows about dispatch.""" + """Report whether this Juju version supports dispatch.""" return (self.major, self.minor, self.patch) >= (2, 8, 0) def has_controller_storage(self) -> bool: - """Determine whether this Juju version supports controller-side storage.""" + """Report whether this Juju version supports controller-side storage.""" return (self.major, self.minor, self.patch) >= (2, 8, 0) @property def has_secrets(self) -> bool: - """Determine whether this Juju version supports the `secrets` feature.""" + """Report whether this Juju version supports the "secrets" feature.""" # Juju version 3.0.0 had an initial version of secrets, but: # * In 3.0.2, secret-get "--update" was renamed to "--refresh", and # secret-get-info was separated into its own hook tool diff --git a/ops/model.py b/ops/model.py index 37b2b3987..d30cb64f0 100644 --- a/ops/model.py +++ b/ops/model.py @@ -103,8 +103,8 @@ class Model: """Represents the Juju Model as seen from this unit. - This should not be instantiated directly by Charmers, but can be accessed as `self.model` - from any class that derives from Object. + This should not be instantiated directly by Charmers, but can be accessed + as ``self.model`` from any class that derives from :class:`Object`. """ def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'): @@ -123,12 +123,18 @@ def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'): @property def unit(self) -> 'Unit': - """A :class:`Unit` that represents the unit that is running this code (eg yourself).""" + """The unit that is running this code (that is, yourself). + + Use :meth:`get_unit` to get an arbitrary unit by name. + """ return self._unit @property def app(self) -> 'Application': - """A :class:`Application` that represents the application this unit is a part of.""" + """The application this unit is a part of. + + Use :meth:`get_app` to get an arbitrary application by name. + """ return self._unit.app @property @@ -161,7 +167,13 @@ def storages(self) -> 'StorageMapping': @property def pod(self) -> 'Pod': - """Use ``model.pod.set_spec`` to set the container specification for Kubernetes charms.""" + """Represents the definition of a pod spec in legacy Kubernetes models. + + DEPRECATED: New charms should use the sidecar pattern with Pebble. + + Use :meth:`Pod.set_spec` to set the container specification for legacy + Kubernetes charms. + """ return self._pod @property @@ -183,6 +195,8 @@ def uuid(self) -> str: def get_unit(self, unit_name: str) -> 'Unit': """Get an arbitrary unit by name. + Use :attr:`unit` to get your own unit. + Internally this uses a cache, so asking for the same unit two times will return the same object. """ @@ -191,6 +205,8 @@ def get_unit(self, unit_name: str) -> 'Unit': def get_app(self, app_name: str) -> 'Application': """Get an application by name. + Use :attr:`app` to get your own application. + Internally this uses a cache, so asking for the same application two times will return the same object. """ @@ -618,11 +634,11 @@ def opened_ports(self) -> Set['OpenedPort']: class OpenedPort: """Represents a port opened by :meth:`Unit.open_port`.""" - """The IP protocol: 'tcp', 'udp', or 'icmp'.""" protocol: typing.Literal['tcp', 'udp', 'icmp'] + """The IP protocol.""" - """The port number. Will be None if protocol is 'icmp'.""" port: Optional[int] + """The port number. Will be ``None`` if protocol is ``'icmp'``.""" class LazyMapping(Mapping[str, str], ABC): @@ -837,15 +853,15 @@ class Network: [NetworkInfo('ens1', '10.1.1.1/32'), NetworkInfo('ens1', '10.1.2.1/32']) """ - """A list of IP addresses that other units should use to get in touch with you.""" ingress_addresses: List[Union[ipaddress.IPv4Address, ipaddress.IPv6Address, str]] + """A list of IP addresses that other units should use to get in touch with you.""" + egress_subnets: List[Union[ipaddress.IPv4Network, ipaddress.IPv6Network]] """A list of networks representing the subnets that other units will see you connecting from. Due to things like NAT it isn't always possible to narrow it down to a single address, but when it is clear, the CIDRs will be constrained to a single address (for example, 10.0.0.1/32). """ - egress_subnets: List[Union[ipaddress.IPv4Network, ipaddress.IPv6Network]] def __init__(self, network_info: '_NetworkDict'): """Initialize a Network instance. @@ -885,8 +901,8 @@ def bind_address(self) -> Optional[Union[ipaddress.IPv4Address, ipaddress.IPv6Ad return None @property - def ingress_address( - self) -> Optional[Union[ipaddress.IPv4Address, ipaddress.IPv6Address, str]]: + def ingress_address(self) -> Optional[ + Union[ipaddress.IPv4Address, ipaddress.IPv6Address, str]]: """The address other applications should use to connect to your unit. Due to things like public/private addresses, NAT and tunneling, the address you bind() @@ -1642,9 +1658,12 @@ def fetch(self, name: str) -> Path: class Pod: - """Represents the definition of a pod spec in Kubernetes models. + """Represents the definition of a pod spec in legacy Kubernetes models. - Currently only supports simple access to setting the Juju pod spec via :attr:`.set_spec`. + DEPRECATED: New charms should use the sidecar pattern with Pebble. + + Currently only supports simple access to setting the Juju pod spec via + :attr:`.set_spec`. """ def __init__(self, backend: '_ModelBackend'): @@ -1736,7 +1755,7 @@ def index(self) -> int: @property def id(self) -> int: - """Deprecated -- use :attr:`Storage.index` instead.""" + """DEPRECATED. Use :attr:`Storage.index` instead.""" logger.warning("model.Storage.id is being replaced - please use model.Storage.index") return self.index @@ -1764,26 +1783,31 @@ def location(self, location: str) -> None: class MultiPushPullError(Exception): - """Aggregates multiple push/pull related exceptions into one. + """Aggregates multiple push and pull exceptions into one. + + This class should not be instantiated directly. It is raised by + :meth:`Container.push_path` and :meth:`Container.pull_path`. + """ - This class should not be instantiated directly. + message: str + """The error message.""" - Attributes: - message: error message - errors: list of errors with each represented by a tuple (,) - where source_path is the path being pushed/pulled from. + errors: List[Tuple[str, Exception]] + """The list of errors. + + Each error is represented by a tuple of (, ), + where source_path is the path being pushed to or pulled from. """ def __init__(self, message: str, errors: List[Tuple[str, Exception]]): - """Create an aggregation of several push/pull errors.""" - self.errors = errors self.message = message + self.errors = errors def __str__(self): return f'{self.message} ({len(self.errors)} errors): {self.errors[0][1]}, ...' def __repr__(self): - return f'MultiError({self.message!r}, {len(self.errors)} errors)' + return f'MultiPushPullError({self.message!r}, {len(self.errors)} errors)' class Container: From b8c03b348eb9e96def78bbf0d2ba48b0b93fd616 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 20 Jun 2023 08:39:35 +1200 Subject: [PATCH 12/20] Tweak charm_dir docstring per review feedback --- ops/framework.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ops/framework.py b/ops/framework.py index 23395d3d8..40a4c94ca 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -559,7 +559,7 @@ class Framework(Object): """The charm's metadata.""" charm_dir: 'pathlib.Path' = None # type: ignore - """The top-level directory of the charm.""" + """The charm project root directory.""" # to help the type checker and IDEs: if TYPE_CHECKING: From a58a89fd82b794923e2e14c83dc19b4e0a8a72d0 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 20 Jun 2023 17:17:35 +1200 Subject: [PATCH 13/20] Various docstring and type tweaks --- ops/charm.py | 24 ++++++++++++++---------- ops/model.py | 51 ++++++++++++++++++++++++++++++--------------------- 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 9a71affb1..4af7f5cc8 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -279,8 +279,8 @@ class UpgradeCharmEvent(HookEvent): class PreSeriesUpgradeEvent(HookEvent): """Event triggered to prepare a unit for series upgrade. - This event triggers when an administrator executes ``juju upgrade-series - MACHINE prepare``. The event will fire for each unit that is running on the + This event triggers when an administrator executes ``juju upgrade-machine + prepare``. The event will fire for each unit that is running on the specified machine. Any callback method bound to this event must prepare the charm for an upgrade to the series. This may include things like exporting database content to a version neutral format, or evacuating running @@ -298,7 +298,7 @@ class PostSeriesUpgradeEvent(HookEvent): This event is triggered after the administrator has done a distribution upgrade (or rolled back and kept the same series). It is called in response - to ``juju upgrade-series MACHINE complete``. Associated charm callback + to ``juju upgrade-machine complete``. Associated charm callback methods are expected to do whatever steps are necessary to reconfigure their applications for the new series. This may include things like populating the upgraded version of a database. Note however charms are expected to check if @@ -360,15 +360,19 @@ class RelationEvent(HookEvent): "stopped". Within that time window, the unit may participate in several different relations at a time, including multiple relations with the same name. + """ - Attributes: - relation: The :class:`~ops.model.Relation` involved in this event - app: The remote :class:`~ops.model.Application` that has triggered this - event - unit: The remote :class:`~ops.model.Unit` that has triggered this event. This may be - ``None`` if the relation event was triggered as an - :class:`~ops.model.Application` level event + relation: 'Relation' + """The relation involved in this event.""" + + app: model.Application + """The remote application that has triggered this event.""" + + unit: model.Unit + """The remote unit that has triggered this event. + This will be ``None`` if the relation event was triggered as an + :class:`Application `-level event. """ def __init__(self, handle: 'Handle', relation: 'Relation', diff --git a/ops/model.py b/ops/model.py index d30cb64f0..1fa54e1a3 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1248,19 +1248,33 @@ def remove_all_revisions(self): class Relation: """Represents an established relation between this application and another application. - This class should not be instantiated directly, instead use :meth:`Model.get_relation` - or :attr:`ops.RelationEvent.relation`. This is principally used by + This class should not be instantiated directly, instead use :meth:`Model.get_relation`, + :attr:`Model.relations`, or :attr:`ops.RelationEvent.relation`. This is principally used by :class:`ops.RelationMeta` to represent the relationships between charms. + """ - Attributes: - name: The name of the local endpoint of the relation (eg 'db') - id: The identifier for a particular relation (integer) - app: An :class:`Application` representing the remote application of this relation. - For peer relations this will be the local application. - units: A set of :class:`Unit` for units that have started and joined this relation. - For subordinate relations, this set will include only one unit: the principal unit. - data: A :class:`RelationData` holding the data buckets for each entity - of a relation. Accessed via eg Relation.data[unit]['foo'] + name: str + """The name of the local endpoint of the relation (for example, 'db').""" + + id: int + """The identifier for a particular relation.""" + + app: Optional[Application] + """Represents the remote application of this relation. + + For peer relations, this will be the local application. + """ + + units: Set[Unit] + """A set of units that have started and joined this relation. + + For subordinate relations, this set will include only one unit: the principal unit. + """ + + data: 'RelationData' + """Holds the data buckets for each entity of a relation. + + This is accessed using, for example, ``Relation.data[unit]['foo']``. """ def __init__( @@ -1303,8 +1317,8 @@ class RelationData(Mapping[Union['Unit', 'Application'], 'RelationDataContent']) """Represents the various data buckets of a given relation. Each unit and application involved in a relation has their own data bucket. - Eg: ``{entity: RelationDataContent}`` - where entity can be either a :class:`Unit` or a :class:`Application`. + For example, ``{entity: RelationDataContent}``, + where entity can be either a :class:`Unit` or an :class:`Application`. Units can read and write their own data, and if they are the leader, they can read and write their application data. They are allowed to read @@ -1672,14 +1686,11 @@ def __init__(self, backend: '_ModelBackend'): def set_spec(self, spec: 'K8sSpec', k8s_resources: Optional['K8sSpec'] = None): """Set the specification for pods that Juju should start in kubernetes. - See `juju help-tool pod-spec-set` for details of what should be passed. + See ``juju help-tool pod-spec-set`` for details of what should be passed. Args: spec: The mapping defining the pod specification k8s_resources: Additional kubernetes specific specification. - - Returns: - None """ if not self._backend.is_leader(): raise ModelError('cannot set a pod spec as this unit is not a leader') @@ -2532,10 +2543,8 @@ class RelationDataError(ModelError): """Raised when a relation data read/write is invalid. This is raised if you're either trying to set a value to something that isn't a string, - or if you are trying to set a value in a bucket that you don't have access to. (eg, - another application/unit or setting your application data but you aren't the leader.) - Also raised when you attempt to read a databag you don't have access to - (i.e. a local app databag if you're not the leader). + or if you are trying to set a value in a bucket that you don't have access to. (For example, + another application/unit, or setting your application data without being the leader.) """ From 67f722abcb10a08dbc1e7645f886cce0bbf96f8e Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 20 Jun 2023 17:31:36 +1200 Subject: [PATCH 14/20] Fix linting errors --- ops/charm.py | 6 +++--- ops/model.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 4af7f5cc8..f8a99c70d 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -365,10 +365,10 @@ class RelationEvent(HookEvent): relation: 'Relation' """The relation involved in this event.""" - app: model.Application + app: Optional[model.Application] """The remote application that has triggered this event.""" - - unit: model.Unit + + unit: Optional[model.Unit] """The remote unit that has triggered this event. This will be ``None`` if the relation event was triggered as an diff --git a/ops/model.py b/ops/model.py index 1fa54e1a3..c254453a2 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1261,7 +1261,7 @@ class Relation: app: Optional[Application] """Represents the remote application of this relation. - + For peer relations, this will be the local application. """ @@ -1273,7 +1273,7 @@ class Relation: data: 'RelationData' """Holds the data buckets for each entity of a relation. - + This is accessed using, for example, ``Relation.data[unit]['foo']``. """ From b55d08476bd5ef93b263ce33fbb2c2e903dee39e Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Mon, 26 Jun 2023 13:56:45 +1200 Subject: [PATCH 15/20] Various docstring fixes --- ops/charm.py | 62 +++++++++++++++++++++++++++++++++++----------------- ops/model.py | 8 +++---- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index f8a99c70d..7c775aec5 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -22,6 +22,7 @@ Any, Dict, List, + Literal, Mapping, Optional, TextIO, @@ -34,7 +35,7 @@ from ops.framework import EventBase, EventSource, Framework, Object, ObjectEvents if TYPE_CHECKING: - from typing_extensions import Literal, Required, TypedDict + from typing_extensions import Required, TypedDict from ops.framework import Handle from ops.model import Container, Relation, Storage @@ -721,8 +722,8 @@ class SecretChangedEvent(SecretEvent): secret will be notified via this event that a new revision is available. Typically, you will want to fetch the new content by calling - :meth:`ops.Secret.get_content` with :code:`refresh=True` to tell Juju to - start tracking the new revision. + :meth:`event.secret.get_content() ` with ``refresh=True`` + to tell Juju to start tracking the new revision. """ @@ -731,7 +732,7 @@ class SecretRotateEvent(SecretEvent): This event is fired on the secret owner to inform it that the secret must be rotated. The event will keep firing until the owner creates a new - revision by calling :meth:`ops.Secret.set_content`. + revision by calling :meth:`event.secret.set_content() `. """ def defer(self): @@ -748,7 +749,8 @@ class SecretRemoveEvent(SecretEvent): observers have updated to that new revision, this event will be fired to inform the secret owner that the old revision can be removed. - Typically, you will want to call :meth:`ops.Secret.remove_revision` to + Typically, you will want to call + :meth:`event.secret.remove_revision() ` to remove the now-unused revision. """ @@ -784,7 +786,7 @@ class SecretExpiredEvent(SecretEvent): This event is fired on the secret owner to inform it that the secret revision must be removed. The event will keep firing until the owner removes the - revision by calling :meth:`ops.Secret.remove_revision()`. + revision by calling :meth:`event.secret.remove_revision() `. """ def __init__(self, handle: 'Handle', id: str, label: Optional[str], revision: int): @@ -1146,9 +1148,9 @@ class RelationRole(enum.Enum): provides = 'provides' def is_peer(self) -> bool: - """Return whether the current role is peer. + """Report whether this role is 'peer'. - A convenience to avoid having to import charm. + ``role.is_peer()`` is a shortcut for ``role == ops.RelationRole.peer``. """ return self is RelationRole.peer @@ -1156,16 +1158,27 @@ def is_peer(self) -> bool: class RelationMeta: """Object containing metadata about a relation definition. - Should not be constructed directly by charm code. Is gotten from one of + Should not be constructed directly by charm code, but gotten from one of :attr:`CharmMeta.peers`, :attr:`CharmMeta.requires`, :attr:`CharmMeta.provides`, or :attr:`CharmMeta.relations`. + """ - Attributes: - role: This is :class:`RelationRole`; one of peer/requires/provides - relation_name: Name of this relation from metadata.yaml - interface_name: Optional definition of the interface protocol. - limit: Optional definition of maximum number of connections to this relation endpoint. - scope: "global" (default) or "container" scope based on how the relation should be used. + role: RelationRole + """Role this relation takes, one of 'peer', 'requires', or 'provides'.""" + + relation_name: str + """Name of this relation.""" + + interface_name: Optional[str] + """Definition of the interface protocol.""" + + limit: Optional[int] + """Maximum number of connections to this relation endpoint.""" + + scope: str + """Scope based on how this relation should be used. + + Will be either ``"global"`` or ``"container"``. """ VALID_SCOPES = ['global', 'container'] @@ -1220,12 +1233,21 @@ def __init__(self, name: str, raw: '_StorageMetaDict'): class ResourceMeta: - """Object containing metadata about a resource definition. + """Object containing metadata about a resource definition.""" - Attributes: - resource_name: Name of resource - filename: Name of file - description: A text description of resource + resource_name: str + """Name of the resource.""" + + type: str + """Type of the resource. One of ``"file"`` or ``"oci-image"``.""" + + filename: Optional[str] + """Filename of the resource file.""" + + description: str + """A description of the resource. + + This will be empty string (rather than None) if not set in ``metadata.yaml``. """ def __init__(self, name: str, raw: '_ResourceMetaDict'): diff --git a/ops/model.py b/ops/model.py index c254453a2..89cb3885e 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1661,8 +1661,8 @@ def __init__(self, names: Iterable[str], backend: '_ModelBackend'): def fetch(self, name: str) -> Path: """Fetch the resource from the controller or store. - If successfully fetched, this returns a Path object to where the resource is stored - on disk, otherwise it raises a NameError. + If successfully fetched, this returns the path where the resource is stored + on disk, otherwise it raises a :class:`NameError`. """ if name not in self._paths: raise NameError(f'invalid resource name: {name}') @@ -2478,7 +2478,7 @@ def __repr__(self): class ServiceInfoMapping(Mapping[str, pebble.ServiceInfo]): - """Map of service names to :class:`ops.pebble.ServiceInfo` objects. + """Map of service names to :class:`pebble.ServiceInfo` objects. This is done as a mapping object rather than a plain dictionary so that we can extend it later, and so it's not mutable. @@ -2561,7 +2561,7 @@ class RelationDataAccessError(RelationDataError): class RelationNotFoundError(ModelError): - """Backend error when querying juju for a given relation and that relation doesn't exist.""" + """Raised when querying Juju for a given relation and that relation doesn't exist.""" class InvalidStatusError(ModelError): From a7eb70f6fdb809be5b077d0f6f063a4f0f4cc39b Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 27 Jun 2023 15:00:58 +1200 Subject: [PATCH 16/20] A bunch more type and docstring fixes --- ops/charm.py | 45 ++++++++++++++++++++++++++++++--------------- ops/framework.py | 8 ++++---- ops/model.py | 44 +++++++++++++++++++++++++++----------------- 3 files changed, 61 insertions(+), 36 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 7c775aec5..a5dcdee4a 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -26,6 +26,7 @@ Mapping, Optional, TextIO, + Tuple, Union, cast, ) @@ -51,7 +52,7 @@ _MultipleRange = TypedDict('_MultipleRange', {'range': str}) _StorageMetaDict = TypedDict('_StorageMetaDict', { 'type': Required[str], - 'description': int, + 'description': str, 'shared': bool, 'read-only': bool, 'minimum-size': str, @@ -554,11 +555,11 @@ class StorageEvent(HookEvent): charms can define several different types of storage that are allocated from Juju. Changes in state of storage trigger sub-types of :class:`StorageEvent`. - - Attributes: - storage: The :class:`~ops.model.Storage` instance this event is about. """ + storage: 'Storage' + """Storage instance this event refers to.""" + def __init__(self, handle: 'Handle', storage: 'Storage'): super().__init__(handle) self.storage = storage @@ -586,7 +587,7 @@ def restore(self, snapshot: Dict[str, Any]): if storage_name and storage_index is not None: storages = self.framework.model.storages[storage_name] - self.storage = next((s for s in storages if s.index == storage_index), None,) + self.storage = next((s for s in storages if s.index == storage_index), None) # type: ignore # noqa if self.storage is None: msg = 'failed loading storage (name={!r}, index={!r}) from snapshot' \ .format(storage_name, storage_index) @@ -1202,17 +1203,31 @@ def __init__(self, role: RelationRole, relation_name: str, raw: '_RelationMetaDi class StorageMeta: - """Object containing metadata about a storage definition. + """Object containing metadata about a storage definition.""" - Attributes: - storage_name: Name of storage - type: Storage type - description: A text description of the storage - read_only: True if the storage is read-only - minimum_size: Minimum size of storage - location: Mount point of storage - multiple_range: Range of numeric qualifiers when multiple storage units are used - """ + storage_name: str + """Name of storage.""" + + type: str + """Storage type, "filesystem" or "block".""" + + description: str + """Text description of the storage.""" + + shared: bool + """True if all units of the application share the storage.""" + + read_only: bool + """True if the storage is read-only.""" + + minimum_size: Optional[str] + """Minimum size of the storage.""" + + location: Optional[str] + """Mount point of the storage.""" + + multiple_range: Optional[Tuple[int, Optional[int]]] + """Range of numeric qualifiers when multiple storage units are used.""" def __init__(self, name: str, raw: '_StorageMetaDict'): self.storage_name = name diff --git a/ops/framework.py b/ops/framework.py index 40a4c94ca..65c0acbbd 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -1083,16 +1083,16 @@ def set_default(self, **kwargs: Any): class StoredState: - """A class used to store data the charm needs persisted across invocations. + """A class used to store data the charm needs, persisted across invocations. Example:: class MyClass(Object): _stored = StoredState() - Instances of `MyClass` can transparently save state between invocations by - setting attributes on `_stored`. Initial state should be set with - `set_default` on the bound object, that is:: + Instances of ``MyClass`` can transparently save state between invocations by + setting attributes on ``_stored``. Initial state should be set with + ``set_default`` on the bound object, that is:: class MyClass(Object): _stored = StoredState() diff --git a/ops/model.py b/ops/model.py index 89cb3885e..3b8804ec0 100644 --- a/ops/model.py +++ b/ops/model.py @@ -452,12 +452,14 @@ class Unit: This might be your unit, another unit of your application, or a unit of another application that you are related to. - - Attributes: - name: The name of the unit (eg, 'mysql/0') - app: The Application the unit is a part of. """ + name: str + """Name of the unit, for example "mysql/0".""" + + app: Application + """Application the unit is part of.""" + def __init__(self, name: str, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend', cache: '_ModelCache'): self.name = name @@ -523,8 +525,6 @@ def is_leader(self) -> bool: This can only be called for your own unit. - Returns: - True if you are the leader, False otherwise Raises: RuntimeError: if called for a unit that is not yourself """ @@ -1538,8 +1538,8 @@ def _load(self): class StatusBase: """Status values specific to applications and units. - To access a status by name, see :meth:`StatusBase.from_name`, most use cases will just - directly use the child class to indicate their status. + To access a status by name, use :meth:`StatusBase.from_name`. However, most use cases will + directly use the child class such as :class:`ActiveStatus` to indicate their status. """ _statuses: Dict[str, Type['StatusBase']] = {} @@ -1562,7 +1562,18 @@ def __repr__(self): @classmethod def from_name(cls, name: str, message: str): - """Get the specific Status for the name (or UnknownStatus if not registered).""" + """Create a status instance from a name and message. + + If ``name`` is "unknown", ``message`` is ignored, because unknown status + does not have an associated message. + + Args: + name: Name of the status, for example "active" or "blocked". + message: Message to include with the status. + + Raises: + KeyError: If ``name`` is not a registered status. + """ if name == 'unknown': # unknown is special return UnknownStatus() @@ -1731,7 +1742,7 @@ def request(self, storage_name: str, count: int = 1): """Requests new storage instances of a given name. Uses storage-add tool to request additional storage. Juju will notify the unit - via -storage-attached events when it becomes available. + via ``-storage-attached`` events when it becomes available. """ if storage_name not in self._storage_map: raise ModelError(('cannot add storage {!r}:' @@ -1747,11 +1758,10 @@ def _invalidate(self, storage_name: str): class Storage: - """Represents a storage as defined in metadata.yaml. + """Represents a storage as defined in ``metadata.yaml``.""" - Attributes: - name: Simple string name of the storage - """ + name: str + """Name of the storage.""" def __init__(self, storage_name: str, storage_index: int, backend: '_ModelBackend'): self.name = storage_name @@ -1761,7 +1771,7 @@ def __init__(self, storage_name: str, storage_index: int, backend: '_ModelBacken @property def index(self) -> int: - """The index associated with the storage (usually 0 for singular storage).""" + """Index associated with the storage (usually 0 for singular storage).""" return self._index @property @@ -1772,12 +1782,12 @@ def id(self) -> int: @property def full_id(self) -> str: - """Returns the canonical storage name and id/index based identifier.""" + """Canonical storage name with index, for example "bigdisk/0".""" return f'{self.name}/{self._index}' @property def location(self) -> Path: - """Return the location of the storage.""" + """Location of the storage.""" if self._location is None: raw = self._backend.storage_get(self.full_id, "location") self._location = Path(raw) From 778d81885d27f853fa2153a3dea002fb427ceafb Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Thu, 29 Jun 2023 13:22:50 +1200 Subject: [PATCH 17/20] Docstring and attribute type updates for ops.pebble --- ops/pebble.py | 199 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 144 insertions(+), 55 deletions(-) diff --git a/ops/pebble.py b/ops/pebble.py index f16dd0afe..15fccb6e4 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -211,6 +211,14 @@ def __enter__(self) -> typing.IO[typing.AnyStr]: ... # noqa _FilesResponse = TypedDict('_FilesResponse', {'result': List[_Item]}) + _WarningDict = TypedDict('_WarningDict', + {'message': str, + 'first-added': str, + 'last-added': str, + 'last-shown': Optional[str], + 'expire-after': str, + 'repeat-after': str}) + class _WebSocket(Protocol): def connect(self, url: str, socket: socket.socket): ... # noqa def shutdown(self): ... # noqa @@ -299,14 +307,18 @@ class ProtocolError(Error): class PathError(Error): - """Raised when there's an error with a specific path. + """Raised when there's an error with a specific path.""" + + kind: str + """Short string representing the kind of error. - Attributes: - kind: A short string representing the kind of error. Possible values - are "not-found", "permission-denied", and "generic-file-error". - message: A human-readable error message. + Possible values are "not-found", "permission-denied", and + "generic-file-error". """ + message: str + """Human-readable error message from the API.""" + def __init__(self, kind: str, message: str): """This shouldn't be instantiated directly.""" self.kind = kind @@ -322,14 +334,25 @@ def __repr__(self): class APIError(Error): """Raised when an HTTP API error occurs talking to the Pebble server.""" + body: Dict[str, Any] + """Body of the HTTP response, parsed as JSON.""" + + code: int + """HTTP status code.""" + + status: str + """HTTP status string (reason).""" + + message: str + """Human-readable error message from the API.""" + def __init__(self, body: Dict[str, Any], code: int, status: str, message: str): """This shouldn't be instantiated directly.""" super().__init__(message) # Makes str(e) return message self.body = body self.code = code self.status = status - # FIXME: pyright rightfully complains that super().message is a method - self.message = message # type: ignore + self.message = message def __repr__(self): return f'APIError({self.body!r}, {self.code!r}, {self.status!r}, {self.message!r})' @@ -338,6 +361,12 @@ def __repr__(self): class ChangeError(Error): """Raised by actions when a change is ready but has an error.""" + err: str + """Human-readable error message.""" + + change: 'Change' + """Change object associated with this error.""" + def __init__(self, err: str, change: 'Change'): """This shouldn't be instantiated directly.""" self.err = err @@ -363,21 +392,33 @@ def __repr__(self): class ExecError(Error, Generic[AnyStr]): - """Raised when a :meth:`Client.exec` command returns a non-zero exit code. - - Attributes: - command: Command line of command being executed. - exit_code: The process's exit code. This will always be non-zero. - stdout: If :meth:`ExecProcess.wait_output` was being called, this is - the captured stdout as a str (or bytes if encoding was None). If - :meth:`ExecProcess.wait` was being called, this is None. - stderr: If :meth:`ExecProcess.wait_output` was being called and - combine_stderr was False, this is the captured stderr as a str (or - bytes if encoding was None). If :meth:`ExecProcess.wait` was being - called or combine_stderr was True, this is None. - """ + """Raised when a :meth:`Client.exec` command returns a non-zero exit code.""" STR_MAX_OUTPUT = 1024 + """Maximum number of characters that stdout/stderr are truncated to in ``__str__``.""" + + command: List[str] + """Command line of command being executed.""" + + exit_code: int + """The process's exit code. Because this is an error, this will always be non-zero.""" + + stdout: Optional[AnyStr] + """Standard output from the process. + + If :meth:`ExecProcess.wait_output` was being called, this is the captured + stdout as a str (or bytes if encoding was None). If :meth:`ExecProcess.wait` + was being called, this is None. + """ + + stderr: Optional[AnyStr] + """Standard error from the process. + + If :meth:`ExecProcess.wait_output` was being called and ``combine_stderr`` + was False, this is the captured stderr as a str (or bytes if encoding was + None). If :meth:`ExecProcess.wait` was being called or ``combine_stderr`` + was True, this is None. + """ def __init__( self, @@ -436,14 +477,6 @@ def __repr__(self): class Warning: """Warning object.""" - if typing.TYPE_CHECKING: - _WarningDict = TypedDict('_WarningDict', - {'message': str, - 'first-added': str, - 'last-added': str, - 'last-shown': Optional[str], - 'expire-after': str, - 'repeat-after': str}) def __init__( self, @@ -980,6 +1013,36 @@ class FileType(enum.Enum): class FileInfo: """Stat-like information about a single file or directory.""" + path: str + """Full path of the file.""" + + name: str + """Base name of the file.""" + + type: Union['FileType', str] + """Type of the file ("file", "directory", "symlink", etc).""" + + size: Optional[int] + """Size of the file (will be 0 if ``type`` is not "file").""" + + permissions: int + """Unix permissions of the file.""" + + last_modified: datetime.datetime + """Time file was last modified.""" + + user_id: Optional[int] + """User ID of the file.""" + + user: Optional[str] + """Username of the file.""" + + group_id: Optional[int] + """Group ID of the file.""" + + group: Optional[str] + """Group name of the file.""" + def __init__( self, path: str, @@ -1043,19 +1106,35 @@ class CheckInfo: """Check status information. A list of these objects is returned from :meth:`Client.get_checks`. + """ + + name: str + """Name of the check.""" + + level: Optional[Union[CheckLevel, str]] + """Check level. + + This can be :attr:`CheckLevel.ALIVE`, :attr:`CheckLevel.READY`, or None (level not set). + """ + + status: Union[CheckStatus, str] + """Status of the check. + + :attr:`CheckStatus.UP` means the check is healthy (the number of failures + is less than the threshold), :attr:`CheckStatus.DOWN` means the check is + unhealthy (the number of failures has reached the threshold). + """ - Attributes: - name: The name of the check. - level: The check level: :attr:`CheckLevel.ALIVE`, - :attr:`CheckLevel.READY`, or None (level not set). - status: The status of the check: :attr:`CheckStatus.UP` means the - check is healthy (the number of failures is less than the - threshold), :attr:`CheckStatus.DOWN` means the check is unhealthy - (the number of failures has reached the threshold). - failures: The number of failures since the check last succeeded (reset - to zero if the check succeeds). - threshold: The failure threshold, that is, how many consecutive - failures for the check to be considered "down". + failures: int + """Number of failures since the check last succeeded. + + This is reset to zero if the check succeeds. + """ + + threshold: int + """Failure threshold. + + This is how many consecutive failures for the check to be considered "down". """ def __init__( @@ -1111,21 +1190,31 @@ class ExecProcess(Generic[AnyStr]): This class should not be instantiated directly, only via :meth:`Client.exec`. + """ + + stdin: Optional[IO[AnyStr]] + """Standard input for the process. + + If the stdin argument was not passed to :meth:`Client.exec`, this is a + writable file-like object the caller can use to stream input to the + process. It is None if stdin was passed to :meth:`Client.exec`. + """ + + stdout: Optional[IO[AnyStr]] + """Standard output from the process. + + If the stdout argument was not passed to :meth:`Client.exec`, this is a + readable file-like object the caller can use to stream output from the + process. It is None if stdout was passed to :meth:`Client.exec`. + """ + + stderr: Optional[IO[AnyStr]] + """Standard error from the process. - Attributes: - stdin: If the stdin argument was not passed to :meth:`Client.exec`, - this is a writable file-like object the caller can use to stream - input to the process. It is None if stdin was passed to - :meth:`Client.exec`. - stdout: If the stdout argument was not passed to :meth:`Client.exec`, - this is a readable file-like object the caller can use to stream - output from the process. It is None if stdout was passed to - :meth:`Client.exec`. - stderr: If the stderr argument was not passed to :meth:`Client.exec` - and combine_stderr was False, this is a readable file-like object - the caller can use to stream error output from the process. It is - None if stderr was passed to :meth:`Client.exec` or combine_stderr - was True. + If the stderr argument was not passed to :meth:`Client.exec` and + ``combine_stderr`` was False, this is a readable file-like object the + caller can use to stream error output from the process. It is None if + stderr was passed to :meth:`Client.exec` or ``combine_stderr`` was True. """ def __init__( @@ -2032,7 +2121,7 @@ def remove_path(self, path: str, *, recursive: bool = False): recursive: If True, and path is a directory recursively deletes it and everything under it. If the path is a file, delete the file and do nothing if the file is non-existent. Behaviourally similar - to `rm -rf ` + to ``rm -rf ``. """ info: Dict[str, Any] = {'path': path} From e7ba22aaf65697ec994ab6c783f9375240185971 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Fri, 30 Jun 2023 14:28:37 +1200 Subject: [PATCH 18/20] ops.testing doc tweaks --- ops/testing.py | 119 ++++++++++++++++++++++++++----------------------- 1 file changed, 64 insertions(+), 55 deletions(-) diff --git a/ops/testing.py b/ops/testing.py index b7509acc2..b29f1d516 100755 --- a/ops/testing.py +++ b/ops/testing.py @@ -116,26 +116,30 @@ class Harness(Generic[CharmType]): Example:: harness = Harness(MyCharm) + self.addCleanup(harness.cleanup) # always clean up after ourselves + # Do initial setup here relation_id = harness.add_relation('db', 'postgresql') + # Now instantiate the charm to see events as the model changes harness.begin() harness.add_relation_unit(relation_id, 'postgresql/0') harness.update_relation_data(relation_id, 'postgresql/0', {'key': 'val'}) + # Check that charm has properly handled the relation_joined event for postgresql/0 self.assertEqual(harness.charm. ...) Args: charm_cls: The Charm class that you'll be testing. meta: A string or file-like object containing the contents of - metadata.yaml. If not supplied, we will look for a 'metadata.yaml' file in the + ``metadata.yaml``. If not supplied, we will look for a ``metadata.yaml`` file in the parent directory of the Charm, and if not found fall back to a trivial - 'name: test-charm' metadata. + ``name: test-charm`` metadata. actions: A string or file-like object containing the contents of - actions.yaml. If not supplied, we will look for a 'actions.yaml' file in the + ``actions.yaml``. If not supplied, we will look for an ``actions.yaml`` file in the parent directory of the Charm. config: A string or file-like object containing the contents of - config.yaml. If not supplied, we will look for a 'config.yaml' file in the + ``config.yaml``. If not supplied, we will look for a ``config.yaml`` file in the parent directory of the Charm. """ @@ -222,7 +226,7 @@ def set_can_connect(self, container: Union[str, model.Container], val: bool): @property def charm(self) -> CharmType: - """Return the instance of the charm class that was passed to __init__. + """Return the instance of the charm class that was passed to ``__init__``. Note that the Charm is not instantiated until you have called :meth:`.begin()`. Until then, attempting to access this property will raise @@ -277,7 +281,7 @@ def begin_with_initial_hooks(self) -> None: containers), and any relation-joined hooks based on what relations have been added before you called begin. Note that all of these are fired before returning control to the test suite, so if you want to introspect what happens at each step, you need to fire - them directly (e.g. Charm.on.install.emit()). + them directly (for example, ``Charm.on.install.emit()``). To use this with all the normal hooks, you should instantiate the harness, setup any relations that you want active when the charm starts, and then call this method. This @@ -379,10 +383,10 @@ def begin_with_initial_hooks(self) -> None: relation, remote_unit.app, remote_unit) def cleanup(self) -> None: - """Called by your test infrastructure to cleanup any temporary directories/files/etc. + """Called by your test infrastructure to clean up any temporary directories/files/etc. - Currently this only needs to be called if you test with resources. But it is reasonable - to always include a `testcase.addCleanup(harness.cleanup)` just in case. + You should always call ``self.addCleanup(harness.cleanup)`` after creating a + :class:`Harness`. """ self._backend._cleanup() @@ -470,7 +474,7 @@ def add_oci_resource(self, resource_name: str, def add_resource(self, resource_name: str, content: AnyStr) -> None: """Add content for a resource to the backend. - This will register the content, so that a call to `Model.resources.fetch(resource_name)` + This will register the content, so that a call to ``model.resources.fetch(resource_name)`` will return a path to a file containing that content. Args: @@ -574,7 +578,7 @@ def add_storage(self, storage_name: str, count: int = 1, def detach_storage(self, storage_id: str) -> None: """Detach a storage device. - The intent of this function is to simulate a "juju detach-storage" call. + The intent of this function is to simulate a ``juju detach-storage`` call. It will trigger a storage-detaching hook if the storage unit in question exists and is presently marked as attached. @@ -596,7 +600,7 @@ def detach_storage(self, storage_id: str) -> None: 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. + 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 and is presently marked as detached. @@ -622,7 +626,7 @@ def attach_storage(self, storage_id: str) -> None: def remove_storage(self, storage_id: str) -> None: """Detach a storage device. - The intent of this function is to simulate a "juju remove-storage" call. + The intent of this function is to simulate a ``juju remove-storage`` call. It will trigger a storage-detaching hook if the storage unit in question exists and is presently marked as attached. Then it will remove the storage unit from the testing backend. @@ -748,7 +752,7 @@ def add_relation_unit(self, relation_id: int, remote_unit_name: str) -> None: testing relations and their data bags slightly more natural. Args: - relation_id: The integer relation identifier (as returned by add_relation). + relation_id: The integer relation identifier (as returned by :meth:`add_relation`). remote_unit_name: A string representing the remote unit that is being added. Return: @@ -803,12 +807,8 @@ def remove_relation_unit(self, relation_id: int, remote_unit_name: str) -> None: charm life cycle explicit. Args: - relation_id: The integer relation identifier (as returned by add_relation). + relation_id: The integer relation identifier (as returned by :meth:`add_relation`). remote_unit_name: A string representing the remote unit that is being removed. - - Raises: - KeyError: if relation_id or remote_unit_name is not valid - ValueError: if remote_unit_name is not valid """ relation_name = self._backend._relation_names[relation_id] @@ -862,12 +862,15 @@ def get_relation_data(self, relation_id: int, app_or_unit: AppUnitOrName) -> Map Args: relation_id: The relation whose content we want to look at. - app_or_unit: An Application or Unit instance, or its name, whose data we want to read + app_or_unit: An :class:`Application ` or + :class:`Unit ` instance, or its name, whose data we + want to read. + Return: - A dict containing the relation data for `app_or_unit` or None. + A dict containing the relation data for ``app_or_unit`` or None. Raises: - KeyError: if relation_id doesn't exist + KeyError: if ``relation_id`` doesn't exist """ name = _get_app_or_unit_name(app_or_unit) @@ -878,22 +881,24 @@ def get_pod_spec(self) -> Tuple[Mapping[Any, Any], Mapping[Any, Any]]: """Return the content of the pod spec as last set by the charm. This returns both the pod spec and any k8s_resources that were supplied. - See the signature of Model.pod.set_spec + See the signature of :meth:`Pod.set_spec `. """ return self._backend._pod_spec def get_container_pebble_plan( self, container_name: str ) -> pebble.Plan: - """Return the current Plan that pebble is executing for the given container. + """Return the current plan that Pebble is executing for the given container. Args: container_name: The simple name of the associated container + Return: - The pebble.Plan for this container. You can use :meth:`ops.pebble.Plan.to_yaml` to get - a string form for the content. Will raise KeyError if no pebble client exists - for that container name. (should only happen if container is not present in - metadata.yaml) + The Pebble plan for this container. You can use + :meth:`Plan.to_yaml ` to get a string + form for the content. Will raise ``KeyError`` if no Pebble client + exists for that container name (should only happen if container is + not present in ``metadata.yaml``). """ client = self._backend._pebble_clients.get(container_name) if client is None: @@ -903,10 +908,10 @@ def get_container_pebble_plan( def container_pebble_ready(self, container_name: str): """Fire the pebble_ready hook for the associated container. - This will switch the given container's can_connect state to True + This will switch the given container's ``can_connect`` state to True before the hook function is called. - It will do nothing if begin() has not been called. + It will do nothing if :meth:`begin()` has not been called. """ if self._charm is None: return @@ -919,13 +924,14 @@ def get_workload_version(self) -> str: return self._backend._workload_version def set_model_info(self, name: Optional[str] = None, uuid: Optional[str] = None) -> None: - """Set the name and uuid of the Model that this is representing. + """Set the name and UUID of the model that this is representing. - This cannot be called once begin() has been called. But it lets you set the value that - will be returned by Model.name and Model.uuid. + This cannot be called once :meth:`begin` has been called. But it lets + you set the value that will be returned by :attr:`Model.name ` + and :attr:`Model.uuid `. - This is a convenience method to invoke both Harness.set_model_name - and Harness.set_model_uuid at once. + This is a convenience method to invoke both :meth:`set_model_name` + and :meth:`set_model_uuid` at once. """ if name is not None: self.set_model_name(name) @@ -935,8 +941,8 @@ def set_model_info(self, name: Optional[str] = None, uuid: Optional[str] = None) def set_model_name(self, name: str) -> None: """Set the name of the Model that this is representing. - This cannot be called once begin() has been called. But it lets you set the value that - will be returned by Model.name. + This cannot be called once :meth:`begin` has been called. But it lets + you set the value that will be returned by :attr:`Model.name `. """ if self._charm is not None: raise RuntimeError('cannot set the Model name after begin()') @@ -945,8 +951,8 @@ def set_model_name(self, name: str) -> None: def set_model_uuid(self, uuid: str) -> None: """Set the uuid of the Model that this is representing. - This cannot be called once begin() has been called. But it lets you set the value that - will be returned by Model.uuid. + This cannot be called once :meth:`begin` has been called. But it lets + you set the value that will be returned by :attr:`Model.uuid `. """ if self._charm is not None: raise RuntimeError('cannot set the Model uuid after begin()') @@ -960,10 +966,10 @@ def update_relation_data( ) -> None: """Update the relation data for a given unit or application in a given relation. - This also triggers the `relation_changed` event for this relation_id. + This also triggers the `relation_changed` event for the given ``relation_id``. Args: - relation_id: The integer relation_id representing this relation. + relation_id: The integer relation ID representing this relation. app_or_unit: The unit or application name that is being updated. This can be the local or remote application. key_values: Each key/value will be updated in the relation data. @@ -1084,12 +1090,12 @@ def update_config( This will trigger a `config_changed` event. - Note that the `key_values` mapping will only add or update configuration items. - To remove existing ones, see the `unset` parameter. + Note that the ``key_values`` mapping will only add or update configuration items. + To remove existing ones, see the ``unset`` parameter. Args: key_values: A Mapping of key:value pairs to update in config. - unset: An iterable of keys to remove from Config. + unset: An iterable of keys to remove from config. This sets the value to the default if defined, otherwise removes the key altogether. """ @@ -1101,13 +1107,13 @@ def update_config( def set_leader(self, is_leader: bool = True) -> None: """Set whether this unit is the leader or not. - If this charm becomes a leader then `leader_elected` will be triggered. If Harness.begin() - has already been called, then the charm's peer relation should usually be added *prior* to - calling this method (i.e. with Harness.add_relation) to properly initialize and make + 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 available relation data that leader elected hooks may want to access. Args: - is_leader: True/False as to whether this unit is the leader. + is_leader: Whether this unit is the leader. """ self._backend._is_leader = is_leader @@ -1117,16 +1123,19 @@ def set_leader(self, is_leader: bool = True) -> None: self._charm.on.leader_elected.emit() def set_planned_units(self, num_units: int) -> None: - """Set the number of "planned" units that "Application.planned_units" should return. + """Set the number of "planned" units. - In real world circumstances, this number will be the number of units in the - application. E.g., this number will be the number of peers this unit has, plus one, as we - count our own unit in the total. + This is the value that :meth:`Application.planned_units ` + should return. - A change to the return from planned_units will not generate an event. Typically, a charm - author would check planned units during a config or install hook, or after receiving a peer - relation joined event. + In real world circumstances, this number will be the number of units + in the application. That is, this number will be the number of peers + this unit has, plus one, as we count our own unit in the total. + A change to the return from ``planned_units`` will not generate an + event. Typically, a charm author would check planned units during a + config or install hook, or after receiving a peer relation joined + event. """ if num_units < 0: raise TypeError("num_units must be 0 or a positive integer.") From c5dc63027342b09d94003226f7ba5aa6dd4e663e Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 4 Jul 2023 10:04:15 +1200 Subject: [PATCH 19/20] Add note about observe() order of events --- ops/framework.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ops/framework.py b/ops/framework.py index 32ef0846f..c9bb0f644 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -732,6 +732,9 @@ def drop_snapshot(self, handle: Handle): def observe(self, bound_event: BoundEvent, observer: Callable[[Any], None]): """Register observer to be called when bound_event is emitted. + If this is called multiple times for the same event type, the + framework calls the observers in the order they were observed. + The bound_event is generally provided as an attribute of the object that emits the event, and is created in this style:: From 39fb51b52f8feb73bcef4baf079c4b6095fc2c6e Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Tue, 4 Jul 2023 11:58:32 +1200 Subject: [PATCH 20/20] Changes from Pietro's code review --- ops/charm.py | 28 +++++++++++++--------------- ops/pebble.py | 10 +++------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 8cc0ece97..1fd46bdac 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -367,6 +367,7 @@ class RelationEvent(HookEvent): relation: 'Relation' """The relation involved in this event.""" + # TODO(benhoyt): I *think* app should never be None, but confirm and update type app: Optional[model.Application] """The remote application that has triggered this event.""" @@ -716,7 +717,7 @@ def restore(self, snapshot: Dict[str, Any]): class SecretChangedEvent(SecretEvent): - """Event triggered by Juju on the observer when the secret owner changes its contents. + """Event triggered on the secret observer charm when the secret owner changes its contents. When the owner of a secret changes the secret's contents, Juju will create a new secret revision, and all applications or units that are tracking this @@ -729,7 +730,7 @@ class SecretChangedEvent(SecretEvent): class SecretRotateEvent(SecretEvent): - """Event triggered by Juju on the owner when the secret's rotation policy elapses. + """Event triggered on the secret owner charm when the secret's rotation policy elapses. This event is fired on the secret owner to inform it that the secret must be rotated. The event will keep firing until the owner creates a new @@ -744,7 +745,7 @@ def defer(self): class SecretRemoveEvent(SecretEvent): - """Event triggered by Juju on the owner when a secret revision can be removed. + """Event triggered on the secret owner charm when a secret revision can be removed. When the owner of a secret creates a new revision, and after all observers have updated to that new revision, this event will be fired to @@ -783,7 +784,7 @@ def restore(self, snapshot: Dict[str, Any]): class SecretExpiredEvent(SecretEvent): - """Event triggered by Juju on the owner when a secret's expiration time elapses. + """Event triggered on the secret owner charm when a secret's expiration time elapses. This event is fired on the secret owner to inform it that the secret revision must be removed. The event will keep firing until the owner removes the @@ -826,20 +827,17 @@ def defer(self): class CharmEvents(ObjectEvents): """Events generated by Juju pertaining to application lifecycle. - This class is used to create an event descriptor (``self.on``) for - a charm class that inherits from :class:`CharmBase`. The event descriptor - may be used to set up event handlers for corresponding events. - - By default, the events listed below will be provided via the - :attr:`CharmBase.on` attribute. For example:: + By default, the events listed as attributes of this class will be + provided via the :attr:`CharmBase.on` attribute. For example:: self.framework.observe(self.on.config_changed, self._on_config_changed) - In addition to the events listed below, dynamically-named events will also - be defined based on the charm's metadata (``metadata.yaml``) for relations, - storage, actions, and containers. These named events may be accessed as - ``self.on[].`` or using a prefix like ``self.on._``, - for example:: + In addition to the events listed as attributes of this class, + dynamically-named events will also be defined based on the charm's + metadata (``metadata.yaml``) for relations, storage, actions, and + containers. These named events may be accessed as + ``self.on[].`` or using a prefix like + ``self.on._``, for example:: self.framework.observe(self.on["db"].relation_created, self._on_db_relation_created) self.framework.observe(self.on.workload_pebble_ready, self._on_workload_pebble_ready) diff --git a/ops/pebble.py b/ops/pebble.py index 15fccb6e4..13e3a9f8d 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -309,19 +309,15 @@ class ProtocolError(Error): class PathError(Error): """Raised when there's an error with a specific path.""" - kind: str - """Short string representing the kind of error. - - Possible values are "not-found", "permission-denied", and - "generic-file-error". - """ + kind: typing.Literal["not-found", "permission-denied", "generic-file-error"] + """Short string representing the kind of error.""" message: str """Human-readable error message from the API.""" def __init__(self, kind: str, message: str): """This shouldn't be instantiated directly.""" - self.kind = kind + self.kind = kind # type: ignore self.message = message def __str__(self):