Merge lp:~jjo/juju-deployer/diff-usability-fixes_and_diff-tests into lp:juju-deployer

[JuanJo Ciarlante]

implement test_diff (using augmented env/mem.py).

[JuanJo Ciarlante (jjo)]

To ease review, I stacked this over lp:~jjo/juju-deployer/diff-usability-fixes

[Kapil Thangavelu (hazmat)]

lgtm

review: Approve

=== modified file 'deployer/action/diff.py'
--- deployer/action/diff.py	2013-11-21 03:19:56 +0000
+++ deployer/action/diff.py	2013-11-29 22:24:49 +0000
@@ -122,10 +122,10 @@
 
     def _diff_service(self, e_s, d_s, charm):
         mod = {}
-        if 'constraints' in d_s:
-            d_sc = parse_constraints(d_s['constraints'])
-            if d_sc != e_s['constraints']:
-                mod['constraints'] = e_s['constraints']
+        d_sc = parse_constraints(d_s.get('constraints',''))
+        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.
@@ -133,17 +133,22 @@
                 continue
             e_v = e_s['options'].get(k, {}).get('value')
             if e_v != v:
-                mod['config'] = {k: e_v}
+                mod.setdefault('env-config', {}).update({k: e_v})
+                mod.setdefault('cfg-config', {}).update({k: v})
         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)
         return mod
 
-    def run(self):
+    def do_diff(self):
         self.start_time = time.time()
+        self.deployment.resolve()
         self.env.connect()
         self.env_status = self.env.status()
         self.load_env()
-        delta = self.get_delta()
-        if delta:
-            print yaml_dump(delta)
+        return self.get_delta()
+
+    def run(self):
+        diff = self.do_diff()
+        if diff:
+            print yaml_dump(diff)

=== modified file 'deployer/env/go.py'
--- deployer/env/go.py	2013-11-15 15:13:01 +0000
+++ deployer/env/go.py	2013-11-29 22:24:49 +0000
@@ -62,7 +62,7 @@
             return self.client.get_constraints(svc_name)
         except EnvError, e:
             if 'constraints do not apply to subordinate services' in str(e):
-                return None
+                return {}
             else:
                 raise e
 

=== modified file 'deployer/env/mem.py'
--- deployer/env/mem.py	2013-07-22 15:29:31 +0000
+++ deployer/env/mem.py	2013-11-29 22:24:49 +0000
@@ -1,26 +1,64 @@
+from deployer.utils import parse_constraints
+from jujuclient import (UnitErrors,
+                        EnvError)
 
 
 class MemoryEnvironment(object):
-
-    def __init__(self):
-        self.data = {}
-
-    def add_units(self, service_name, num_units):
+    """ In memory deployment: not all features implemented
+        (notably subordinates and their relations).
+    """
+
+    def __init__(self, name, deployment):
+        """ Two main dicts: _services (return-able as part of status(),
+            and _services_data (to hold e.g. config, constraints)
+        """
+        super(MemoryEnvironment, self).__init__()
+        self.name = name
+        self._deployment = deployment
+        self._services = {}
+        self._services_data = {}
+        self._relations = {}
+        self._do_deploy()
+
+    def add_units(self, svc_name, num):
         """Add units
         """
-        for n in range(num_units):
-            self.data['services'][service_name]
-
-    def _get_service(self, service_name):
-        if not service_name in self.data['services']:
+        next_num = self._services_data[svc_name]['next_unit_num']
+        for idx in xrange(next_num, next_num + num):
+            self._services[svc_name]['units'].append(
+                '{}/{}'.format(svc_name, idx))
+        self._services_data[svc_name]['next_unit_num'] = \
+            next_num + num
+
+    def remove_unit(self, unit_name):
+        """ Remove a unit by name """
+        svc_name = unit_name.split('/')[0]
+        units_idx = {unit: idx for idx, unit in
+                     enumerate(self._services[svc_name]['units'])}
+        try:
+            self._services[svc_name]['units'].pop(
+                units_idx[unit_name])
+        except KeyError:
+            raise UnitErrors("Invalid unit name")
+
+    def _get_service(self, svc_name):
+        """ Get service by name (as returned by status())
+        """
+        if not svc_name in self._services:
             raise EnvError("Invalid service name")
-        idx = self.data['services'][service_name]['unit_sequence']
-        return
+        return self._services[svc_name]
 
     def add_relation(self, endpoint_a, endpoint_b):
         """Add relations
         """
 
+    def destroy_service(self, svc_name):
+        """ Destroy a service
+        """
+        if not svc_name in self._services:
+            raise EnvError("Invalid service name")
+        del self._services[svc_name]
+
     def close(self):
         """
         """
@@ -29,11 +67,33 @@
         """
         """
 
