Merge lp:~johnsca/charm-helpers/provide-data-fix into lp:charm-helpers

[Cory Johns]

Fix issues with provide_data in services framework:

  * Providing data is gated on data being complete, with no option for sending partial data (necessary for two-way communication)

  * Providing data is gated on current hook (no way to, e.g., send new data out in response to a config-changed hook)

  * Providing data happens before data_ready with no way of telling if data_ready was triggered

I also fixed an issue with update-revno where the generated version.py was failing lint.

[lp:~johnsca/charm-helpers/provide-data-fix]

377. By Cory Johns on 2015-05-20

Removed class type check

[Nick Moffitt (nick-moffitt)]

review: Approve

[Tim Van Steenburgh (tvansteenburgh)]

+1 LGTM.

review: Approve

=== modified file 'VERSION'
--- VERSION	2015-04-27 11:27:24 +0000
+++ VERSION	2015-05-20 14:52:28 +0000
@@ -1,1 +1,1 @@
-0.3.1
+0.3.2

=== modified file 'charmhelpers/core/services/base.py'
--- charmhelpers/core/services/base.py	2015-01-22 13:56:24 +0000
+++ charmhelpers/core/services/base.py	2015-05-20 14:52:28 +0000
@@ -15,8 +15,8 @@
 # along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import json
+from inspect import getargspec
 from collections import Iterable, OrderedDict
 
 from charmhelpers.core import host
@@ -132,8 +132,8 @@
         if hook_name == 'stop':
             self.stop_services()
         else:
+            self.reconfigure_services()
             self.provide_data()
-            self.reconfigure_services()
         cfg = hookenv.config()
         if cfg.implicit_save:
             cfg.save()
@@ -145,15 +145,36 @@
         A provider must have a `name` attribute, which indicates which relation
         to set data on, and a `provide_data()` method, which returns a dict of
         data to set.
+
+        The `provide_data()` method can optionally accept two parameters:
+
+          * ``remote_service`` The name of the remote service that the data will
+            be provided to.  The `provide_data()` method will be called once
+            for each connected service (not unit).  This allows the method to
+            tailor its data to the given service.
+          * ``service_ready`` Whether or not the service definition had all of
+            its requirements met, and thus the ``data_ready`` callbacks run.
+
+        Note that the ``provided_data`` methods are now called **after** the
+        ``data_ready`` callbacks are run.  This gives the ``data_ready`` callbacks
+        a chance to generate any data necessary for the providing to the remote
+        services.
         """
-        hook_name = hookenv.hook_name()
-        for service in self.services.values():
+        for service_name, service in self.services.items():
+            service_ready = self.is_ready(service_name)
             for provider in service.get('provided_data', []):
-                if re.match(r'{}-relation-(joined|changed)'.format(provider.name), hook_name):
-                    data = provider.provide_data()
-                    _ready = provider._is_ready(data) if hasattr(provider, '_is_ready') else data
-                    if _ready:
-                        hookenv.relation_set(None, data)
+                for relid in hookenv.relation_ids(provider.name):
+                    units = hookenv.related_units(relid)
+                    if not units:
+                        continue
+                    remote_service = units[0].split('/')[0]
+                    argspec = getargspec(provider.provide_data)
+                    if len(argspec.args) > 1:
+                        data = provider.provide_data(remote_service, service_ready)
+                    else:
+                        data = provider.provide_data()
+                    if data:
+                        hookenv.relation_set(relid, data)
 
     def reconfigure_services(self, *service_names):
         """

=== modified file 'scripts/update-revno'
--- scripts/update-revno	2013-05-11 20:24:29 +0000
+++ scripts/update-revno	2015-05-20 14:52:28 +0000
@@ -6,6 +6,6 @@
     REVNO="${REVNO}+"
 fi
 cat << EOF > charmhelpers/version.py
-CHARMHELPERS_VERSION='${VERSION}'
-CHARMHELPERS_BZRREVNO='${REVNO}'
+CHARMHELPERS_VERSION = '${VERSION}'
+CHARMHELPERS_BZRREVNO = '${REVNO}'
 EOF

=== modified file 'tests/core/test_services.py'
--- tests/core/test_services.py	2015-04-03 14:53:10 +0000
+++ tests/core/test_services.py	2015-05-20 14:52:28 +0000
@@ -378,49 +378,56 @@
         assert not manager.was_ready('bar')
 
     @mock.patch.object(services.base.hookenv, 'relation_set')
