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

.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:

docs/custom_conf.py

@@ -1,5 +1,5 @@
 # ruff: noqa
-import datetime 
+import datetime
 import pathlib
 import sys
 
@@ -318,17 +318,19 @@ 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'),
     ('py:class', 'ops.pebble._SystemInfoDict'),
     ('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'),
 ]

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,22 +778,25 @@ 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
     the basis for many of the dicts that the framework tracks.
     """
 
     # 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.

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]})
 
 

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.