Merge lp:~wgrant/juju-deployer/diff-all-options into lp:juju-deployer

[William Grant]

Fix service config diffs to include all keys and respect charm defaults.

Fix service config diffs to include all keys and respect charm defaults.

Cherrypicked and adapted from jjo's multiple relations branch.

[Tim Van Steenburgh (tvansteenburgh)]

LGTM, thanks.

review: Approve

=== modified file 'deployer/action/diff.py'
--- deployer/action/diff.py	2016-07-12 23:43:08 +0000
+++ deployer/action/diff.py	2016-12-14 10:48:27 +0000
@@ -122,28 +122,39 @@
             delta['modified'][cs] = mod
         return delta
 
-    def _diff_service(self, e_s, d_s, charm):
+    def _diff_service(self, env_service, dep_service, charm):
         mod = {}
-        d_sc = parse_constraints(d_s.get('constraints', ''))
+
+        # Start by diffing the constraints.
+        dep_constraints = parse_constraints(dep_service.get('constraints', ''))
         # 'tags' is a special case, as it can be multi-valued: convert to list
-        # if cfg one is a string
-        if isinstance(d_sc.get('tags'), six.string_types):
-            d_sc['tags'] = [d_sc['tags']]
-        if d_sc != e_s['constraints']:
-            mod['env-constraints'] = e_s['constraints']
-            mod['cfg-constraints'] = d_sc
-        for k, v in d_s.get('options', {}).items():
-            # Deploy options not known to the env may originate
-            # from charm version delta or be an invalid config.
-            if k not in e_s['options']:
-                continue
-            e_v = e_s['options'].get(k, {}).get('value')
-            if e_v != v:
-                mod.setdefault('env-config', {}).update({k: e_v})
-                mod.setdefault('cfg-config', {}).update({k: v})
+        # if the deployment specifies just a string
+        if isinstance(dep_constraints.get('tags'), six.string_types):
+            dep_constraints['tags'] = [dep_constraints['tags']]
+        if dep_constraints != env_service['constraints']:
+            mod['env-constraints'] = env_service['constraints']
+            mod['cfg-constraints'] = dep_constraints
+
+        # Now collect all the options from both services and diff them.
+        all_options = set(dep_service.get('options', {}).keys() +
+                          env_service.get('options', {}).keys())
+        for key in all_options:
+            dep_value = dep_service.get('options', {}).get(key)
+            # Assume the charm default if the deployment specifies no value.
+            if dep_value is None:
+                dep_value = charm.config.get(key, {}).get('default', None)
+            env_value = env_service['options'].get(key, {}).get('value')
+            if env_value != dep_value:
+                mod.setdefault('env-config', {}).update({key: env_value})
+                mod.setdefault('cfg-config', {}).update({key: dep_value})
+
+        # Finally, compare the unit counts if appropriate.
         if not charm or not charm.is_subordinate():
-            if e_s['unit_count'] != d_s.get('num_units', 1):
-                mod['num_units'] = e_s['unit_count'] - d_s.get('num_units', 1)
+            # XXX: The num_units default policy should really be
+            # implemented centrally.
+            dep_unit_count = dep_service.get('num_units', 1)
+            if env_service['unit_count'] != dep_unit_count:
+                mod['num_units'] = env_service['unit_count'] - dep_unit_count
         return mod
 
     def do_diff(self):

=== modified file 'deployer/tests/test_diff.py'
--- deployer/tests/test_diff.py	2016-05-07 23:48:30 +0000
+++ deployer/tests/test_diff.py	2016-12-14 10:48:27 +0000
@@ -79,26 +79,24 @@
     def test_diff_config(self):
         dpl = self.get_deployment()
         env = MemoryEnvironment(dpl.name, dpl)
-        env.set_config('blog', {'tuning': 'bare'})
-        diff = Diff(env, dpl, {}).do_diff()
-        mod_blog = diff['services']['modified']['blog']
-        self.assertTrue(mod_blog['env-config']['tuning'] !=
-                        mod_blog['cfg-config']['tuning'])
-        self.assertEquals(mod_blog['env-config']['tuning'], 'bare')
-
-    def test_diff_config_many(self):
-        dpl = self.get_deployment()
-        env = MemoryEnvironment(dpl.name, dpl)
-        env.set_config('blog', {'tuning': 'bare', 'engine': 'duck'})
-        diff = Diff(env, dpl, {}).do_diff()
-        mod_blog = diff['services']['modified']['blog']
-        self.assertEqual(
-            set(mod_blog['env-config'].keys()),
-            set(['tuning', 'engine']))
-        self.assertTrue(mod_blog['env-config']['tuning'] !=
-                        mod_blog['cfg-config']['tuning'])
-        self.assertTrue(mod_blog['env-config']['engine'] !=
-                        mod_blog['cfg-config']['engine'])
+        # Reconfigure the environment to have a couple of explicit
+        # changes (tuning and engine), one that differs from the charm
+        # default (debug), and an option that no longer exists in the
+        # charm (obsolete).
+        env.set_config(
+            'blog',
+            {'tuning': 'bare', 'engine': 'duck', 'debug': 'please',
+             'obsolete': 'for ages'})
+        diff = Diff(env, dpl, {}).do_diff()
+        mod_blog = diff['services']['modified']['blog']
+        self.assertEqual(
+            mod_blog['env-config'],
+            {'engine': 'duck', 'tuning': 'bare', 'debug': 'please',
+             'obsolete': 'for ages'})
+        self.assertEqual(
+            mod_blog['cfg-config'],
+            {'engine': 'nginx', 'tuning': 'optimized', 'debug': 'no',
+             'obsolete': None})
 
     def test_diff_constraints(self):
         dpl = self.get_deployment()