Repository: canonical/operator
Pull request #465 (2021-01-08 20:23:50)

Allow Harness.get_relation_data to accept instances
It's tripped me up a few times that you have to pass in the name of the app or unit rather than the instance itself that you use everywhere else, so this allows get_relation_data to handle either.

johnsca: There are several other methods where this would be nice as well, so if I have time I might go through and update them all.  If the general idea is ok, feel free to block this on making them all consistent.
johnsca: @jameinel I think my main issue was that I was used to using the object instances themselves in the charm and interface code to access the data, so that's just what I naturally did in the tests as well.  Then my tests failed with confusing results because instead of rejecting the instances, it just treated them as if there was no data when I expected there to be.

Also, I do find typing strings all of the place more prone to typos and mistakes that, again, will lead to confusing failures rather than being caught by the linter or interpreter. That can be mitigated by assigning the strings to variables, but then it's not really any different than using the instances directly.

One downside of using instances is that it is harder to reference a remote unit or application, since you don't have an easy way to get the instance, so in that case, just being able to say `"my-charm/0"` and `"my-charm/1"` is easier.
johnsca: FWIW, I'm using helpers which do something akin to this in [the loadbalancer-interface functional tests](https://github.com/juju-solutions/loadbalancer-interface/blob/main/tests/functional/test_interface.py). That test should probably be refactored into steps to help with readability, but maybe it can at least give an indication as to why it might be easier to work with model objects directly rather than just names.
benhoyt: @jameinel I'm on the fence about this (though maybe that's my recent statically-typed Go experience speaking).

However, one thing that comes to mind is: we already use many type annotations in the project. If we finish that and get `mypy` running on the project, would this be better solved by using mypy's type checking? That way you'd catch "oops, I should be using `app.name` here" errors at build/type check time rather than runtime.
Vultaire: In my opinion:
* I'm +1 for being more forgiving about input here.  While I agree that strings look simpler, I also agree that typos are more likely an issue.  While there's other ways to address this, as the framework does use Unit/Application objects elsewhere, to me it seems reasonable to accept such here.
* This seems like an issue specifically with the relation-related harness functions, i.e. add_relation, add_relation_unit, get_relation_data and update_relation_data.  And for most of these, it seems a fairly simple change with no real API changes aside from making the type annotations looser.
* There is one small backwards compatibility concern re: add_relation_unit, as it uses "remote_unit_name", and with this rework it'd make better sense for that to be "remote_unit".  That's break tests which pass in by keyword argument if we rename that, although perhaps that's a fairly minor concern.
jnsgruk: Oops, that merge commit broke the tests. I did some work on this locally to rectify but cannot push to the branch, [this patch](https://gist.github.com/jnsgruk/ba2a64a1b642c45c6cec5a237c5057f5) brings this PR up to date, and makes the test suite pass etc. taking into account changes that have happened since!

Patch:
diff --git a/ops/charm.py b/ops/charm.py
index 4fa7955..0420c3e 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -895,6 +895,7 @@ class ContainerMeta:
         name: Name of container (key in the YAML)
         mounts: :class:`ContainerStorageMeta` mounts available to the container
     """
+
     def __init__(self, name, raw):
         self.name = name
         self._mounts = {}
@@ -956,6 +957,7 @@ class ContainerStorageMeta:
     `location` will not be an accessible attribute, as it would not be possible to determine
     which mount point was desired, and `locations` should be iterated over.
     """
+
     def __init__(self, storage, location):
         self.storage = storage
         self._locations = [location]
diff --git a/ops/model.py b/ops/model.py
index 47640cf..b757fa4 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -296,7 +296,7 @@ class Unit:
         self._is_our_unit = self.name == self._backend.unit_name
         self._status = None
 
-        if self._is_our_unit:
+        if self._is_our_unit and hasattr(meta, "containers"):
             self._containers = ContainerMapping(meta.containers, backend)
 
     def _invalidate(self):
@@ -1052,6 +1052,7 @@ class Container:
     Attributes:
         name: The name of the container from metadata.yaml (eg, 'postgres').
     """
+
     def __init__(self, name, backend, pebble_client=None):
         self.name = name
 


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 11dd67e..343e0f2 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -37,6 +37,13 @@ from ops._private import yaml
 # pass in a file-like object or the string directly.
 OptionalYAML = typing.Optional[typing.Union[str, typing.TextIO]]
 
+
+# An instance of an Application or Unit, or the name of either.
+# This is done here to avoid a scoping issue with the `model` property
+# of the Harness class below.
+AppUnitOrName = typing.Union[str, model.Application, model.Unit]
+
+
 # CharmType represents user charms that are derived from CharmBase.
 CharmType = typing.TypeVar('CharmType', bound=charm.CharmBase)
 
@@ -72,6 +79,7 @@ class Harness(typing.Generic[CharmType]):
             config.yaml. If not supplied, we will look for a 'config.yaml' file in the
             parent directory of the Charm.
     """
+
     def __init__(
             self,
             charm_cls: typing.Type[CharmType],
@@ -578,7 +586,7 @@ class Harness(typing.Generic[CharmType]):
             raise ValueError('Invalid Unit Name')
         self._charm.on[rel_name].relation_departed.emit(relation, app, unit)
 
-    def get_relation_data(self, relation_id: int, app_or_unit: str) -> typing.Mapping:
+    def get_relation_data(self, relation_id: int, app_or_unit: AppUnitOrName) -> typing.Mapping:
         """Get the relation data bucket for a single app or unit in a given relation.
 
         This ignores all of the safety checks of who can and can't see data in relations (eg,
@@ -587,13 +595,15 @@ class Harness(typing.Generic[CharmType]):
 
         Args:
             relation_id: The relation whose content we want to look at.
-            app_or_unit: The name of the application or unit whose data we want to read
+            app_or_unit: An Application or Unit instance, or its name, whose data we want to read
         Return:
             A dict containing the relation data for `app_or_unit` or None.
 
         Raises:
             KeyError: if relation_id doesn't exist
         """
+        if hasattr(app_or_unit, 'name'):
+            app_or_unit = app_or_unit.name
         return self._backend._relation_data[relation_id].get(app_or_unit, None)
 
     def get_pod_spec(self) -> (typing.Mapping, typing.Mapping):
diff --git a/test/test_charm.py b/test/test_charm.py
index 556c606..8c23c93 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -473,8 +473,8 @@ containers:
         self.assertIsInstance(meta.containers['test1'], ContainerMeta)
         self.assertIsInstance(meta.containers['test1'].mounts["data"], ContainerStorageMeta)
         self.assertEqual(
-                meta.containers['test1'].mounts["data"].locations[0],
-                '/test/storagemount')
+            meta.containers['test1'].mounts["data"].locations[0],
+            '/test/storagemount')
         self.assertEqual(meta.containers['test1'].mounts["data"].locations[1], '/test/otherdata')
 
         with self.assertRaises(RuntimeError):
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 6f31542..7715f6c 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -469,13 +469,13 @@ services:
                                      raw={
                                           "override": "replace",
                                           "command": "echo foo"
-                                         })
+                                     })
         old_services = {"foo": old_service}
         self.assertEqual(plan.services, old_services)
 
         services_as_dict = {
             "foo": {"override": "replace", "command": "echo foo"}
-            }
+        }
         self.assertEqual(plan.services, services_as_dict)
 
 
