Merge lp:~stub/charm-helpers/bug-1182959-no-implicit-serializable-magic into lp:charm-helpers

Proposed by Stuart Bishop
Status: Merged
Merged at revision: 42
Proposed branch: lp:~stub/charm-helpers/bug-1182959-no-implicit-serializable-magic
Merge into: lp:charm-helpers
Prerequisite: lp:~stub/charm-helpers/bug-1192845-fix-serializable
Diff against target: 187 lines (+58/-32)
2 files modified
charmhelpers/core/hookenv.py (+10/-14)
tests/core/test_hookenv.py (+48/-18)
To merge this branch: bzr merge lp:~stub/charm-helpers/bug-1182959-no-implicit-serializable-magic
Reviewer Review Type Date Requested Status
Matthew Wedgwood (community) Approve
Charm Helper Maintainers Pending
Review via email: mp+170557@code.launchpad.net

Description of the change

Going further than https://code.launchpad.net/~stub/charm-helpers/bug-1192845-fix-serializable/+merge/170555, I put forward that implicitly wrapping results with Serializable is surprising and more harm than help. It introduces time bombs, such as if the implementation is moved to a different module old pickles in charm state will fail to load.

The alternative is when a wrapped object is desired that it be explicitly wrapped.

To post a comment you must log in.
Revision history for this message
Matthew Wedgwood (mew) wrote :

Yep, fair enough. Better tear of the bandage now while we're still under heavy development.

review: Approve
Revision history for this message
Matthew Wedgwood (mew) :
review: Needs Fixing
Revision history for this message
Matthew Wedgwood (mew) wrote :

Sorry, after being more prudent, I see that this fails tests.

review: Needs Fixing
31. By Stuart Bishop

Merged bug-1192845-fix-serializable into bug-1182959-no-implicit-serializable-magic.

32. By Stuart Bishop

Fix tests

Revision history for this message
Stuart Bishop (stub) wrote :

I've fixed the tests so they no longer rely on magic, and removed one explicitly checking that magic was returned.

33. By Stuart Bishop

Merged fix-executution-environment into bug-1182959-no-implicit-serializable-magic.

Revision history for this message
Matthew Wedgwood (mew) wrote :

