Repository: canonical/operator
Pull request #67 (2019-12-04 15:48:40)

Make Handle attributes read-only
Fixup for code TODO: Handle attributes really should not be changing at runtime, so this makes them read-only and pre-computes the path rather than doing it on demand.

dshcherb: I think we need a simple test to make sure they are read-only in case somebody changes this in the future by accident.

Patch:
diff --git a/op/charm.py b/op/charm.py
index b185848..a4edbaf 100755
--- a/op/charm.py
+++ b/op/charm.py
@@ -139,7 +139,7 @@ class CharmBase(Object):
             self.on.define_event(f'{relation_name}_relation_departed', RelationDepartedEvent)
             self.on.define_event(f'{relation_name}_relation_broken', RelationBrokenEvent)
 
-        for storage_name in self.framework.meta.storage:
+        for storage_name in self.framework.meta.storages:
             storage_name = storage_name.replace('-', '_')
             self.on.define_event(f'{storage_name}_storage_attached', StorageAttachedEvent)
             self.on.define_event(f'{storage_name}_storage_detaching', StorageDetachingEvent)
@@ -183,8 +183,8 @@ class CharmMeta:
         self.relations.update(self.requires)
         self.relations.update(self.provides)
         self.relations.update(self.peers)
-        self.storage = {name: StorageMeta(name, store)
-                        for name, store in raw.get('storage', {}).items()}
+        self.storages = {name: StorageMeta(name, storage)
+                         for name, storage in raw.get('storage', {}).items()}
         self.resources = {name: ResourceMeta(name, res)
                           for name, res in raw.get('resources', {}).items()}
         self.payloads = {name: PayloadMeta(name, payload)
diff --git a/op/framework.py b/op/framework.py
index 7c51fee..491e118 100755
--- a/op/framework.py
+++ b/op/framework.py
@@ -26,9 +26,19 @@ class Handle:
     def __init__(self, parent, kind, key):
         if parent and not isinstance(parent, Handle):
             parent = parent.handle
-        self.parent = parent
-        self.kind = kind
-        self.key = key
+        self._parent = parent
+        self._kind = kind
+        self._key = key
+        if parent:
+            if key:
+                self._path = f"{parent}/{kind}[{key}]"
+            else:
+                self._path = f"{parent}/{kind}"
+        else:
+            if key:
+                self._path = f"{kind}[{key}]"
+            else:
+                self._path = f"{kind}"
 
     def nest(self, kind, key):
         return Handle(self, kind, key)
@@ -42,19 +52,21 @@ class Handle:
     def __str__(self):
         return self.path
 
+    @property
+    def parent(self):
+        return self._parent
+
+    @property
+    def kind(self):
+        return self._kind
+
+    @property
+    def key(self):
+        return self._key
+
     @property
     def path(self):
-        # TODO Cache result and either clear cache when attributes change, or make it read-only.
-        if self.parent:
-            if self.key:
-                return f"{self.parent}/{self.kind}[{self.key}]"
-            else:
-                return f"{self.parent}/{self.kind}"
-        else:
-            if self.key:
-                return f"{self.kind}[{self.key}]"
-            else:
-                return f"{self.kind}"
+        return self._path
 
     @classmethod
     def from_path(cls, path):
diff --git a/op/model.py b/op/model.py
index 231c42c..b27c0d9 100644
--- a/op/model.py
+++ b/op/model.py
@@ -23,6 +23,7 @@ class Model:
         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)
 
     def get_relation(self, relation_name, relation_id=None):
         """Get a specific Relation instance.
@@ -423,6 +424,56 @@ class Pod:
         self._backend.pod_spec_set(spec, k8s_resources)
 
 
+class StorageMapping(Mapping):
+    """Map of storage names to lists of Storage instances."""
+
+    def __init__(self, storage_names, backend):
+        self._backend = backend
+        self._storage_map = {storage_name: None for storage_name in storage_names}
+
+    def __contains__(self, key):
+        return key in self._storage_map
+
+    def __len__(self):
+        return len(self._storage_map)
+
+    def __iter__(self):
+        return iter(self._storage_map)
+
+    def __getitem__(self, storage_name):
+        storage_list = self._storage_map[storage_name]
+        if storage_list is None:
+            storage_list = self._storage_map[storage_name] = []
+            for storage_id in self._backend.storage_list(storage_name):
+                storage_list.append(Storage(storage_name, storage_id, self._backend))
+        return storage_list
+
+    def request(self, storage_name, count=1):
+        """Requests new storage instances of a given name.
+
+        Uses storage-add tool to request additional storage. Juju will notify the unit
+        via <storage-name>-storage-attached events when it becomes available.
+        """
+        if storage_name not in self._storage_map:
+            raise ModelError(f'cannot add storage with {storage_name} as it is not present in the charm metadata')
+        self._backend.storage_add(storage_name, count)
+
+
+class Storage:
+
+    def __init__(self, storage_name, storage_id, backend):
+        self.name = storage_name
+        self.id = storage_id
+        self._backend = backend
+        self._location = None
+
+    @property
+    def location(self):
+        if self._location is None:
+            self._location = Path(self._backend.storage_get(f'{self.name}/{self.id}', "location"))
+        return self._location
+
+
 class ModelError(Exception):
     pass
 
@@ -490,7 +541,7 @@ class ModelBackend:
 
     def relation_get(self, relation_id, member_name, is_app):
         if not isinstance(is_app, bool):
-            raise RuntimeError('is_app parameter to relation_get must be a boolean')
+            raise TypeError('is_app parameter to relation_get must be a boolean')
 
         try:
             return self._run('relation-get', '-r', str(relation_id), '-', member_name, f'--app={is_app}', return_output=True, use_json=True)
@@ -501,7 +552,7 @@ class ModelBackend:
 
     def relation_set(self, relation_id, key, value, is_app):
         if not isinstance(is_app, bool):
-            raise RuntimeError('is_app parameter to relation_set must be a boolean')
+            raise TypeError('is_app parameter to relation_set must be a boolean')
 
         try:
             return self._run('relation-set', '-r', str(relation_id), f'{key}={value}', f'--app={is_app}')
@@ -556,5 +607,16 @@ class ModelBackend:
         app -- A boolean indicating whether the status should be set for a unit or an application.
         """
         if not isinstance(is_app, bool):