+    def set_config(self, svc_name, cfg_dict):
+        """ Set service config from passed dict, keeping
+            the structure as needed for status() return
+        """
+        config = self.get_config(svc_name)
+        if cfg_dict:
+            for cfg_k, cfg_v in cfg_dict.items():
+                config_entry = config.setdefault(cfg_k, {})
+                config_entry['value'] = cfg_v
+
+    def set_constraints(self, svc_name, constr_str):
+        """ Set service constraints from "key=value ..."
+            passed string
+        """
+        constraints = parse_constraints(constr_str) if constr_str else {}
+        self._services_data[svc_name]['constraints'] = constraints
+
     def get_config(self, svc_name):
-        pass
+        """ Return service configs - note its structure:
+            config{thename: {'value': thevalue}, ...}
+        """
+        return self._services_data[svc_name]['config']
 
     def get_constraints(self, svc_name):
-        pass
+        """ Return service constraints dict
+        """
+        return self._services_data[svc_name]['constraints']
 
     def get_cli_status(self):
         pass
@@ -44,8 +104,69 @@
     def resolve_errors(self, retry_count=0, timeout=600, watch=False, delay=5):
         pass
 
+    def _do_deploy(self):
+        """ Fake deploy: build in-memory representation of the deployed set
+            of services from deployment
+        """
+        self._compile_relations()
+        for service in self._deployment.get_services():
+            svc_name = service.name
+            charm = self._deployment.get_charm_for(svc_name)
+            relations = self._relations.setdefault(svc_name, {})
+            self._services[svc_name] = {
+                'units': [],
+                'charm': charm.name,
+                'relations': relations,
+            }
+            self._services_data[svc_name] = {
+                'next_unit_num': 0,
+                'config': {},
+                'constraints': {},
+            }
+            # XXX: Incomplete relations support: only add units for non-subords
+            num_units = 0 if charm.is_subordinate() else service.num_units
+            self.add_units(svc_name, num_units)
+            self.set_config(svc_name, service.config)
+            self.set_constraints(svc_name, service.constraints)
+
+    def _compile_relations(self):
+        """ Compile a relation dictionary by svc_name, with
+            their values structured for status() return
+        """
+        for rel in self._deployment.get_relations():
+            for src, dst in (rel[0], rel[1]), (rel[1], rel[0]):
+                try:
+                    src_requires = self._deployment.get_charm_for(
+                        src).get_requires()
+                    dst_provides = self._deployment.get_charm_for(
+                        dst).get_provides()
+                except KeyError:
+                    continue
+                # Create dicts key-ed by:
+                #  { interface_type: (interface_name, svc_name)}
+                src_dict = {}
+                dst_dict = {}
+                # {rel_name: [{interface: name }...]}
+                for _, interfaces in src_requires.items():
+                    for interface in interfaces:
+                        src_dict[interface.get('interface')] = (
+                            interface.get('name'), src)
+                for _, interfaces in dst_provides.items():
+                    for interface in interfaces:
+                        dst_dict[interface.get('interface')] = (
+                            interface.get('name'), dst)
+                # Create juju env relation entries as:
+                # {svc_name: { interface_name: [ svc_name2, ...] }, ...}
+                for src_rel, (if_name, src_svc_name) in src_dict.items():
+                    if src_rel in dst_dict:
+                        src_rels = self._relations.setdefault(src_svc_name, {})
+                        src_rels_name = src_rels.setdefault(if_name, [])
+                        dst_svc_name = dst_dict[src_rel][1]
+                        src_rels[if_name].append(dst_svc_name)
+
     def status(self):
-        pass
+        """ Return all services status """
+        return {'services': self._services}
 
     def wait_for_units(
             self, timeout, goal_state="started", watch=False, no_exit=False):

=== added file 'deployer/tests/test_data/blog-haproxy-services.yaml'
--- deployer/tests/test_data/blog-haproxy-services.yaml	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_data/blog-haproxy-services.yaml	2013-11-29 22:24:49 +0000
@@ -0,0 +1,14 @@
+- service_name: blog
+  service_host: 0.0.0.0
+  service_port: 80
+  service_options:
+    - mode http
+    - option httplog
+    - balance uri
+    - hash-type consistent
+    - timeout client 60000
+    - timeout server 60000
+  server_options:
+    - check inter 2000
+    - rise 2
+    - fall 5

=== modified file 'deployer/tests/test_data/blog.yaml'
--- deployer/tests/test_data/blog.yaml	2013-07-22 15:29:31 +0000
+++ deployer/tests/test_data/blog.yaml	2013-11-29 22:24:49 +0000
@@ -28,14 +28,14 @@
         tuning: optimized
         engine: apache
         wp-content: include-base64://blog.snippet
