Merge lp:~tvansteenburgh/charm-helpers/config-wrapper into lp:charm-helpers

[Tim Van Steenburgh]

Add Config wrapper around charm config dictionary.

Backwards-compatible with previous config dictionary. New wrapper
tracks which config values have changed since the previous hook
invocation and allows the user to query for the previous value.

[Benjamin Saller (bcsaller)]

LGTM +1 Thanks for adding this.

Some optional minors, prev_dict might be _prev_dict. It is private.

in changed
   call self.previous(key) rather than directly access prev_dict

use CONFIG_FILE_NAME as the default path in __init__, but allow it to be modified, that might make some of the patching in the tests go away

review: Approve

[lp:~tvansteenburgh/charm-helpers/config-wrapper]

146. By Tim Van Steenburgh on 2014-05-06

Minor changes per code review.

[Marco Ceppi (marcoceppi)]

LGTM

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2014-01-27 10:29:55 +0000
+++ charmhelpers/core/hookenv.py	2014-05-06 20:43:25 +0000
@@ -155,6 +155,100 @@
     return os.path.basename(sys.argv[0])
 
 
+class Config(dict):
+    """A Juju charm config dictionary that can write itself to
+    disk (as json) and track which values have changed since
+    the previous hook invocation.
+
+    Do not instantiate this object directly - instead call
+    ``hookenv.config()``
+
+    Example usage::
+
+        >>> # inside a hook
+        >>> from charmhelpers.core import hookenv
+        >>> config = hookenv.config()
+        >>> config['foo']
+        'bar'
+        >>> config['mykey'] = 'myval'
+        >>> config.save()
+
+
+        >>> # user runs `juju set mycharm foo=baz`
+        >>> # now we're inside subsequent config-changed hook
+        >>> config = hookenv.config()
+        >>> config['foo']
+        'baz'
+        >>> # test to see if this val has changed since last hook
+        >>> config.changed('foo')
+        True
+        >>> # what was the previous value?
+        >>> config.previous('foo')
+        'bar'
+        >>> # keys/values that we add are preserved across hooks
+        >>> config['mykey']
+        'myval'
+        >>> # don't forget to save at the end of hook!
+        >>> config.save()
+
+    """
+    CONFIG_FILE_NAME = '.juju-persistent-config'
+
+    def __init__(self, *args, **kw):
+        super(Config, self).__init__(*args, **kw)
+        self._prev_dict = None
+        self.path = os.path.join(charm_dir(), Config.CONFIG_FILE_NAME)
+        if os.path.exists(self.path):
+            self.load_previous()
+
+    def load_previous(self, path=None):
+        """Load previous copy of config from disk so that current values
+        can be compared to previous values.
+
+        :param path:
+
+            File path from which to load the previous config. If `None`,
+            config is loaded from the default location. If `path` is
+            specified, subsequent `save()` calls will write to the same
+            path.
+
+        """
+        self.path = path or self.path
+        with open(self.path) as f:
+            self._prev_dict = json.load(f)
+
+    def changed(self, key):
+        """Return true if the value for this key has changed since
+        the last save.
+
+        """
+        if self._prev_dict is None:
+            return True
+        return self.previous(key) != self.get(key)
+
+    def previous(self, key):
+        """Return previous value for this key, or None if there
+        is no "previous" value.
+
+        """
+        if self._prev_dict:
+            return self._prev_dict.get(key)
+        return None
+
+    def save(self):
+        """Save this config to disk.
+
+        Preserves items in _prev_dict that do not exist in self.
+
+        """
+        if self._prev_dict:
+            for k, v in self._prev_dict.iteritems():
+                if k not in self:
+                    self[k] = v
+        with open(self.path, 'w') as f:
+            json.dump(self, f)
+
+
 @cached
 def config(scope=None):
     """Juju charm configuration"""
@@ -163,7 +257,10 @@
         config_cmd_line.append(scope)
     config_cmd_line.append('--format=json')
     try:
