Merge lp:~tvansteenburgh/charm-helpers/config-implicit-save into lp:charm-helpers

[Tim Van Steenburgh]

Make Config implicitly save itself instead of requiring charm author to config.save() in every hook.

[Stuart Bishop (stub)]

I don't think using the destructor is the right thing to do. It will cause the changes to be persisted even if the hook fails.

The sanest place I can think of to save the config only on successful termination is in the @hooks.hook decorator.

[Tim Van Steenburgh (tvansteenburgh)]

> I don't think using the destructor is the right thing to do. It will cause the
> changes to be persisted even if the hook fails.
>
> The sanest place I can think of to save the config only on successful
> termination is in the @hooks.hook decorator.

Good observation. Doing the save in the @hooks.hook will be difficult since there is not a global Config object.

Can you think of a downside of doing this instead:

```
def __del__(self):
    if self.implicit_save and not sys.exc_type:
        self.save()
```

[Stuart Bishop (stub)]

There actually *is* a global config object, because the config() method is using the @cached decorator. This could even be made explicit.

I didn't know you could check for sys.exc_type to catch this case (I imagine you would need to ignore SystemExit?). In any case, I don't think it is a good idea here; I have hooks that catch exceptions, log nice error messages, and sys.exit(1).

[Stuart Bishop (stub)]

... although if you *can* sniff for sys.exc_* reliably, and see a SystemExit, it may be good enough to save only if a) no exception b) its a SystemExit with a 0 return code

[Tim Van Steenburgh (tvansteenburgh)]

One other idea: do the save in Config.__setitem__()

I like this for two reasons:

1. No sys.exc_type voodoo
2. Keeps code local to the Config object, no coupling to or dependency on @hooks.hook

The downsides are:

1. May save() multiple times. Performance hit for writing file multiple times.
2. save()s are immediate, no rollback if exception occurs.

I think #1 is minor, and #2 is actually consistent with the way many charms have handled persisting config thus far - by writing dot files.

I think the pros outweigh the cons but would appreciate other opinions.

[Tim Van Steenburgh (tvansteenburgh)]

In retrospect I guess this is no different than the original idea of doing the save in the destructor. :/

[lp:~tvansteenburgh/charm-helpers/config-implicit-save]

175. By Tim Van Steenburgh on 2014-08-25

Merge from trunk

176. By Tim Van Steenburgh on 2014-08-26

Change how Config is implicitly saved

Call Config.save() automatically after hook execution if the hook
code is running in the context of a @hook decorator or the
Services Framework.

177. By Tim Van Steenburgh on 2014-08-26

Merge from lp:charm-helpers

178. By Tim Van Steenburgh on 2014-08-26

Remove no-op tests

[Tim Van Steenburgh (tvansteenburgh)]

After discussion with stub and cory_fu, the following changes were made:

* Don't implicitly save in __del__ since that will fire even after exceptions occur.

Instead:

* Save in Hooks.execute(), if the hook code is running in the context of a @hook decorated function, or
* Save in ServiceManager.manage(), if hook code is running in the context of the Services Framework

In other scenarios, client code would need to call Config.save() explicitly. Implicit saves can still be disabled by setting `Config.implicit_save = False`.

The important changes are on r176. The other commits are just merges from trunk and cleanup.

[Stuart Bishop (stub)]

All looks good to me. Thanks for this - it will let me drop a chunk of code in the PostgreSQL charm I've been wanting to drop.

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2014-08-19 14:04:37 +0000
+++ charmhelpers/core/hookenv.py	2014-08-26 15:16:14 +0000
@@ -156,12 +156,15 @@
 
 
 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()``