Perfect, thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/core/hookenv.py'
2--- charmhelpers/core/hookenv.py 2013-06-20 08:32:11 +0000
3+++ charmhelpers/core/hookenv.py 2013-06-28 17:11:26 +0000
4@@ -108,11 +108,12 @@
5 """A convenient bundling of the current execution context"""
6 context = {}
7 context['conf'] = config()
8- context['reltype'] = relation_type()
9- context['relid'] = relation_id()
10+ if relation_id():
11+ context['reltype'] = relation_type()
12+ context['relid'] = relation_id()
13+ context['rel'] = relation_get()
14 context['unit'] = local_unit()
15 context['rels'] = relations()
16- context['rel'] = relation_get()
17 context['env'] = os.environ
18 return context
19
20@@ -150,13 +151,9 @@
21 config_cmd_line.append(scope)
22 config_cmd_line.append('--format=json')
23 try:
24- value = json.loads(subprocess.check_output(config_cmd_line))
25+ return json.loads(subprocess.check_output(config_cmd_line))
26 except ValueError:
27 return None
28- if isinstance(value, dict):
29- return Serializable(value)
30- else:
31- return value
32
33
34 @cached
35@@ -169,13 +166,9 @@
36 if unit:
37 _args.append(unit)
38 try:
39- value = json.loads(subprocess.check_output(_args))
40+ return json.loads(subprocess.check_output(_args))
41 except ValueError:
42 return None
43- if isinstance(value, dict):
44- return Serializable(value)
45- else:
46- return value
47
48
49 def relation_set(relation_id=None, relation_settings={}, **kwargs):
50@@ -222,7 +215,7 @@
51 if key.endswith('-list'):
52 relation[key] = relation[key].split()
53 relation['__unit__'] = unit
54- return Serializable(relation)
55+ return relation
56
57
58 @cached
59@@ -331,5 +324,8 @@
60 self.register(hook_name, decorated)
61 else:
62 self.register(decorated.__name__, decorated)
63+ if '_' in decorated.__name__:
64+ self.register(
65+ decorated.__name__.replace('_', '-'), decorated)
66 return decorated
67 return wrapper
68
69=== modified file 'tests/core/test_hookenv.py'
70--- tests/core/test_hookenv.py 2013-06-20 08:53:31 +0000
71+++ tests/core/test_hookenv.py 2013-06-28 17:11:26 +0000
72@@ -140,17 +140,6 @@
73 mock_call.assert_called_with(['juju-log', '-l', level, 'foo'])
74
75 @patch('subprocess.check_output')
76- def test_gets_charm_config_as_serializable(self, check_output):
77- config_data = {'foo': 'bar'}
78- check_output.return_value = json.dumps(config_data)
79-
80- result = hookenv.config()
81-
82- self.assert_(isinstance(result, hookenv.Serializable))
83- self.assertEqual(result.foo, 'bar')
84- check_output.assert_called_with(['config-get', '--format=json'])
85-
86- @patch('subprocess.check_output')
87 def test_gets_charm_config_with_scope(self, check_output):
88 config_data = 'bar'
89 check_output.return_value = json.dumps(config_data)
90@@ -311,15 +300,14 @@
91 unit = 'foo-unit'
92 raw_relation = {
93 'foo': 'bar',
94- 'baz-list': '1 2 3',
95 }
96 remote_unit.return_value = unit
97 relation_get.return_value = raw_relation
98
99 result = hookenv.relation_for_unit()
100
101- self.assertEqual(result.__unit__, unit)
102- self.assertEqual(getattr(result, 'baz-list'), ['1', '2', '3'])
103+ self.assertEqual(result['__unit__'], unit)
104+ self.assertEqual(result['foo'], 'bar')
105 relation_get.assert_called_with(unit=unit, rid=None)
106
107 @patch('charmhelpers.core.hookenv.remote_unit')
108@@ -328,14 +316,13 @@
109 unit = 'foo-unit'
110 raw_relation = {
111 'foo': 'bar',
112- 'baz-list': '1 2 3',
113 }
114 relation_get.return_value = raw_relation
115
116 result = hookenv.relation_for_unit(unit)
117
118- self.assertEqual(result.__unit__, unit)
119- self.assertEqual(getattr(result, 'baz-list'), ['1', '2', '3'])
120+ self.assertEqual(result['__unit__'], unit)
121+ self.assertEqual(result['foo'], 'bar')
122 relation_get.assert_called_with(unit=unit, rid=None)
123 self.assertFalse(remote_unit.called)
124
125@@ -496,6 +483,34 @@
126 'env': 'some-environment',
127 })
128
129+ @patch('charmhelpers.core.hookenv.config')
130+ @patch('charmhelpers.core.hookenv.relation_type')
131+ @patch('charmhelpers.core.hookenv.local_unit')
132+ @patch('charmhelpers.core.hookenv.relation_id')
133+ @patch('charmhelpers.core.hookenv.relations')
134+ @patch('charmhelpers.core.hookenv.relation_get')
135+ @patch('charmhelpers.core.hookenv.os')
136+ def test_gets_execution_environment_no_relation(
137+ self, os_, relations_get, relations, relation_id,
138+ local_unit, relation_type, config):
139+ config.return_value = 'some-config'
140+ relation_type.return_value = 'some-type'
141+ local_unit.return_value = 'some-unit'
142+ relation_id.return_value = None
143+ relations.return_value = 'all-relations'
144+ relations_get.return_value = 'some-relations'
145+ os_.environ = 'some-environment'
146+
147+ result = hookenv.execution_environment()
148+
149+ self.assertEqual(result, {
150+ 'conf': 'some-config',
151+ 'unit': 'some-unit',
152+ 'rels': 'all-relations',
153+ 'env': 'some-environment',
154+ })
155+
156+
157 @patch('charmhelpers.core.hookenv.os')
158 def test_gets_the_relation_id(self, os_):
159 os_.environ = {
160@@ -515,7 +530,7 @@
161 check_output.return_value = json.dumps(data)
162 result = hookenv.relation_get()
163
164- self.assertEqual(result.foo, 'BAR')
165+ self.assertEqual(result['foo'], 'BAR')
166 check_output.assert_called_with(['relation-get', '--format=json', '-'])
167
168 @patch('subprocess.check_output')
169@@ -697,3 +712,18 @@
170 self.assertRaises(hookenv.UnregisteredHookError, hooks.execute,
171 ['brew'])
172 self.assertEqual(execs, [True])
173+
174+ def test_magic_underscores(self):
175+ # Juju hook names use hypens as separators. Python functions use
176+ # underscores. If explicit names have not been provided, hooks
177+ # are registered with both the function name and the function
178+ # name with underscores replaced with hypens for convenience.
179+ execs = []
180+ hooks = hookenv.Hooks()
181+ @hooks.hook()
182+ def call_me_maybe():
183+ execs.append(True)
184+
185+ hooks.execute(['call-me-maybe'])
186+ hooks.execute(['call_me_maybe'])
187+ self.assertEqual(execs, [True, True])

Subscribers

People subscribed via source and target branches