-            raise RuntimeError('is_app parameter must be boolean')
+            raise TypeError('is_app parameter must be boolean')
         return self._run('status-set', f'--application={is_app}', status, message)
+
+    def storage_list(self, name):
+        return [int(s.split('/')[1]) for s in self._run('storage-list', name, return_output=True, use_json=True)]
+
+    def storage_get(self, storage_name_id, attribute):
+        return self._run('storage-get', '-s', storage_name_id, attribute, return_output=True, use_json=True)
+
+    def storage_add(self, name, count=1):
+        if not isinstance(count, int) or isinstance(count, bool):
+            raise TypeError(f'storage count must be integer, got: {count} ({type(count)})')
+        self._run('storage-add', f'{name}={count}')


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index ee671cf..7c77077 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -174,10 +174,10 @@ class TestCharm(unittest.TestCase):
             },
         })
 
-        self.assertIsNone(self.meta.storage['stor1'].multiple_range)
-        self.assertEqual(self.meta.storage['stor2'].multiple_range, (2, 2))
-        self.assertEqual(self.meta.storage['stor3'].multiple_range, (2, None))
-        self.assertEqual(self.meta.storage['stor-4'].multiple_range, (2, 4))
+        self.assertIsNone(self.meta.storages['stor1'].multiple_range)
+        self.assertEqual(self.meta.storages['stor2'].multiple_range, (2, 2))
+        self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None))
+        self.assertEqual(self.meta.storages['stor-4'].multiple_range, (2, 4))
 
         charm = MyCharm(self.create_framework(), None)
 
diff --git a/test/test_framework.py b/test/test_framework.py
index ee2c6f0..a1feffb 100755
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -35,6 +35,17 @@ class TestFramework(unittest.TestCase):
             self.assertEqual(str(handle), path)
             self.assertEqual(Handle.from_path(path), handle)
 
+    def test_handle_attrs_readonly(self):
+        handle = Handle(None, 'kind', 'key')
+        with self.assertRaises(AttributeError):
+            handle.parent = 'foo'
+        with self.assertRaises(AttributeError):
+            handle.kind = 'foo'
+        with self.assertRaises(AttributeError):
+            handle.key = 'foo'
+        with self.assertRaises(AttributeError):
+            handle.path = 'foo'
+
     def test_restore_unknown(self):
         framework = self.create_framework()
 
diff --git a/test/test_model.py b/test/test_model.py
index 5f53470..7c02dfc 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -288,10 +288,10 @@ class TestModel(unittest.TestCase):
 
         # Invalid types for is_app.
         for is_app_v in [None, 1, 2.0, 'a', b'beef']:
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(TypeError):
                 self.backend.relation_set(1, 'fookey', 'barval', is_app=is_app_v)
 
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(TypeError):
                 self.backend.relation_get(1, 'fooentity', is_app=is_app_v)
 
     def test_relation_data_type_check(self):