+    """A dictionary representation of the charm's config.yaml, with some
+    extra features:
+
+    - See which values in the dictionary have changed since the previous hook.
+    - For values that have changed, see what the previous value was.
+    - Store arbitrary data for use in a later hook.
+
+    NOTE: Do not instantiate this object directly - instead call
+    ``hookenv.config()``, which will return an instance of :class:`Config`.
 
     Example usage::
 
@@ -170,8 +173,8 @@
         >>> config = hookenv.config()
         >>> config['foo']
         'bar'
+        >>> # store a new key/value for later use
         >>> config['mykey'] = 'myval'
-        >>> config.save()
 
 
         >>> # user runs `juju set mycharm foo=baz`
@@ -188,22 +191,23 @@
         >>> # 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.implicit_save = True
         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.
+        """Load previous copy of config from disk.
+
+        In normal usage you don't need to call this method directly - it
+        is called automatically at object initialization.
 
         :param path:
 
@@ -218,8 +222,8 @@
             self._prev_dict = json.load(f)
 
     def changed(self, key):
-        """Return true if the value for this key has changed since
-        the last save.
+        """Return True if the current value for this key is different from
+        the previous value.
 
         """
         if self._prev_dict is None:
@@ -228,7 +232,7 @@
 
     def previous(self, key):
         """Return previous value for this key, or None if there
-        is no "previous" value.
+        is no previous value.
 
         """
         if self._prev_dict:
@@ -238,7 +242,13 @@
     def save(self):
         """Save this config to disk.
 
-        Preserves items in _prev_dict that do not exist in self.
+        If the charm is using the :mod:`Services Framework <services.base>`
+        or :meth:'@hook <Hooks.hook>' decorator, this
+        is called automatically at the end of successful hook execution.
+        Otherwise, it should be called directly by user code.
+
+        To disable automatic saves, set ``implicit_save=False`` on this
+        instance.
 
         """
         if self._prev_dict:
@@ -478,6 +488,9 @@
         hook_name = os.path.basename(args[0])
         if hook_name in self._hooks:
             self._hooks[hook_name]()
+            cfg = config()
+            if cfg.implicit_save:
+                cfg.save()
         else:
             raise UnregisteredHookError(hook_name)
 
 
=== modified file 'charmhelpers/core/services/base.py'
--- charmhelpers/core/services/base.py	2014-08-18 17:21:31 +0000
+++ charmhelpers/core/services/base.py	2014-08-26 15:16:14 +0000
@@ -118,6 +118,9 @@
         else:
             self.provide_data()
             self.reconfigure_services()
+        cfg = hookenv.config()
+        if cfg.implicit_save:
+            cfg.save()
 
     def provide_data(self):
         """
 
=== modified file 'docs/examples/config.rst'
--- docs/examples/config.rst	2014-06-12 17:55:56 +0000
+++ docs/examples/config.rst	2014-08-26 15:16:14 +0000
@@ -38,8 +38,6 @@
 
       assert config['app-name'] == 'My App'
 
-      config.save()
-
 Checking if a config value has changed
 --------------------------------------
 
@@ -64,8 +62,6 @@
       assert config['app-name'] == 'My New App'
       assert config.previous('app-name') == 'My App'
 
-      config.save()
-
 Saving arbitrary key/value data
 -------------------------------
 
@@ -85,12 +81,8 @@
 
       config['mykey'] = 'myval'
 
-      config.save()
-
   @hooks.hook('config-changed')
   def config_changed():
       config = hookenv.config()
 
       assert config['mykey'] == 'myval'
-
-      config.save()
 
=== modified file 'tests/contrib/ansible/test_ansible.py'
--- tests/contrib/ansible/test_ansible.py	2014-07-30 13:30:43 +0000
+++ tests/contrib/ansible/test_ansible.py	2014-08-26 15:16:14 +0000
@@ -11,6 +11,7 @@
 
 
 import charmhelpers.contrib.ansible
+from charmhelpers.core import hookenv
 
 
 class InstallAnsibleSupportTestCase(unittest.TestCase):
@@ -185,7 +186,8 @@
             'ansible-playbook', '-c', 'local', 'playbooks/complete-state.yaml',
             '--tags', 'install,somethingelse'])
 
-    def test_hooks_executes_playbook_with_tag(self):
+    @mock.patch.object(hookenv, 'config')
+    def test_hooks_executes_playbook_with_tag(self, config):
         hooks = charmhelpers.contrib.ansible.AnsibleHooks('my/playbook.yaml')
         foo = mock.MagicMock()
         hooks.register('foo', foo)