diff --git a/test/test_testing.py b/test/test_testing.py
index 848d421..663f8b3 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -32,17 +32,16 @@ from ops.framework import (
     Object,
 )
 from ops.model import (
+    Application,
     ActiveStatus,
     MaintenanceStatus,
     UnknownStatus,
     ModelError,
     RelationNotFoundError,
     _ModelBackend,
+    Unit,
 )
-from ops.testing import (
-    Harness,
-    _TestingPebbleClient,
-)
+from ops.testing import Harness, _TestingPebbleClient
 
 
 class TestHarness(unittest.TestCase):
@@ -548,12 +547,13 @@ class TestHarness(unittest.TestCase):
               }}])
 
     def test_get_relation_data(self):
-        harness = Harness(CharmBase, meta='''
+        charm_meta = '''
             name: test-app
             requires:
                 db:
                     interface: pgsql
-            ''')
+        '''
+        harness = Harness(CharmBase, meta=charm_meta)
         self.addCleanup(harness.cleanup)
         rel_id = harness.add_relation('db', 'postgresql')
         harness.update_relation_data(rel_id, 'postgresql', {'remote': 'data'})
@@ -565,6 +565,16 @@ class TestHarness(unittest.TestCase):
             # unknown relation id
             harness.get_relation_data(99, 'postgresql')
 
+        meta = yaml.safe_load(charm_meta)
+        t_app = Application('test-app', meta, harness._backend, None)
+        t_unit0 = Unit('test-app/0', meta, harness._backend, {Application: t_app})
+        t_unit1 = Unit('test-app/1', meta, harness._backend, {Application: t_app})
+        self.assertEqual(harness.get_relation_data(rel_id, t_app), {})
+        self.assertEqual(harness.get_relation_data(rel_id, t_unit0), {})
+        self.assertEqual(harness.get_relation_data(rel_id, t_unit1), None)
+        pg_app = Application('postgresql', meta, harness._backend, None)
+        self.assertEqual(harness.get_relation_data(rel_id, pg_app), {'remote': 'data'})
+
     def test_create_harness_twice(self):
         metadata = '''
             name: my-charm