-        return json.loads(subprocess.check_output(config_cmd_line))
+        config_data = json.loads(subprocess.check_output(config_cmd_line))
+        if scope is not None:
+            return config_data
+        return Config(config_data)
     except ValueError:
         return None
 

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2013-11-06 03:30:37 +0000
+++ tests/core/test_hookenv.py	2014-05-06 20:43:25 +0000
@@ -1,5 +1,8 @@
+import os
 import json
 from subprocess import CalledProcessError
+import shutil
+import tempfile
 
 import cPickle as pickle
 from mock import patch, call, mock_open
@@ -25,6 +28,92 @@
 """
 
 
+class ConfigTest(TestCase):
+    def setUp(self):
+        super(ConfigTest, self).setUp()
+
+        self.charm_dir = tempfile.mkdtemp()
+        self.addCleanup(lambda: shutil.rmtree(self.charm_dir))
+
+        patcher = patch.object(hookenv, 'charm_dir', lambda: self.charm_dir)
+        self.addCleanup(patcher.stop)
+        patcher.start()
+
+    def test_init(self):
+        d = dict(foo='bar')
+        c = hookenv.Config(d)
+
+        self.assertEqual(c['foo'], 'bar')
+        self.assertEqual(c._prev_dict, None)
+
+    def test_load_previous(self):
+        d = dict(foo='bar')
+        c = hookenv.Config()
+
+        with open(c.path, 'w') as f:
+            json.dump(d, f)
+
+        c.load_previous()
+        self.assertEqual(c._prev_dict, d)
+
+    def test_load_previous_alternate_path(self):
+        d = dict(foo='bar')
+        c = hookenv.Config()
+
+        alt_path = os.path.join(self.charm_dir, '.alt-config')
+        with open(alt_path, 'w') as f:
+            json.dump(d, f)
+
+        c.load_previous(path=alt_path)
+        self.assertEqual(c._prev_dict, d)
+        self.assertEqual(c.path, alt_path)
+
+    def test_changed_without_prev_dict(self):
+        d = dict(foo='bar')
+        c = hookenv.Config(d)
+
+        self.assertTrue(c.changed('foo'))
+
+    def test_changed_with_prev_dict(self):
+        c = hookenv.Config(dict(foo='bar', a='b'))
+        c.save()
+        c = hookenv.Config(dict(foo='baz', a='b'))
+
+        self.assertTrue(c.changed('foo'))
+        self.assertFalse(c.changed('a'))
+
+    def test_previous_without_prev_dict(self):
+        c = hookenv.Config()
+
+        self.assertEqual(c.previous('foo'), None)
+
+    def test_previous_with_prev_dict(self):
+        c = hookenv.Config(dict(foo='bar'))
+        c.save()
+        c = hookenv.Config(dict(foo='baz', a='b'))
+
+        self.assertEqual(c.previous('foo'), 'bar')
+        self.assertEqual(c.previous('a'), None)
+
+    def test_save_without_prev_dict(self):
+        c = hookenv.Config(dict(foo='bar'))
+        c.save()
+
+        with open(c.path, 'r') as f:
+            self.assertEqual(c, json.load(f))
+            self.assertEqual(c, dict(foo='bar'))
+
+    def test_save_with_prev_dict(self):
+        c = hookenv.Config(dict(foo='bar'))
+        c.save()
+        c = hookenv.Config(dict(a='b'))
+        c.save()
+
+        with open(c.path, 'r') as f:
+            self.assertEqual(c, json.load(f))
+            self.assertEqual(c, dict(foo='bar', a='b'))
+
+
 class SerializableTest(TestCase):
     def test_serializes_object_to_json(self):
         foo = {
@@ -165,6 +254,18 @@
         self.assertEqual(result, None)
         check_output.assert_called_with(['config-get', 'baz', '--format=json'])
 
+    @patch('charmhelpers.core.hookenv.charm_dir')
+    @patch('subprocess.check_output')
+    def test_gets_config_without_scope(self, check_output, charm_dir):
+        check_output.return_value = json.dumps(dict(foo='bar'))
+        charm_dir.side_effect = tempfile.mkdtemp
+
+        result = hookenv.config()
+
+        self.assertIsInstance(result, hookenv.Config)
+        self.assertEqual(result['foo'], 'bar')
+        check_output.assert_called_with(['config-get', '--format=json'])
+
     @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_local_unit(self, os_):
         os_.environ = {