-      memcached:
-        branch: lp:charms/precise/memcached
-        options:
-          size: 100
-      haproxy:
-        charm_url: cs:precise/haproxy
-        options:
-          services: include-file://blog-include.yaml
+    cache:
+      branch: lp:charms/precise/memcached
+      options:
+        size: 100
+    haproxy:
+      charm_url: cs:precise/haproxy
+      options:
+        services: include-file://blog-haproxy-services.yaml
   relations:
         - [blog, db]
         - - blog
@@ -56,4 +56,4 @@
         db:
             constraints: instance-type=m1.large
             options:
-                tuning-level: safest
\ No newline at end of file
+                tuning-level: safest

=== modified file 'deployer/tests/test_deployment.py'
--- deployer/tests/test_deployment.py	2013-10-31 19:19:36 +0000
+++ deployer/tests/test_deployment.py	2013-11-29 22:24:49 +0000
@@ -41,7 +41,7 @@
 
         # Load up overrides and resolves
         d.load_overrides(["key=abc"])
-        d.resolve_config()
+        d.resolve()
 
         # Verify include-base64
         self.assertEqual(d.get_service('newrelic').config, {'key': 'abc'})

=== added file 'deployer/tests/test_diff.py'
--- deployer/tests/test_diff.py	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_diff.py	2013-11-29 22:24:49 +0000
@@ -0,0 +1,111 @@
+""" Unittest for juju-deployer diff action (--diff) """
+# pylint: disable=C0103
+import StringIO
+import os
+import unittest
+import shutil
+import tempfile
+from deployer.env.mem import MemoryEnvironment
+from deployer.config import ConfigStack
+from deployer.utils import setup_logging
+
+from .base import Base
+from ..action.diff import Diff
+
+
+# pylint: disable=C0111, R0904
[e-mail]("DEB_BUILD_ARCH" in os.environ,
+                 "Requires configured bzr launchpad id and network access")
+class DiffTest(Base):
+
+    def setUp(self):
+        self.output = setup_logging(
+            debug=True, verbose=True, stream=StringIO.StringIO())
+
+    # Because fetch_charms is expensive, do it once for all tests
+    @classmethod
+    def setUpClass(cls):
+        deployment = ConfigStack(
+            [os.path.join(
+                cls.test_data_dir, "blog.yaml")]).get('wordpress-prod')
+        cls._dir = tempfile.mkdtemp()
+        os.mkdir(os.path.join(cls._dir, "precise"))
+        deployment.repo_path = cls._dir
+        print 'Fetching charms into "{}" ...'.format(cls._dir)
+        deployment.fetch_charms()
+        deployment.resolve()
+        cls._deployment = deployment
+
+    @classmethod
+    def tearDownClass(cls):
+        print 'Removing "{}"'.format(cls._dir)
+        shutil.rmtree(cls._dir)
+
+    @classmethod
+    def get_deployment(cls):
+        """ Return saved deployment at class initialization
+        """
+        return cls._deployment
+
+    def test_diff_nil(self):
+        dpl = self.get_deployment()
+        # No changes, assert nil diff
+        env = MemoryEnvironment(dpl.name, dpl)
+        diff = Diff(env, dpl, {}).do_diff()
+        self.assertEqual(diff, {})
+
+    def test_diff_num_units(self):
+        # Removing 1 unit must show -1 'num_units'
+        dpl = self.get_deployment()
+        env = MemoryEnvironment(dpl.name, dpl)
+        env.remove_unit(env.status()['services']['haproxy']['units'][0])
+        diff = Diff(env, dpl, {}).do_diff()
+        self.assertEqual(
+            diff['services']['modified']['haproxy']['num_units'], -1)
+        # re-adding a unit -> nil diff
+        env.add_units('haproxy', 1)
+        diff = Diff(env, dpl, {}).do_diff()
+        self.assertEqual(diff, {})
+
+    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'])
+
+    def test_diff_constraints(self):
+        dpl = self.get_deployment()
+        env = MemoryEnvironment(dpl.name, dpl)
+        env.set_constraints('haproxy', 'foo=bar')
+        diff = Diff(env, dpl, {}).do_diff()
+        mod_haproxy = diff['services']['modified']['haproxy']
+        self.assertTrue(
+            mod_haproxy['env-constraints'] != mod_haproxy['cfg-constraints'])
+        self.assertEqual(mod_haproxy['env-constraints'], {'foo': 'bar'})
+
+    def test_diff_service_destroy(self):
+        dpl = self.get_deployment()
+        env = MemoryEnvironment(dpl.name, dpl)
+        env.destroy_service('haproxy')
+        diff = Diff(env, dpl, {}).do_diff()
+        self.assertTrue(str(diff['relations']['missing'][0]).find('haproxy')
+                        != -1)
+        self.assertTrue(diff['services']['missing'].keys() == ['haproxy'])