@@ -545,7 +545,7 @@ class TestModel(unittest.TestCase):
         self.backend = op.model.ModelBackend()
 
         for is_app_v in [None, 1, 2.0, 'a', b'beef', object]:
-            with self.assertRaises(RuntimeError):
+            with self.assertRaises(TypeError):
                 self.backend.status_set(op.model.ActiveStatus, is_app=is_app_v)
 
     def test_remote_unit_status(self):
@@ -596,6 +596,59 @@ class TestModel(unittest.TestCase):
             ['relation-list', '-r', '4', '--format=json'],
         ])
 
+    def test_storage(self):
+        meta = op.charm.CharmMeta()
+        meta.storages = {'disks': None, 'data': None}
+        self.model = op.model.Model('myapp/0', meta, self.backend)
+
+        fake_script(self, 'storage-list', """[ "$1" = disks ] && echo '["disks/0", "disks/1"]' || echo '[]'""")
+        fake_script(self, 'storage-get',
+                    """
+                    if [ "$2" = disks/0 ]; then
+                      echo '"/var/srv/disks/0"'
+                    elif [ "$2" = disks/1 ]; then
+                      echo '"/var/srv/disks/1"'
+                    else
+                      exit 2
+                    fi
+                    """)
+        fake_script(self, 'storage-add', '')
+
+        self.assertEqual(len(self.model.storages), 2)
+        self.assertEqual(self.model.storages.keys(), meta.storages.keys())
+        self.assertIn('disks', self.model.storages)
+        test_cases = {
+            0: {'name': 'disks', 'location': pathlib.Path('/var/srv/disks/0')},
+            1: {'name': 'disks', 'location': pathlib.Path('/var/srv/disks/1')},
+        }
+        for storage in self.model.storages['disks']:
+            self.assertEqual(storage.name, 'disks')
+            self.assertIn(storage.id, test_cases)
+            self.assertEqual(storage.name, test_cases[storage.id]['name'])
+            self.assertEqual(storage.location, test_cases[storage.id]['location'])
+
+        self.assertEqual(fake_script_calls(self, clear=True), [
+            ['storage-list', 'disks', '--format=json'],
+            ['storage-get', '-s', 'disks/0', 'location', '--format=json'],
+            ['storage-get', '-s', 'disks/1', 'location', '--format=json'],
+        ])
+
+        self.assertSequenceEqual(self.model.storages['data'], [])
+        self.model.storages.request('data', count=3)
+        self.assertEqual(fake_script_calls(self), [
+            ['storage-list', 'data', '--format=json'],
+            ['storage-add', 'data=3'],
+        ])
+
+        # Try to add storage not present in charm metadata.
+        with self.assertRaises(op.model.ModelError):
+            self.model.storages.request('deadbeef')
+
+        # Invalid count parameter types.
+        for count_v in [None, False, 2.0, 'a', b'beef', object]:
+            with self.assertRaises(TypeError):
+                self.model.storages.request('data', count_v)
+
 
 class TestModelBackend(unittest.TestCase):
 
@@ -661,6 +714,39 @@ class TestModelBackend(unittest.TestCase):
             with self.assertRaises(TypeError):
                 case()
 
+    def storage_tool_errors(self):
+        test_cases = [(
+            lambda: fake_script(self, 'storage-list', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.storage_list('foobar'),
+            op.model.ModelError,
+            [['storage-list', 'foobar', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'storage-get', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.storage_get('foobar', 'someattr'),
+            op.model.ModelError,
+            [['storage-get', '-s', 'foobar', 'someattr', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'storage-add', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.storage_add('foobar', count=2),
+            op.model.ModelError,
+            [['storage-get', '-s', 'foobar', 'someattr', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'storage-add', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.storage_add('foobar', count=object),
+            TypeError,
+            [['storage-get', '-s', 'foobar', 'someattr', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'storage-add', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.storage_add('foobar', count=True),
+            TypeError,
+            [['storage-get', '-s', 'foobar', 'someattr', '--format=json']],
+        )]
+        for do_fake, run, exception, calls in test_cases:
+            do_fake()
+            with self.assertRaises(exception):
+                run()
+            self.assertEqual(fake_script_calls(self, clear=True), calls)
+
 
 def fake_script(test_case, name, content):
     if not hasattr(test_case, 'fake_script_path'):