@@ -197,7 +199,8 @@
             'ansible-playbook', '-c', 'local', 'my/playbook.yaml',
             '--tags', 'foo'])
 
-    def test_specifying_ansible_handled_hooks(self):
+    @mock.patch.object(hookenv, 'config')
+    def test_specifying_ansible_handled_hooks(self, config):
         hooks = charmhelpers.contrib.ansible.AnsibleHooks(
             'my/playbook.yaml', default_hooks=['start', 'stop'])
 
 
=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2014-05-06 20:42:06 +0000
+++ tests/core/test_hookenv.py	2014-08-26 15:16:14 +0000
@@ -932,6 +932,37 @@
 
 
 class HooksTest(TestCase):
+    def setUp(self):
+        super(HooksTest, self).setUp()
+        self.config = patch.object(hookenv, 'config')
+        self.config.start()
+
+    def tearDown(self):
+        super(HooksTest, self).tearDown()
+        self.config.stop()
+
+    def test_config_saved_after_execute(self):
+        config = hookenv.config()
+        config.implicit_save = True
+
+        foo = MagicMock()
+        hooks = hookenv.Hooks()
+        hooks.register('foo', foo)
+        hooks.execute(['foo', 'some', 'other', 'args'])
+
+        self.assertTrue(config.save.called)
+
+    def test_config_not_saved_after_execute(self):
+        config = hookenv.config()
+        config.implicit_save = False
+
+        foo = MagicMock()
+        hooks = hookenv.Hooks()
+        hooks.register('foo', foo)
+        hooks.execute(['foo', 'some', 'other', 'args'])
+
+        self.assertFalse(config.save.called)
+
     def test_runs_a_registered_function(self):
         foo = MagicMock()
         hooks = hookenv.Hooks()
 
=== modified file 'tests/core/test_services.py'
--- tests/core/test_services.py	2014-07-10 14:43:59 +0000
+++ tests/core/test_services.py	2014-08-26 15:16:14 +0000
@@ -30,7 +30,8 @@
     @mock.patch.object(services.ServiceManager, 'reconfigure_services')
     @mock.patch.object(services.ServiceManager, 'stop_services')
     @mock.patch.object(hookenv, 'hook_name')
-    def test_manage_stop(self, hook_name, stop_services, reconfigure_services):
+    @mock.patch.object(hookenv, 'config')
+    def test_manage_stop(self, config, hook_name, stop_services, reconfigure_services):
         manager = services.ServiceManager()
         hook_name.return_value = 'stop'
         manager.manage()
@@ -41,7 +42,8 @@
     @mock.patch.object(services.ServiceManager, 'reconfigure_services')
     @mock.patch.object(services.ServiceManager, 'stop_services')
     @mock.patch.object(hookenv, 'hook_name')
-    def test_manage_other(self, hook_name, stop_services, reconfigure_services, provide_data):
+    @mock.patch.object(hookenv, 'config')
+    def test_manage_other(self, config, hook_name, stop_services, reconfigure_services, provide_data):
         manager = services.ServiceManager()
         hook_name.return_value = 'config-changed'
         manager.manage()
@@ -49,6 +51,22 @@
         reconfigure_services.assert_called_once_with()
         provide_data.assert_called_once_with()
 
+    @mock.patch.object(hookenv, 'config')
+    def test_manage_config_saved(self, config):
+        config = config.return_value
+        config.implicit_save = True
+        manager = services.ServiceManager()
+        manager.manage()
+        self.assertTrue(config.save.called)
+
+    @mock.patch.object(hookenv, 'config')
+    def test_manage_config_not_saved(self, config):
+        config = config.return_value
+        config.implicit_save = False
+        manager = services.ServiceManager()
+        manager.manage()
+        self.assertFalse(config.save.called)
+
     @mock.patch.object(services.ServiceManager, 'save_ready')
     @mock.patch.object(services.ServiceManager, 'fire_event')
     @mock.patch.object(services.ServiceManager, 'is_ready')