cf-charmers team mailing list archive

[Cory Johns <cory.johns@xxxxxxxxxxxxx>]

Cory Johns has proposed merging lp:~johnsca/charm-helpers/multi-unit into lp:~cf-charmers/charm-helpers/cloud-foundry.

Requested reviews:
  Cloud Foundry Charmers (cf-charmers)

For more details, see:
https://code.launchpad.net/~johnsca/charm-helpers/multi-unit/+merge/221801

Refactored RelationContext multi-unit support

Added a little magic to the RelationContext to make it better able to support
multiple units and still DWIM in both cases.  This might be too magical,
though.

https://codereview.appspot.com/96680043/

-- 
https://code.launchpad.net/~johnsca/charm-helpers/multi-unit/+merge/221801
Your team Cloud Foundry Charmers is requested to review the proposed merge of lp:~johnsca/charm-helpers/multi-unit into lp:~cf-charmers/charm-helpers/cloud-foundry.

=== modified file 'charmhelpers/contrib/cloudfoundry/contexts.py'
--- charmhelpers/contrib/cloudfoundry/contexts.py	2014-05-26 18:49:24 +0000
+++ charmhelpers/contrib/cloudfoundry/contexts.py	2014-06-02 22:16:10 +0000
@@ -62,3 +62,13 @@
 class LoggregatorRelation(RelationContext):
     interface = 'loggregator'
     required_keys = ['shared_secret', 'loggregator_address']
+
+
+class EtcdRelation(RelationContext):
+    interface = 'etcd'
+    required_keys = ['hostname', 'port']
+
+
+class CloudControllerRelation(RelationContext):
+    interface = 'cc'
+    required_keys = ['hostname', 'port', 'user', 'password']

=== modified file 'charmhelpers/core/services.py'
--- charmhelpers/core/services.py	2014-05-29 17:23:48 +0000
+++ charmhelpers/core/services.py	2014-06-02 22:16:10 +0000
@@ -88,7 +88,7 @@
             ...         'stop': runit_stop,
             ...     },
             ... ])
-            ... manager.manage()
+            >>> manager.manage()
         """
         self.services = {}
         for service in services or []:
@@ -198,6 +198,33 @@
         return os.path.exists(ready_file)
 
 
+class RelationData(dict):
+    """
+    Convenience class to default to using the unit/0 data
+    if no unit number is specified.
+    """
+    def __init__(self, mapping=None, required_keys=None, **kwargs):
+        super(RelationData, self).__init__(mapping or {}, **kwargs)
+        self.required_keys = set(required_keys or [])
+
+    def _get_default_unit(self):
+        keys = sorted([key for key, value in self.iteritems()
+                      if set(value.keys()).issuperset(self.required_keys)])
+        return dict.get(self, keys[0])
+
+    def __getitem__(self, key):
+        mapping = self
+        if not isinstance(key, int):
+            mapping = self._get_default_unit()
+        return dict.__getitem__(mapping, key)
+
+    def get(self, key, default=None):
+        mapping = self
+        if not isinstance(key, int):
+            mapping = self._get_default_unit()
+        return dict.get(mapping, key, default)
+
+
 class RelationContext(dict):
     """
     Base class for a context generator that gets relation data from juju.
@@ -222,46 +249,51 @@
 
     __nonzero__ = __bool__
 
+    def __repr__(self):
+        return super(RelationContext, self).__repr__()
+
     def is_ready(self):
         """
         Returns True if all of the required_keys are available.
         """
-        return set(self.get(self.interface, {}).keys()).issuperset(set(self.required_keys))
+        return any(set(unit.keys()).issuperset(set(self.required_keys))
+                   for unit in self.get(self.interface, {}).values())
 
     def get_data(self):
         """
-        Retrieve the relation data and store it under `self[self.interface]`.
-
-        If there are more than one units related on the desired interface,
-        then each unit will have its data stored under `self[self.interface][unit_id]`
-        and one of the units with complete information will chosen at random
-        to fill the values at `self[self.interface]`.
-
+        Retrieve the relation data and store it under `self[self.interface][unit_id]`,
+        where `unit_id` is the id (e.g., 'unit/0') of the individual units.
+
+        When accessing the data, if the `unit_id` is omitted (e.g.,
+        `self[self.interface]['foo']`), a default unit is chosen by sorting
+        the `unit_id`s and choosing the first one with complete data.
+
+        Iterating over the keys, values, or items of the relation will iterate
+        over the units, not the items of the default unit.
 
         For example:
 
+            >>> ctx = RelationContext()
             {
-                'foo': 'bar',
-                'unit/0': {
-                    'foo': 'bar',
-                },
-                'unit/1': {
-                    'foo': 'baz',
-                },
+                'unit/0': {'foo': 'baz'},
+                'unit/1': {'foo': 'qux'},
+                'other/0': {'foo': 'bar'},
             }
+            >>> ctx['foo'] == 'bar'
+            True
+            >>> ctx[1]['foo'] == 'qux'
+            True
         """
         if not hookenv.relation_ids(self.interface):
             return
 