-    @mock.patch.object(services.base.hookenv, 'hook_name')
-    def test_provide_data_no_match(self, hook_name, relation_set):
-        provider = mock.Mock()
-        provider.name = 'provided'
-        manager = services.ServiceManager([
-            {'service': 'service', 'provided_data': [provider]}
-        ])
-        hook_name.return_value = 'not-provided-relation-joined'
-        manager.provide_data()
-        assert not provider.provide_data.called
-
-        hook_name.return_value = 'provided-relation-broken'
-        manager.provide_data()
-        assert not provider.provide_data.called
-
-    @mock.patch.object(services.base.hookenv, 'relation_set')
-    @mock.patch.object(services.base.hookenv, 'hook_name')
-    def test_provide_data_not_ready(self, hook_name, relation_set):
-        provider = mock.Mock()
-        provider.name = 'provided'
-        data = provider.provide_data.return_value = {'data': True}
-        provider._is_ready.return_value = False
-        manager = services.ServiceManager([
-            {'service': 'service', 'provided_data': [provider]}
-        ])
-        hook_name.return_value = 'provided-relation-joined'
-        manager.provide_data()
-        assert not relation_set.called
-        provider._is_ready.assert_called_once_with(data)
-
-    @mock.patch.object(services.base.hookenv, 'relation_set')
-    @mock.patch.object(services.base.hookenv, 'hook_name')
-    def test_provide_data_ready(self, hook_name, relation_set):
-        provider = mock.Mock()
-        provider.name = 'provided'
-        data = provider.provide_data.return_value = {'data': True}
-        provider._is_ready.return_value = True
-        manager = services.ServiceManager([
-            {'service': 'service', 'provided_data': [provider]}
-        ])
-        hook_name.return_value = 'provided-relation-changed'
-        manager.provide_data()
-        relation_set.assert_called_once_with(None, data)
+    @mock.patch.object(services.base.hookenv, 'related_units')
+    @mock.patch.object(services.base.hookenv, 'relation_ids')
+    def test_provide_data_no_match(self, relation_ids, related_units, relation_set):
+        provider = mock.Mock()
+        provider.name = 'provided'
+        manager = services.ServiceManager([
+            {'service': 'service', 'provided_data': [provider]}
+        ])
+        relation_ids.return_value = []
+        manager.provide_data()
+        assert not provider.provide_data.called
+        relation_ids.assert_called_once_with('provided')
+
+    @mock.patch.object(services.base.hookenv, 'relation_set')
+    @mock.patch.object(services.base.hookenv, 'related_units')
+    @mock.patch.object(services.base.hookenv, 'relation_ids')
+    def test_provide_data_not_ready(self, relation_ids, related_units, relation_set):
+        provider = mock.Mock()
+        provider.name = 'provided'
+        pd = mock.Mock()
+        data = pd.return_value = {'data': True}
+        provider.provide_data = lambda remote_service, service_ready: pd(remote_service, service_ready)
+        manager = services.ServiceManager([
+            {'service': 'service', 'provided_data': [provider]}
+        ])
+        manager.is_ready = mock.Mock(return_value=False)
+        relation_ids.return_value = ['relid']
+        related_units.return_value = ['service/0']
+        manager.provide_data()
+        relation_set.assert_called_once_with('relid', data)
+        pd.assert_called_once_with('service', False)
+
+    @mock.patch.object(services.base.hookenv, 'relation_set')
+    @mock.patch.object(services.base.hookenv, 'related_units')
+    @mock.patch.object(services.base.hookenv, 'relation_ids')
+    def test_provide_data_ready(self, relation_ids, related_units, relation_set):
+        provider = mock.Mock()
+        provider.name = 'provided'
+        pd = mock.Mock()
+        data = pd.return_value = {'data': True}
+        provider.provide_data = lambda remote_service, service_ready: pd(remote_service, service_ready)
+        manager = services.ServiceManager([
+            {'service': 'service', 'provided_data': [provider]}
+        ])
+        manager.is_ready = mock.Mock(return_value=True)
+        relation_ids.return_value = ['relid']
+        related_units.return_value = ['service/0']
+        manager.provide_data()
+        relation_set.assert_called_once_with('relid', data)
+        pd.assert_called_once_with('service', True)
 
 
 class TestRelationContext(unittest.TestCase):