diff --git a/HACKING.md b/HACKING.md index 189a2aa1b..85a046859 100644 --- a/HACKING.md +++ b/HACKING.md @@ -105,6 +105,8 @@ Currently we don't publish separate versions of documentation for separate relea next to the relevant content (e.g. headings, etc.). +Noteworthy changes should also get a new entry in [CHANGES.md](CHANGES.md). + ## Dependencies diff --git a/ops/charm.py b/ops/charm.py index 3263ddce2..079744841 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -25,6 +25,7 @@ List, Literal, Mapping, + NoReturn, Optional, TextIO, Tuple, @@ -120,11 +121,14 @@ class ActionEvent(EventBase): params: Dict[str, Any] """The parameters passed to the action.""" - def defer(self) -> None: + def defer(self) -> NoReturn: """Action events are not deferrable like other events. This is because an action runs synchronously and the administrator is waiting for the result. + + Raises: + RuntimeError: always. """ raise RuntimeError('cannot defer action events') @@ -180,6 +184,13 @@ def set_results(self, results: Dict[str, Any]): Args: results: The result of the action as a Dict + + Raises: + ModelError: if a reserved key is used. + ValueError: if ``results`` has a mix of dotted/non-dotted keys that expand out to + result in duplicate keys, for example: :code:`{'a': {'b': 1}, 'a.b': 2}`. Also + raised if a dict is passed with a key that fails to meet the format requirements. + OSError: if extremely large (>100KB) results are provided. """ self.framework.model._backend.action_set(results) @@ -364,8 +375,11 @@ def add_metrics(self, metrics: Mapping[str, Union[int, float]], Args: metrics: Key-value mapping of metrics that have been gathered. labels: Key-value labels applied to the metrics. + + Raises: + ModelError: if invalid keys or values are provided. """ - self.framework.model._backend.add_metrics(metrics, labels) # type:ignore + self.framework.model._backend.add_metrics(metrics, labels) class RelationEvent(HookEvent): @@ -767,8 +781,12 @@ class SecretRotateEvent(SecretEvent): revision by calling :meth:`event.secret.set_content() `. """ - def defer(self) -> None: - """Secret rotation events are not deferrable (Juju handles re-invocation).""" + def defer(self) -> NoReturn: + """Secret rotation events are not deferrable (Juju handles re-invocation). + + Raises: + RuntimeError: always. + """ raise RuntimeError( 'Cannot defer secret rotation events. Juju will keep firing this ' 'event until you create a new revision.') @@ -847,8 +865,12 @@ def restore(self, snapshot: Dict[str, Any]): super().restore(snapshot) self._revision = cast(int, snapshot['revision']) - def defer(self) -> None: - """Secret expiration events are not deferrable (Juju handles re-invocation).""" + def defer(self) -> NoReturn: + """Secret expiration events are not deferrable (Juju handles re-invocation). + + Raises: + RuntimeError: always. + """ raise RuntimeError( 'Cannot defer secret expiration events. Juju will keep firing ' 'this event until you create a new revision.') diff --git a/ops/framework.py b/ops/framework.py index 54cf85778..e1be6b1a8 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -263,11 +263,11 @@ class EventSource: It is generally used as:: - class SomethingHappened(EventBase): + class SomethingHappened(ops.EventBase): pass class SomeObject(Object): - something_happened = EventSource(SomethingHappened) + something_happened = ops.EventSource(SomethingHappened) 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 @@ -445,6 +445,10 @@ def define_event(cls, event_kind: str, event_type: 'Type[EventBase]'): event. Must be a valid Python identifier, not be a keyword or an existing attribute. event_type: A type of the event to define. + + Raises: + RuntimeError: if the same event is defined twice, or if ``event_kind`` + is an invalid name. """ prefix = 'unable to define an event with event_kind that ' if not event_kind.isidentifier(): @@ -710,7 +714,7 @@ def save_snapshot(self, value: Union["StoredStateData", "EventBase"]): marshal.dumps(data) except ValueError: msg = "unable to save the data for {}, it must contain only simple types: {!r}" - raise ValueError(msg.format(value.__class__.__name__, data)) + raise ValueError(msg.format(value.__class__.__name__, data)) from None self._storage.save_snapshot(value.handle.path, data) @@ -954,6 +958,9 @@ def breakpoint(self, name: Optional[str] = None): stop execution when a hook event is about to be handled. For those reasons, the "all" and "hook" breakpoint names are reserved. + + Raises: + ValueError: if the breakpoint name is invalid. """ # If given, validate the name comply with all the rules if name is not None: @@ -1099,15 +1106,15 @@ class StoredState: Example:: - class MyClass(Object): - _stored = StoredState() + class MyClass(ops.Object): + _stored = ops.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:: - class MyClass(Object): - _stored = StoredState() + class MyClass(ops.Object): + _stored = ops.StoredState() def __init__(self, parent, key): super().__init__(parent, key) diff --git a/ops/model.py b/ops/model.py index 110223351..33f7f12cc 100644 --- a/ops/model.py +++ b/ops/model.py @@ -355,7 +355,7 @@ def status(self) -> 'StatusBase': Example:: - self.model.app.status = BlockedStatus('I need a human to come help me') + self.model.app.status = ops.BlockedStatus('I need a human to come help me') """ if not self._is_our_app: return UnknownStatus() @@ -402,6 +402,8 @@ def planned_units(self) -> int: This method only returns data for this charm's application -- the Juju agent isn't able to see planned unit counts for other applications in the model. + Raises: + RuntimeError: on trying to get the planned units for a remote application. """ if not self._is_our_app: raise RuntimeError( @@ -432,6 +434,9 @@ def add_secret(self, content: Dict[str, str], *, rotate: Rotation policy/time. Every time this elapses, Juju will notify the charm by sending a SecretRotate event. None (the default) means to use the Juju default, which is never rotate. + + Raises: + ValueError: if the secret is empty, or the secret key is invalid. """ Secret._validate_content(content) id = self._backend.secret_add( @@ -507,9 +512,10 @@ def status(self) -> 'StatusBase': RuntimeError: if setting the status of a unit other than the current unit InvalidStatusError: if setting the status to something other than a :class:`StatusBase` + Example:: - self.model.unit.status = MaintenanceStatus('reconfiguring the frobnicators') + self.model.unit.status = ops.MaintenanceStatus('reconfiguring the frobnicators') """ if not self._is_our_unit: return UnknownStatus() @@ -568,7 +574,11 @@ def set_workload_version(self, version: str) -> None: @property def containers(self) -> Mapping[str, 'Container']: - """Return a mapping of containers indexed by name.""" + """Return a mapping of containers indexed by name. + + Raises: + RuntimeError: if called for another unit + """ if not self._is_our_unit: raise RuntimeError(f'cannot get container for a remote unit {self}') return self._containers @@ -582,7 +592,7 @@ def get_container(self, container_name: str) -> 'Container': try: return self.containers[container_name] except KeyError: - raise ModelError(f'container {container_name!r} not found') + raise ModelError(f'container {container_name!r} not found') from None def add_secret(self, content: Dict[str, str], *, label: Optional[str] = None, @@ -592,6 +602,9 @@ def add_secret(self, content: Dict[str, str], *, """Create a :class:`Secret` owned by this unit. See :meth:`Application.add_secret` for parameter details. + + Raises: + ValueError: if the secret is empty, or the secret key is invalid. """ Secret._validate_content(content) id = self._backend.secret_add( @@ -679,6 +692,11 @@ def set_ports(self, *ports: Union[int, 'Port']) -> None: Args: ports: The ports to open. Provide an int to open a TCP port, or a :class:`Port` to open a port for another protocol. + + Raises: + ModelError: if a :class:`Port` is provided where ``protocol`` is 'icmp' but + ``port`` is not ``None``, or where ``protocol`` is 'tcp' or 'udp' and ``port`` + is ``None``. """ # Normalise to get easier comparisons. existing = { @@ -1850,6 +1868,9 @@ def fetch(self, name: str) -> Path: If successfully fetched, this returns the path where the resource is stored on disk, otherwise it raises a :class:`NameError`. + + Raises: + NameError: if the resource's path cannot be fetched. """ if name not in self._paths: raise NameError(f'invalid resource name: {name}') @@ -1919,6 +1940,9 @@ def request(self, storage_name: str, count: int = 1): Uses storage-add tool to request additional storage. Juju will notify the unit via ``-storage-attached`` events when it becomes available. + + Raises: + ModelError: if the storage is not in the charm's metadata. """ if storage_name not in self._storage_map: raise ModelError(('cannot add storage {!r}:' @@ -2012,6 +2036,17 @@ class Container: This class should not be instantiated directly, instead use :meth:`Unit.get_container` or :attr:`Unit.containers`. + + For methods that make changes to the container, if the change fails or times out, then a + :class:`ops.pebble.ChangeError` or :class:`ops.pebble.TimeoutError` will be raised. + + Interactions with the container use Pebble, so all methods may raise exceptions when there are + problems communicating with Pebble. Problems connecting to or transferring data with Pebble + will raise a :class:`ops.pebble.ConnectionError` - generally you can guard against these by + first checking :meth:`can_connect`, but it is possible for problems to occur after + :meth:`can_connect` has succeeded. When an error occurs executing the request, such as trying + to add an invalid layer or execute a command that does not exist, an + :class:`ops.pebble.APIError` is raised. """ name: str @@ -2139,7 +2174,8 @@ def get_services(self, *service_names: str) -> Mapping[str, 'pebble.ServiceInfo' def get_service(self, service_name: str) -> pebble.ServiceInfo: """Get status information for a single named service. - Raises :class:`ModelError` if service_name is not found. + Raises: + ModelError: if service_name is not found. """ services = self.get_services(service_name) if not services: @@ -2166,7 +2202,8 @@ 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: + ModelError: if ``check_name`` is not found. """ checks = self.get_checks(check_name) if not checks: @@ -2557,7 +2594,6 @@ def remove_path(self, path: Union[str, PurePath], *, recursive: bool = False): Raises: pebble.PathError: If a relative path is provided, or if `recursive` is False and the file or directory cannot be removed (it does not exist or is not empty). - """ self._pebble.remove_path(str(path), recursive=recursive) @@ -2627,6 +2663,13 @@ def exec( See :meth:`ops.pebble.Client.exec` for documentation of the parameters and return value, as well as examples. + + Note that older versions of Juju do not support the ``service_content`` parameter, so if + the Charm is to be used on those versions, then + :meth:`JujuVersion.supports_exec_service_context` should be used as a guard. + + Raises: + ExecError: if the command exits with a non-zero exit code. """ if service_context is not None: version = JujuVersion.from_environ() @@ -2902,7 +2945,7 @@ def _run(self, *args: str, return_output: bool = False, try: result = subprocess.run(args, **kwargs) # type: ignore except subprocess.CalledProcessError as e: - raise ModelError(e.stderr) + raise ModelError(e.stderr) from e if return_output: if result.stdout is None: # type: ignore return '' diff --git a/ops/pebble.py b/ops/pebble.py index 80ce8f6e7..e1e8e3648 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -1344,6 +1344,7 @@ def wait_output(self) -> Tuple[AnyStr, Optional[AnyStr]]: Raises: ChangeError: if there was an error starting or running the process. ExecError: if the process exits with a non-zero exit code. + TypeError: if :meth:`Client.exec` was called with the ``stdout`` argument. """ if self.stdout is None: raise TypeError( @@ -1528,6 +1529,15 @@ class Client: Defaults to using a Unix socket at socket_path (which must be specified unless a custom opener is provided). + + For methods that wait for changes, such as :meth:`start_services` and :meth:`replan_services`, + if the change fails or times out, then a :class:`ChangeError` or :class:`TimeoutError` will be + raised. + + All methods may raise exceptions when there are problems communicating with Pebble. Problems + connecting to or transferring data with Pebble will raise a :class:`ConnectionError`. When an + error occurs executing the request, such as trying to add an invalid layer or execute a command + that does not exist, an :class:`APIError` is raised. """ _chunk_size = 8192 @@ -1621,13 +1631,13 @@ def _request_raw( # Will only happen on read error or if Pebble sends invalid JSON. body: Dict[str, Any] = {} message = f'{type(e2).__name__} - {e2}' - raise APIError(body, code, status, message) + raise APIError(body, code, status, message) from None except urllib.error.URLError as e: if e.args and isinstance(e.args[0], FileNotFoundError): raise ConnectionError( f"Could not connect to Pebble: socket not found at {self.socket_path!r} " "(container restarted?)") from None - raise ConnectionError(e.reason) + raise ConnectionError(e.reason) from e return response @@ -1673,16 +1683,16 @@ def autostart_services(self, timeout: float = 30.0, delay: float = 0.1) -> Chang """Start the startup-enabled services and wait (poll) for them to be started. Args: - timeout: Seconds before autostart change is considered timed out (float). + timeout: Seconds before autostart change is considered timed out (float). If + timeout is 0, submit the action but don't wait; just return the change ID + immediately. delay: Seconds before executing the autostart change (float). Returns: ChangeID of the autostart change. Raises: - ChangeError: if one or more of the services didn't start. If - timeout is 0, submit the action but don't wait; just return the change - ID immediately. + ChangeError: if one or more of the services didn't start, and ``timeout`` is non-zero. """ return self._services_action('autostart', [], timeout, delay) @@ -1690,16 +1700,17 @@ def replan_services(self, timeout: float = 30.0, delay: float = 0.1) -> ChangeID """Replan by (re)starting changed and startup-enabled services and wait for them to start. Args: - timeout: Seconds before replan change is considered timed out (float). + timeout: Seconds before replan change is considered timed out (float). If + timeout is 0, submit the action but don't wait; just return the change + ID immediately. delay: Seconds before executing the replan change (float). Returns: ChangeID of the replan change. Raises: - ChangeError: if one or more of the services didn't stop/start. If - timeout is 0, submit the action but don't wait; just return the change - ID immediately. + ChangeError: if one or more of the services didn't stop/start, and ``timeout`` is + non-zero. """ return self._services_action('replan', [], timeout, delay) @@ -1710,16 +1721,17 @@ def start_services( Args: services: Non-empty list of services to start. - timeout: Seconds before start change is considered timed out (float). + timeout: Seconds before start change is considered timed out (float). If + timeout is 0, submit the action but don't wait; just return the change + ID immediately. delay: Seconds before executing the start change (float). Returns: ChangeID of the start change. Raises: - ChangeError: if one or more of the services didn't stop/start. If - timeout is 0, submit the action but don't wait; just return the change - ID immediately. + ChangeError: if one or more of the services didn't stop/start, and ``timeout`` is + non-zero. """ return self._services_action('start', services, timeout, delay) @@ -1730,16 +1742,17 @@ def stop_services( Args: services: Non-empty list of services to stop. - timeout: Seconds before stop change is considered timed out (float). + timeout: Seconds before stop change is considered timed out (float). If + timeout is 0, submit the action but don't wait; just return the change + ID immediately. delay: Seconds before executing the stop change (float). Returns: ChangeID of the stop change. Raises: - ChangeError: if one or more of the services didn't stop/start. If - timeout is 0, submit the action but don't wait; just return the change - ID immediately. + ChangeError: if one or more of the services didn't stop/start and ``timeout`` is + non-zero. """ return self._services_action('stop', services, timeout, delay) @@ -1750,16 +1763,17 @@ def restart_services( Args: services: Non-empty list of services to restart. - timeout: Seconds before restart change is considered timed out (float). + timeout: Seconds before restart change is considered timed out (float). If + timeout is 0, submit the action but don't wait; just return the change + ID immediately. delay: Seconds before executing the restart change (float). Returns: ChangeID of the restart change. Raises: - ChangeError: if one or more of the services didn't stop/start. If - timeout is 0, submit the action but don't wait; just return the change - ID immediately. + ChangeError: if one or more of the services didn't stop/start and ``timeout`` is + non-zero. """ return self._services_action('restart', services, timeout, delay) @@ -2021,6 +2035,10 @@ def push( group_id: Group ID (GID) for file. group: Group name for file. Group's GID must match group_id if both are specified. + + Raises: + PathError: If there was an error writing the file to the path; for example, if the + destination path doesn't exist and ``make_dirs`` is not used. """ info = self._make_auth_dict(permissions, user_id, user, group_id, group) info['path'] = path @@ -2118,6 +2136,10 @@ def list_files(self, path: str, *, pattern: Optional[str] = None, for example ``*.txt``. itself: If path refers to a directory, return information about the directory itself, rather than its contents. + + Raises: + PathError: if there was an error listing the directory; for example, if the directory + does not exist. """ query = { 'action': 'list', @@ -2151,6 +2173,10 @@ def make_dir( group_id: Group ID (GID) for directory. group: Group name for directory. Group's GID must match group_id if both are specified. + + Raises: + PathError: if there was an error making the directory; for example, if the parent path + does not exist, and ``make_parents`` is not used. """ info = self._make_auth_dict(permissions, user_id, user, group_id, group) info['path'] = path @@ -2176,7 +2202,6 @@ def remove_path(self, path: str, *, recursive: bool = False): Raises: pebble.PathError: If a relative path is provided, or if `recursive` is False and the file or directory cannot be removed (it does not exist or is not empty). - """ info: Dict[str, Any] = {'path': path} if recursive: @@ -2368,6 +2393,11 @@ def exec( :meth:`ExecProcess.wait` if stdout/stderr were provided as arguments to :meth:`exec`, or :meth:`ExecProcess.wait_output` if not. + + Raises: + APIError: if an error occurred communicating with pebble, or if the command is not + found. + ExecError: if the command exits with a non-zero exit code. """ if not isinstance(command, list) or not all(isinstance(s, str) for s in command): raise TypeError(f'command must be a list of str, not {type(command).__name__}') diff --git a/ops/storage.py b/ops/storage.py index d8ddacbd5..5dda21d0e 100755 --- a/ops/storage.py +++ b/ops/storage.py @@ -104,7 +104,7 @@ def save_snapshot(self, handle_path: str, snapshot_data: Any) -> None: Args: handle_path: The string identifying the snapshot. snapshot_data: The data to be persisted. (as returned by Object.snapshot()). This - might be a dict/tuple/int, but must only contain 'simple' python types. + might be a dict/tuple/int, but must only contain 'simple' Python types. """ # Use pickle for serialization, so the value remains portable. raw_data = pickle.dumps(snapshot_data) @@ -235,7 +235,7 @@ def load_snapshot(self, handle_path: str): try: content = self._backend.get(handle_path) except KeyError: - raise NoSnapshotError(handle_path) + raise NoSnapshotError(handle_path) from None return content def drop_snapshot(self, handle_path: str): diff --git a/ops/testing.py b/ops/testing.py index f52473628..8d638b9a1 100755 --- a/ops/testing.py +++ b/ops/testing.py @@ -305,6 +305,8 @@ def begin(self) -> None: Before calling :meth:`begin`, there is no Charm instance, so changes to the Model won't emit events. Call :meth:`.begin` for :attr:`.charm` to be valid. + + Should only be called once. """ if self._charm is not None: raise RuntimeError('cannot call the begin method on the harness more than once') @@ -542,9 +544,9 @@ def _get_config(self, charm_config_yaml: Optional['YAMLStringOrFile']): def add_oci_resource(self, resource_name: str, contents: Optional[Mapping[str, str]] = None) -> None: - """Add oci resources to the backend. + """Add OCI resources to the backend. - This will register an oci resource and create a temporary file for processing metadata + This will register an OCI resource and create a temporary file for processing metadata about the resource. A default set of values will be used for all the file contents unless a specific contents dict is provided. @@ -577,7 +579,7 @@ def add_resource(self, resource_name: str, content: AnyStr) -> None: returned by resource-get. If contents is a string, it will be encoded in utf-8 """ if resource_name not in self._meta.resources.keys(): - raise RuntimeError(f'Resource {resource_name} is not a defined resources') + raise RuntimeError(f'Resource {resource_name} is not a defined resource') record = self._meta.resources[resource_name] if record.type != "file": raise RuntimeError( @@ -679,6 +681,9 @@ def detach_storage(self, storage_id: str) -> None: It will trigger a storage-detaching hook if the storage unit in question exists and is presently marked as attached. + Note that the Charm is not instantiated until :meth:`begin` is called. + Until then, attempting to use this method will raise an exception. + Args: storage_id: The full storage ID of the storage unit being detached, including the storage key, e.g. my-storage/0. @@ -735,6 +740,9 @@ def remove_storage(self, storage_id: str) -> None: Args: storage_id: The full storage ID of the storage unit being removed, including the storage key, e.g. my-storage/0. + + Raises: + RuntimeError: if the storage is not in the metadata. """ storage_name, storage_index = storage_id.split('/', 1) storage_index = int(storage_index) @@ -893,9 +901,6 @@ def add_relation_unit(self, relation_id: int, remote_unit_name: str) -> None: Args: 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: - None """ self._backend._relation_list_map[relation_id].append(remote_unit_name) # we can write remote unit data iff we are not in a hook env @@ -1035,9 +1040,11 @@ def get_container_pebble_plan( Return: The Pebble plan for this container. 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``). + form for the content. + + Raises: + 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: @@ -1241,6 +1248,9 @@ def update_config( unset: An iterable of keys to remove from config. This sets the value to the default if defined, otherwise removes the key altogether. + + Raises: + ValueError: if the key is not present in the config. """ self._update_config(key_values, unset) if self._charm is None or not self._hooks_enabled: @@ -1290,7 +1300,6 @@ def reset_planned_units(self) -> None: This allows the harness to fall through to the built in methods that will try to guess at a value for planned units, based on the number of peer relations that have been setup in the testing harness. - """ self._backend._planned_units = None @@ -2001,17 +2010,17 @@ def _get_resource_dir(self) -> pathlib.Path: def relation_ids(self, relation_name: str) -> List[int]: try: return self._relation_ids_map[relation_name] - except KeyError as e: + except KeyError: if relation_name not in self._meta.relations: - raise model.ModelError(f'{relation_name} is not a known relation') from e + raise model.ModelError(f'{relation_name} is not a known relation') from None no_ids: List[int] = [] return no_ids def relation_list(self, relation_id: int): try: return self._relation_list_map[relation_id] - except KeyError as e: - raise model.RelationNotFoundError from e + except KeyError: + raise model.RelationNotFoundError from None def relation_remote_app_name(self, relation_id: int) -> Optional[str]: if relation_id not in self._relation_app_and_units: @@ -2136,7 +2145,7 @@ def storage_get(self, storage_name_id: str, attribute: str) -> Any: return self._storage_list[name][index][attribute] except KeyError: raise model.ModelError( - f'ERROR invalid value "{name}/{index}" for option -s: storage not found') + f'ERROR invalid value "{name}/{index}" for option -s: storage not found') from None def storage_add(self, name: str, count: int = 1) -> List[int]: if '/' in name: @@ -2945,10 +2954,10 @@ def remove_path(self, path: str, *, recursive: bool = False): else: try: file_path.rmdir() - except OSError: + except OSError as e: raise pebble.PathError( 'generic-file-error', - 'cannot remove non-empty directory without recursive=True') + 'cannot remove non-empty directory without recursive=True') from e else: file_path.unlink() @@ -3055,7 +3064,7 @@ def exec( raise RuntimeError( "a TimeoutError occurred in the execution handler, " "but no timeout value was provided in the execution arguments." - ) + ) from None if result is None: exit_code = 0 proc_stdout = self._transform_exec_handler_output(b'', encoding) @@ -3119,7 +3128,7 @@ def send_signal(self, sig: Union[int, str], service_names: Iterable[str]): body=body, code=500, status='Internal Server Error', - message=message) + message=message) from None def get_checks(self, level=None, names=None): # type:ignore raise NotImplementedError(self.get_checks) # type:ignore