-        ns = self.setdefault(self.interface, {})
-        required = set(self.required_keys)
+        ns = self.setdefault(self.interface,
+                             RelationData(required_keys=self.required_keys))
         for rid in hookenv.relation_ids(self.interface):
             for unit in hookenv.related_units(rid):
                 reldata = hookenv.relation_get(rid=rid, unit=unit)
                 unit_ns = ns.setdefault(unit, {})
                 unit_ns.update(reldata)
-                if set(reldata.keys()).issuperset(required):
-                    ns.update(reldata)
 
 
 class ManagerCallback(object):
@@ -316,6 +348,6 @@
 
 
 # Convenience aliases
-template = TemplateCallback
+render_template = template = TemplateCallback
 open_ports = PortManagerCallback()
 close_ports = PortManagerCallback()

=== modified file 'tests/contrib/cloudfoundry/test_render_context.py'
--- tests/contrib/cloudfoundry/test_render_context.py	2014-05-26 18:49:24 +0000
+++ tests/contrib/cloudfoundry/test_render_context.py	2014-06-02 22:16:10 +0000
@@ -22,12 +22,11 @@
                              'nats_user': 'user', 'nats_password': 'password'}
         mrelated.return_value = ['router/0']
         n = contexts.NatsRelation()
-        expected = {'nats': {'nats_port': 1234, 'nats_address': 'host',
-                             'nats_user': 'user', 'nats_password': 'password',
-                             'router/0': {'nats_port': 1234, 'nats_address': 'host',
+        expected = {'nats': {'router/0': {'nats_port': 1234, 'nats_address': 'host',
                                           'nats_user': 'user', 'nats_password': 'password'}}}
         self.assertTrue(bool(n))
         self.assertEqual(n, expected)
+        self.assertEqual(n['nats']['nats_port'], 1234)
 
     @mock.patch('charmhelpers.core.hookenv.related_units')
     @mock.patch('charmhelpers.core.hookenv.relation_ids')
@@ -56,10 +55,10 @@
         mrel.return_value = {'domain': 'example.com'}
         mrelated.return_value = ['router/0']
         n = contexts.RouterRelation()
-        expected = {'router': {'domain': 'example.com',
-                               'router/0': {'domain': 'example.com'}}}
+        expected = {'router': {'router/0': {'domain': 'example.com'}}}
         self.assertTrue(bool(n))
         self.assertEqual(n, expected)
+        self.assertEqual(n['router']['domain'], 'example.com')
 
 
 class TestStoredContext(unittest.TestCase):

=== modified file 'tests/core/test_services.py'
--- tests/core/test_services.py	2014-05-29 17:23:48 +0000
+++ tests/core/test_services.py	2014-06-02 22:16:10 +0000
@@ -290,6 +290,41 @@
         exists.assert_called_once_with('charm_dir/.ready.foo')
 
 
+class TestRelationData(unittest.TestCase):
+    def setUp(self):
+        self.data = services.RelationData({
+            0: {'foo': 1, 'baz': 2},
+            1: {'foo': 3, 'bar': 4},
+            2: {'foo': 5, 'bar': 6},
+        }, required_keys=['foo', 'bar'])
+
+    def test_explicit(self):
+        self.assertEqual(self.data[0]['foo'], 1)
+        self.assertEqual(self.data[0]['baz'], 2)
+        self.assertEqual(self.data[1]['foo'], 3)
+        self.assertEqual(self.data[1]['bar'], 4)
+        self.assertEqual(self.data[2]['foo'], 5)
+        self.assertEqual(self.data[2]['bar'], 6)
+
+    def test_implicit(self):
+        self.assertEqual(self.data['foo'], 3)
+        self.assertEqual(self.data['bar'], 4)
+
+    def test_missing(self):
+        self.assertRaises(KeyError, lambda: self.data['qux'])
+        self.assertRaises(KeyError, lambda: self.data[0]['qux'])
+
+    def test_get(self):
+        self.assertEqual(self.data.get('foo'), 3)
+        self.assertEqual(self.data.get(2), {'foo': 5, 'bar': 6})
+        self.assertEqual(self.data.get('qux'), None)
+        self.assertEqual(self.data.get(3), None)
+
+    def test_get_default(self):
+        self.assertEqual(self.data.get('qux', 5), 5)
+        self.assertEqual(self.data.get(3, {'qux': 5}), {'qux': 5})
+
+
 class TestRelationContext(unittest.TestCase):
     def setUp(self):
         self.context = services.RelationContext()
@@ -329,9 +364,8 @@
         mhookenv.related_units.side_effect = lambda i: [i+'/0']
         mhookenv.relation_get.side_effect = [{'foo': '1'}, {'foo': '2', 'bar': '3'}, {}]
         self.context.get_data()
+        self.assertTrue(self.context.is_ready())
         self.assertEqual(self.context, {'http': {
-            'foo': '2',
-            'bar': '3',
             'nginx/0': {
                 'foo': '1',
             },