From fa185b48563b4b67c20a8b46dbae03c71efc8fe9 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 17 Apr 2024 17:07:16 +1200 Subject: [PATCH] fix!: correct the model config types (#1183) Corrects types related to the model configuration: * `LazyMapping` may have Boolean, integer, or float values, as well as strings * `_ConfigOption` may have a `secret` value for `type - also move this to [testing.py](ops/testing.py) since that's the only place it's used and it's private * `_ModelBackend.config_get` returns a dict with Boolean, integer, float, or string values The PR also sets a missing return type so that pyright would detect the `config_get` issue. There are no tests or pyright changes for the `_ConfigOption` change: we don't generally rely on pyright rather than unit tests for ensuring the typing is correct, but that doesn't seem feasible here. Let me know if you think we do need something added here to prevent regression. There are no tests or pyright changes for the `LazyMapping` change, other than making sure we do have [test_model.py](tests/test_model.py) exercise each of the config types. I don't think it's possible to have pyright detect this issue - e.g. you can get it to show an error if you try to use an integer config value as an integer (`self.config["int-opt"] + 1`), but that's still the case after the fix, since it could be a string (float, Boolean, secret) option. The test could have `typing.assert_type` calls but (a) that's only in Python 3.11, and (b) I'm not convinced we really want to do that. Let me know if you think we do need something added here to prevent regression. Fixes #1182 --- .../workflows/observability-charm-tests.yaml | 7 ++- docs/custom_conf.py | 6 +- ops/model.py | 56 ++++++++++++------- ops/testing.py | 7 ++- test/test_model.py | 9 ++- 5 files changed, 60 insertions(+), 25 deletions(-) diff --git a/.github/workflows/observability-charm-tests.yaml b/.github/workflows/observability-charm-tests.yaml index 98a96372f..7edd0c9c8 100644 --- a/.github/workflows/observability-charm-tests.yaml +++ b/.github/workflows/observability-charm-tests.yaml @@ -10,8 +10,11 @@ jobs: fail-fast: false matrix: charm-repo: - - "canonical/alertmanager-k8s-operator" - - "canonical/prometheus-k8s-operator" +# Disabled until the type checking passes (ops#1183) +# https://github.com/canonical/alertmanager-k8s-operator/pull/241 +# https://github.com/canonical/prometheus-k8s-operator/pull/587 +# - "canonical/alertmanager-k8s-operator" +# - "canonical/prometheus-k8s-operator" - "canonical/grafana-k8s-operator" steps: diff --git a/docs/custom_conf.py b/docs/custom_conf.py index 1a9a68c66..4b58be54c 100644 --- a/docs/custom_conf.py +++ b/docs/custom_conf.py @@ -1,5 +1,5 @@ # ruff: noqa -import datetime +import datetime import pathlib import sys @@ -318,10 +318,11 @@ def _compute_navigation_tree(context): ('py:class', '_Writeable'), ('py:class', 'ops.charm._ContainerBaseDict'), ('py:class', 'ops.model._AddressDict'), - ('py:class', 'ops.model._ConfigOption'), + ('py:class', 'ops.model._GenericLazyMapping'), ('py:class', 'ops.model._ModelBackend'), ('py:class', 'ops.model._ModelCache'), ('py:class', 'ops.model._NetworkDict'), + ('py:class', 'ops.model._SupportsKeysAndGetItem'), ('py:class', 'ops.pebble._FileLikeIO'), ('py:class', 'ops.pebble._IOSource'), ('py:class', 'ops.pebble._ServiceInfoDict'), @@ -329,6 +330,7 @@ def _compute_navigation_tree(context): ('py:class', 'ops.pebble._WebSocket'), ('py:class', 'ops.storage.JujuStorage'), ('py:class', 'ops.storage.SQLiteStorage'), + ('py:class', 'ops.testing._ConfigOption'), ('py:class', 'ops.testing.CharmType'), ('py:obj', 'ops.testing.CharmType'), ] diff --git a/ops/model.py b/ops/model.py index 2ccdc33ea..10ee8ec17 100644 --- a/ops/model.py +++ b/ops/model.py @@ -41,7 +41,6 @@ Generator, Iterable, List, - Literal, Mapping, MutableMapping, Optional, @@ -61,12 +60,6 @@ # a k8s spec is a mapping from names/"types" to json/yaml spec objects K8sSpec = Mapping[str, Any] -_ConfigOption = TypedDict('_ConfigOption', { - 'type': Literal['string', 'int', 'float', 'boolean'], - 'description': str, - 'default': Union[str, int, float, bool], -}) - _StorageDictType = Dict[str, Optional[List['Storage']]] _BindingDictType = Dict[Union[str, 'Relation'], 'Binding'] @@ -98,6 +91,16 @@ }) +# Copied from typeshed. +_KT = typing.TypeVar("_KT") +_VT_co = typing.TypeVar("_VT_co", covariant=True) + + +class _SupportsKeysAndGetItem(typing.Protocol[_KT, _VT_co]): + def keys(self) -> typing.Iterable[_KT]: ... + def __getitem__(self, __key: _KT) -> _VT_co: ... + + logger = logging.getLogger(__name__) MAX_LOG_LINE_LEN = 131071 # Max length of strings to pass to subshell. @@ -775,7 +778,10 @@ class Port: """ -class LazyMapping(Mapping[str, str], ABC): +_LazyValueType = typing.TypeVar("_LazyValueType") + + +class _GenericLazyMapping(Mapping[str, _LazyValueType], ABC): """Represents a dict that isn't populated until it is accessed. Charm authors should generally never need to use this directly, but it forms @@ -783,14 +789,14 @@ class LazyMapping(Mapping[str, str], ABC): """ # key-value mapping - _lazy_data: Optional[Dict[str, str]] = None + _lazy_data: Optional[Dict[str, _LazyValueType]] = None @abstractmethod - def _load(self) -> Dict[str, str]: + def _load(self) -> Dict[str, _LazyValueType]: raise NotImplementedError() @property - def _data(self) -> Dict[str, str]: + def _data(self) -> Dict[str, _LazyValueType]: data = self._lazy_data if data is None: data = self._lazy_data = self._load() @@ -802,19 +808,27 @@ def _invalidate(self): def __contains__(self, key: str) -> bool: return key in self._data - def __len__(self): + def __len__(self) -> int: return len(self._data) def __iter__(self): return iter(self._data) - def __getitem__(self, key: str) -> str: + def __getitem__(self, key: str) -> _LazyValueType: return self._data[key] - def __repr__(self): + def __repr__(self) -> str: return repr(self._data) +class LazyMapping(_GenericLazyMapping[str]): + """Represents a dict[str, str] that isn't populated until it is accessed. + + Charm authors should generally never need to use this directly, but it forms + the basis for many of the dicts that the framework tracks. + """ + + class RelationMapping(Mapping[str, List['Relation']]): """Map of relation names to lists of :class:`Relation` instances.""" @@ -1564,7 +1578,7 @@ def __len__(self): def __iter__(self): return iter(self._data) - def __getitem__(self, key: Union['Unit', 'Application']): + def __getitem__(self, key: Union['Unit', 'Application']) -> 'RelationDataContent': return self._data[key] def __repr__(self): @@ -1708,6 +1722,10 @@ def __getitem__(self, key: str) -> str: self._validate_read() return super().__getitem__(key) + def update(self, other: _SupportsKeysAndGetItem[str, str], **kwargs: str): + """Update the data from dict/iterable other and the kwargs.""" + super().update(other, **kwargs) + def __delitem__(self, key: str): self._validate_write(key, '') # Match the behavior of Juju, which is that setting the value to an empty @@ -1722,7 +1740,7 @@ def __repr__(self): return super().__repr__() -class ConfigData(LazyMapping): +class ConfigData(_GenericLazyMapping[Union[bool, int, float, str]]): """Configuration data. This class should not be instantiated directly. It should be accessed via :attr:`Model.config`. @@ -1731,7 +1749,7 @@ class ConfigData(LazyMapping): def __init__(self, backend: '_ModelBackend'): self._backend = backend - def _load(self): + def _load(self) -> Dict[str, Union[bool, int, float, str]]: return self._backend.config_get() @@ -3131,9 +3149,9 @@ def relation_set(self, relation_id: int, key: str, value: str, is_app: bool) -> raise RelationNotFoundError() from e raise - def config_get(self) -> Dict[str, '_ConfigOption']: + def config_get(self) -> Dict[str, Union[bool, int, float, str]]: out = self._run('config-get', return_output=True, use_json=True) - return typing.cast(Dict[str, '_ConfigOption'], out) + return typing.cast(Dict[str, Union[bool, int, float, str]], out) def is_leader(self) -> bool: """Obtain the current leadership status for the unit the charm code is executing on. diff --git a/ops/testing.py b/ops/testing.py index 88d56e2cb..882d8b07d 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -60,7 +60,7 @@ from ops import charm, framework, model, pebble, storage from ops._private import yaml from ops.charm import CharmBase, CharmMeta, RelationRole -from ops.model import Container, RelationNotFoundError, _ConfigOption, _NetworkDict +from ops.model import Container, RelationNotFoundError, _NetworkDict from ops.pebble import ExecProcess ReadableBuffer = Union[bytes, str, StringIO, BytesIO, BinaryIO] @@ -84,6 +84,11 @@ 'status': _StatusName, 'message': str, }) +_ConfigOption = TypedDict('_ConfigOption', { + 'type': Literal['string', 'int', 'float', 'boolean', 'secret'], + 'description': str, + 'default': Union[str, int, float, bool], +}) _RawConfig = TypedDict("_RawConfig", {'options': Dict[str, _ConfigOption]}) diff --git a/test/test_model.py b/test/test_model.py index fc00272ef..a13dc1d49 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -60,6 +60,10 @@ def setUp(self): type: int qux: type: boolean + baz: + type: float + secretfoo: + type: secret ''') self.addCleanup(self.harness.cleanup) self.relation_id_db0 = self.harness.add_relation('db0', 'db') @@ -618,11 +622,14 @@ def test_relation_no_units(self): def test_config(self): self.harness._get_backend_calls(reset=True) - self.harness.update_config({'foo': 'foo', 'bar': 1, 'qux': True}) + self.harness.update_config({'foo': 'foo', 'bar': 1, 'qux': True, + 'baz': 3.1, 'secretfoo': 'secret:1234'}) assert self.model.config == { 'foo': 'foo', 'bar': 1, 'qux': True, + 'baz': 3.1, + 'secretfoo': 'secret:1234', } with pytest.raises(TypeError): # Confirm that we cannot modify config values.