Repository: canonical/operator
Pull request #411 (2020-09-22 13:48:41)

make model.config immutable
fixes: #332

chipaca: (I addressed @facundobatista's concerns in chat: it can at most break the tests of a charm that was doing something it shouldn't)

Patch:
diff --git a/ops/model.py b/ops/model.py
index ea8c0a3..f696391 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -45,33 +45,62 @@ class Model:
 
     This should not be instantiated directly by Charmers, but can be accessed as `self.model`
     from any class that derives from Object.
-
-    Attributes:
-        unit: A :class:`Unit` that represents the unit that is running this code (eg yourself)
-        app: A :class:`Application` that represents the application this unit is a part of.
-        relations: Mapping of endpoint to list of :class:`Relation` answering the question
-            "what am I currently related to". See also :meth:`.get_relation`
-        config: A dict of the config for the current application.
-        resources: Access to resources for this charm. Use ``model.resources.fetch(resource_name)``
-            to get the path on disk where the resource can be found.
-        storages: Mapping of storage_name to :class:`Storage` for the storage points defined in
-            metadata.yaml
-        pod: Used to get access to ``model.pod.set_spec`` to set the container specification
-            for Kubernetes charms.
     """
 
     def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
         self._cache = _ModelCache(backend)
         self._backend = backend
-        self.unit = self.get_unit(self._backend.unit_name)
-        self.app = self.unit.app
-        self.relations = RelationMapping(meta.relations, self.unit, self._backend, self._cache)
-        self.config = ConfigData(self._backend)
-        self.resources = Resources(list(meta.resources), self._backend)
-        self.pod = Pod(self._backend)
-        self.storages = StorageMapping(list(meta.storages), self._backend)
+        self._unit = self.get_unit(self._backend.unit_name)
+        self._relations = RelationMapping(meta.relations, self.unit, self._backend, self._cache)
+        self._config = ConfigData(self._backend)
+        self._resources = Resources(list(meta.resources), self._backend)
+        self._pod = Pod(self._backend)
+        self._storages = StorageMapping(list(meta.storages), self._backend)
         self._bindings = BindingMapping(self._backend)
 
+    @property
+    def unit(self) -> 'Unit':
+        """A :class:`Unit` that represents the unit that is running this code (eg yourself)"""
+        return self._unit
+
+    @property
+    def app(self):
+        """A :class:`Application` that represents the application this unit is a part of."""
+        return self._unit.app
+
+    @property
+    def relations(self) -> 'RelationMapping':
+        """Mapping of endpoint to list of :class:`Relation`
+
+        Answers the question "what am I currently related to".
+        See also :meth:`.get_relation`.
+        """
+        return self._relations
+
+    @property
+    def config(self) -> 'ConfigData':
+        """Return a mapping of config for the current application."""
+        return self._config
+
+    @property
+    def resources(self) -> 'Resources':
+        """Access to resources for this charm.
+
+        Use ``model.resources.fetch(resource_name)`` to get the path on disk
+        where the resource can be found.
+        """
+        return self._resources
+
+    @property
+    def storages(self) -> 'StorageMapping':
+        """Mapping of storage_name to :class:`Storage` as defined in metadata.yaml"""
+        return self._storages
+
+    @property
+    def pod(self) -> 'Pod':
+        """Use ``model.pod.set_spec`` to set the container specification for Kubernetes charms."""
+        return self._pod
+
     @property
     def name(self) -> str:
         """Return the name of the Model that this unit is running in.


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index b7519c4..35c3010 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -55,6 +55,14 @@ class TestModel(unittest.TestCase):
         self.assertIs(self.model.app, self.model.unit.app)
         self.assertIsNone(self.model.name)
 
+    def test_unit_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.unit = object()
+
+    def test_app_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.app = object()
+
     def test_model_name_from_backend(self):
         self.harness.set_model_name('default')
         m = ops.model.Model(ops.charm.CharmMeta(), self.harness._backend)
@@ -83,6 +91,10 @@ class TestModel(unittest.TestCase):
             ('relation_list', rel_app2),
         ])
 
+    def test_relations_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.relations = {}
+
     def test_get_relation(self):
         # one relation on db1
         # two relations on db0
@@ -403,6 +415,10 @@ class TestModel(unittest.TestCase):
 
         self.assertBackendCalls([('config_get',)])
 
+    def test_config_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.config = {}
+
     def test_is_leader(self):
         relation_id = self.harness.add_relation('db1', 'remoteapp1')
         self.harness.add_relation_unit(relation_id, 'remoteapp1/0')
@@ -457,6 +473,10 @@ class TestModel(unittest.TestCase):
         self.assertEqual(self.harness.model.resources.fetch('foo').name, 'foo.txt')
         self.assertEqual(self.harness.model.resources.fetch('bar').name, 'bar.txt')
 
+    def test_resources_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.resources = object()
+
     def test_pod_spec(self):
         self.harness.set_leader(True)
         self.harness.model.pod.set_spec({'foo': 'bar'})
@@ -470,6 +490,10 @@ class TestModel(unittest.TestCase):
         with self.assertRaises(ops.model.ModelError):
             self.harness.model.pod.set_spec({'foo': 'bar'})
 
+    def test_pod_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.pod = object()
+
     def test_base_status_instance_raises(self):
         with self.assertRaises(TypeError):
             ops.model.StatusBase('test')
@@ -697,6 +721,10 @@ class TestModel(unittest.TestCase):
             with self.assertRaises(TypeError):
                 model.storages.request('data', count_v)
 
+    def test_storages_immutable(self):
+        with self.assertRaises(AttributeError):
+            self.model.storages = {}
+
     def resetBackendCalls(self):
         self.harness._get_backend_calls(reset=True)