Merge lp:~tvansteenburgh/charm-helpers/config-implicit-save into lp:charm-helpers
- config-implicit-save
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 200 | ||||
Proposed branch: | lp:~tvansteenburgh/charm-helpers/config-implicit-save | ||||
Merge into: | lp:charm-helpers | ||||
Diff against target: |
278 lines (+87/-27) 6 files modified
charmhelpers/core/hookenv.py (+28/-15) charmhelpers/core/services/base.py (+3/-0) docs/examples/config.rst (+0/-8) tests/contrib/ansible/test_ansible.py (+5/-2) tests/core/test_hookenv.py (+31/-0) tests/core/test_services.py (+20/-2) |
||||
To merge this branch: | bzr merge lp:~tvansteenburgh/charm-helpers/config-implicit-save | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stuart Bishop (community) | Approve | ||
Review via email: mp+224678@code.launchpad.net |
Commit message
Description of the change
Make Config implicitly save itself instead of requiring charm author to config.save() in every hook.
Stuart Bishop (stub) wrote : | # |
Tim Van Steenburgh (tvansteenburgh) wrote : | # |
> 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) wrote : | # |
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) wrote : | # |
... 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) wrote : | # |
One other idea: do the save in Config.
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) wrote : | # |
In retrospect I guess this is no different than the original idea of doing the save in the destructor. :/
- 175. By Tim Van Steenburgh
-
Merge from trunk
- 176. By Tim Van Steenburgh
-
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
-
Merge from lp:charm-helpers
- 178. By Tim Van Steenburgh
-
Remove no-op tests
Tim Van Steenburgh (tvansteenburgh) wrote : | # |
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.
In other scenarios, client code would need to call Config.save() explicitly. Implicit saves can still be disabled by setting `Config.
The important changes are on r176. The other commits are just merges from trunk and cleanup.
Stuart Bishop (stub) wrote : | # |
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.
Preview Diff
1 | === modified file 'charmhelpers/core/hookenv.py' |
2 | --- charmhelpers/core/hookenv.py 2014-08-19 14:04:37 +0000 |
3 | +++ charmhelpers/core/hookenv.py 2014-08-26 15:16:14 +0000 |
4 | @@ -156,12 +156,15 @@ |
5 | |
6 | |
7 | class Config(dict): |
8 | - """A Juju charm config dictionary that can write itself to |
9 | - disk (as json) and track which values have changed since |
10 | - the previous hook invocation. |
11 | - |
12 | - Do not instantiate this object directly - instead call |
13 | - ``hookenv.config()`` |
14 | + """A dictionary representation of the charm's config.yaml, with some |
15 | + extra features: |
16 | + |
17 | + - See which values in the dictionary have changed since the previous hook. |
18 | + - For values that have changed, see what the previous value was. |
19 | + - Store arbitrary data for use in a later hook. |
20 | + |
21 | + NOTE: Do not instantiate this object directly - instead call |
22 | + ``hookenv.config()``, which will return an instance of :class:`Config`. |
23 | |
24 | Example usage:: |
25 | |
26 | @@ -170,8 +173,8 @@ |
27 | >>> config = hookenv.config() |
28 | >>> config['foo'] |
29 | 'bar' |
30 | + >>> # store a new key/value for later use |
31 | >>> config['mykey'] = 'myval' |
32 | - >>> config.save() |
33 | |
34 | |
35 | >>> # user runs `juju set mycharm foo=baz` |
36 | @@ -188,22 +191,23 @@ |
37 | >>> # keys/values that we add are preserved across hooks |
38 | >>> config['mykey'] |
39 | 'myval' |
40 | - >>> # don't forget to save at the end of hook! |
41 | - >>> config.save() |
42 | |
43 | """ |
44 | CONFIG_FILE_NAME = '.juju-persistent-config' |
45 | |
46 | def __init__(self, *args, **kw): |
47 | super(Config, self).__init__(*args, **kw) |
48 | + self.implicit_save = True |
49 | self._prev_dict = None |
50 | self.path = os.path.join(charm_dir(), Config.CONFIG_FILE_NAME) |
51 | if os.path.exists(self.path): |
52 | self.load_previous() |
53 | |
54 | def load_previous(self, path=None): |
55 | - """Load previous copy of config from disk so that current values |
56 | - can be compared to previous values. |
57 | + """Load previous copy of config from disk. |
58 | + |
59 | + In normal usage you don't need to call this method directly - it |
60 | + is called automatically at object initialization. |
61 | |
62 | :param path: |
63 | |
64 | @@ -218,8 +222,8 @@ |
65 | self._prev_dict = json.load(f) |
66 | |
67 | def changed(self, key): |
68 | - """Return true if the value for this key has changed since |
69 | - the last save. |
70 | + """Return True if the current value for this key is different from |
71 | + the previous value. |
72 | |
73 | """ |
74 | if self._prev_dict is None: |
75 | @@ -228,7 +232,7 @@ |
76 | |
77 | def previous(self, key): |
78 | """Return previous value for this key, or None if there |
79 | - is no "previous" value. |
80 | + is no previous value. |
81 | |
82 | """ |
83 | if self._prev_dict: |
84 | @@ -238,7 +242,13 @@ |
85 | def save(self): |
86 | """Save this config to disk. |
87 | |
88 | - Preserves items in _prev_dict that do not exist in self. |
89 | + If the charm is using the :mod:`Services Framework <services.base>` |
90 | + or :meth:'@hook <Hooks.hook>' decorator, this |
91 | + is called automatically at the end of successful hook execution. |
92 | + Otherwise, it should be called directly by user code. |
93 | + |
94 | + To disable automatic saves, set ``implicit_save=False`` on this |
95 | + instance. |
96 | |
97 | """ |
98 | if self._prev_dict: |
99 | @@ -478,6 +488,9 @@ |
100 | hook_name = os.path.basename(args[0]) |
101 | if hook_name in self._hooks: |
102 | self._hooks[hook_name]() |
103 | + cfg = config() |
104 | + if cfg.implicit_save: |
105 | + cfg.save() |
106 | else: |
107 | raise UnregisteredHookError(hook_name) |
108 | |
109 | |
110 | === modified file 'charmhelpers/core/services/base.py' |
111 | --- charmhelpers/core/services/base.py 2014-08-18 17:21:31 +0000 |
112 | +++ charmhelpers/core/services/base.py 2014-08-26 15:16:14 +0000 |
113 | @@ -118,6 +118,9 @@ |
114 | else: |
115 | self.provide_data() |
116 | self.reconfigure_services() |
117 | + cfg = hookenv.config() |
118 | + if cfg.implicit_save: |
119 | + cfg.save() |
120 | |
121 | def provide_data(self): |
122 | """ |
123 | |
124 | === modified file 'docs/examples/config.rst' |
125 | --- docs/examples/config.rst 2014-06-12 17:55:56 +0000 |
126 | +++ docs/examples/config.rst 2014-08-26 15:16:14 +0000 |
127 | @@ -38,8 +38,6 @@ |
128 | |
129 | assert config['app-name'] == 'My App' |
130 | |
131 | - config.save() |
132 | - |
133 | Checking if a config value has changed |
134 | -------------------------------------- |
135 | |
136 | @@ -64,8 +62,6 @@ |
137 | assert config['app-name'] == 'My New App' |
138 | assert config.previous('app-name') == 'My App' |
139 | |
140 | - config.save() |
141 | - |
142 | Saving arbitrary key/value data |
143 | ------------------------------- |
144 | |
145 | @@ -85,12 +81,8 @@ |
146 | |
147 | config['mykey'] = 'myval' |
148 | |
149 | - config.save() |
150 | - |
151 | @hooks.hook('config-changed') |
152 | def config_changed(): |
153 | config = hookenv.config() |
154 | |
155 | assert config['mykey'] == 'myval' |
156 | - |
157 | - config.save() |
158 | |
159 | === modified file 'tests/contrib/ansible/test_ansible.py' |
160 | --- tests/contrib/ansible/test_ansible.py 2014-07-30 13:30:43 +0000 |
161 | +++ tests/contrib/ansible/test_ansible.py 2014-08-26 15:16:14 +0000 |
162 | @@ -11,6 +11,7 @@ |
163 | |
164 | |
165 | import charmhelpers.contrib.ansible |
166 | +from charmhelpers.core import hookenv |
167 | |
168 | |
169 | class InstallAnsibleSupportTestCase(unittest.TestCase): |
170 | @@ -185,7 +186,8 @@ |
171 | 'ansible-playbook', '-c', 'local', 'playbooks/complete-state.yaml', |
172 | '--tags', 'install,somethingelse']) |
173 | |
174 | - def test_hooks_executes_playbook_with_tag(self): |
175 | + @mock.patch.object(hookenv, 'config') |
176 | + def test_hooks_executes_playbook_with_tag(self, config): |
177 | hooks = charmhelpers.contrib.ansible.AnsibleHooks('my/playbook.yaml') |
178 | foo = mock.MagicMock() |
179 | hooks.register('foo', foo) |
180 | @@ -197,7 +199,8 @@ |
181 | 'ansible-playbook', '-c', 'local', 'my/playbook.yaml', |
182 | '--tags', 'foo']) |
183 | |
184 | - def test_specifying_ansible_handled_hooks(self): |
185 | + @mock.patch.object(hookenv, 'config') |
186 | + def test_specifying_ansible_handled_hooks(self, config): |
187 | hooks = charmhelpers.contrib.ansible.AnsibleHooks( |
188 | 'my/playbook.yaml', default_hooks=['start', 'stop']) |
189 | |
190 | |
191 | === modified file 'tests/core/test_hookenv.py' |
192 | --- tests/core/test_hookenv.py 2014-05-06 20:42:06 +0000 |
193 | +++ tests/core/test_hookenv.py 2014-08-26 15:16:14 +0000 |
194 | @@ -932,6 +932,37 @@ |
195 | |
196 | |
197 | class HooksTest(TestCase): |
198 | + def setUp(self): |
199 | + super(HooksTest, self).setUp() |
200 | + self.config = patch.object(hookenv, 'config') |
201 | + self.config.start() |
202 | + |
203 | + def tearDown(self): |
204 | + super(HooksTest, self).tearDown() |
205 | + self.config.stop() |
206 | + |
207 | + def test_config_saved_after_execute(self): |
208 | + config = hookenv.config() |
209 | + config.implicit_save = True |
210 | + |
211 | + foo = MagicMock() |
212 | + hooks = hookenv.Hooks() |
213 | + hooks.register('foo', foo) |
214 | + hooks.execute(['foo', 'some', 'other', 'args']) |
215 | + |
216 | + self.assertTrue(config.save.called) |
217 | + |
218 | + def test_config_not_saved_after_execute(self): |
219 | + config = hookenv.config() |
220 | + config.implicit_save = False |
221 | + |
222 | + foo = MagicMock() |
223 | + hooks = hookenv.Hooks() |
224 | + hooks.register('foo', foo) |
225 | + hooks.execute(['foo', 'some', 'other', 'args']) |
226 | + |
227 | + self.assertFalse(config.save.called) |
228 | + |
229 | def test_runs_a_registered_function(self): |
230 | foo = MagicMock() |
231 | hooks = hookenv.Hooks() |
232 | |
233 | === modified file 'tests/core/test_services.py' |
234 | --- tests/core/test_services.py 2014-07-10 14:43:59 +0000 |
235 | +++ tests/core/test_services.py 2014-08-26 15:16:14 +0000 |
236 | @@ -30,7 +30,8 @@ |
237 | @mock.patch.object(services.ServiceManager, 'reconfigure_services') |
238 | @mock.patch.object(services.ServiceManager, 'stop_services') |
239 | @mock.patch.object(hookenv, 'hook_name') |
240 | - def test_manage_stop(self, hook_name, stop_services, reconfigure_services): |
241 | + @mock.patch.object(hookenv, 'config') |
242 | + def test_manage_stop(self, config, hook_name, stop_services, reconfigure_services): |
243 | manager = services.ServiceManager() |
244 | hook_name.return_value = 'stop' |
245 | manager.manage() |
246 | @@ -41,7 +42,8 @@ |
247 | @mock.patch.object(services.ServiceManager, 'reconfigure_services') |
248 | @mock.patch.object(services.ServiceManager, 'stop_services') |
249 | @mock.patch.object(hookenv, 'hook_name') |
250 | - def test_manage_other(self, hook_name, stop_services, reconfigure_services, provide_data): |
251 | + @mock.patch.object(hookenv, 'config') |
252 | + def test_manage_other(self, config, hook_name, stop_services, reconfigure_services, provide_data): |
253 | manager = services.ServiceManager() |
254 | hook_name.return_value = 'config-changed' |
255 | manager.manage() |
256 | @@ -49,6 +51,22 @@ |
257 | reconfigure_services.assert_called_once_with() |
258 | provide_data.assert_called_once_with() |
259 | |
260 | + @mock.patch.object(hookenv, 'config') |
261 | + def test_manage_config_saved(self, config): |
262 | + config = config.return_value |
263 | + config.implicit_save = True |
264 | + manager = services.ServiceManager() |
265 | + manager.manage() |
266 | + self.assertTrue(config.save.called) |
267 | + |
268 | + @mock.patch.object(hookenv, 'config') |
269 | + def test_manage_config_not_saved(self, config): |
270 | + config = config.return_value |
271 | + config.implicit_save = False |
272 | + manager = services.ServiceManager() |
273 | + manager.manage() |
274 | + self.assertFalse(config.save.called) |
275 | + |
276 | @mock.patch.object(services.ServiceManager, 'save_ready') |
277 | @mock.patch.object(services.ServiceManager, 'fire_event') |
278 | @mock.patch.object(services.ServiceManager, 'is_ready') |
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.