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

[Stuart Bishop]

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.

[Matthew Wedgwood (mew)]

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

review: Approve

[Matthew Wedgwood (mew)]

review: Needs Fixing

[Matthew Wedgwood (mew)]

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

review: Needs Fixing

[lp:~stub/charm-helpers/bug-1182959-no-implicit-serializable-magic]

31. By Stuart Bishop on 2013-06-27

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

32. By Stuart Bishop on 2013-06-28

Fix tests

[Stuart Bishop (stub)]

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

[lp:~stub/charm-helpers/bug-1182959-no-implicit-serializable-magic]

33. By Stuart Bishop on 2013-06-28

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

[Matthew Wedgwood (mew)]

Perfect, thanks.

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2013-06-20 08:32:11 +0000
+++ charmhelpers/core/hookenv.py	2013-06-28 17:11:26 +0000
@@ -108,11 +108,12 @@
     """A convenient bundling of the current execution context"""
     context = {}
     context['conf'] = config()
-    context['reltype'] = relation_type()
-    context['relid'] = relation_id()
+    if relation_id():
+        context['reltype'] = relation_type()
+        context['relid'] = relation_id()
+        context['rel'] = relation_get()
     context['unit'] = local_unit()
     context['rels'] = relations()
-    context['rel'] = relation_get()
     context['env'] = os.environ
     return context
 
@@ -150,13 +151,9 @@
         config_cmd_line.append(scope)
     config_cmd_line.append('--format=json')
     try:
-        value = json.loads(subprocess.check_output(config_cmd_line))
+        return json.loads(subprocess.check_output(config_cmd_line))
     except ValueError:
         return None
-    if isinstance(value, dict):
-        return Serializable(value)
-    else:
-        return value
 
 
 @cached
@@ -169,13 +166,9 @@
     if unit:
         _args.append(unit)
     try:
-        value = json.loads(subprocess.check_output(_args))
+        return json.loads(subprocess.check_output(_args))
     except ValueError:
         return None
-    if isinstance(value, dict):
-        return Serializable(value)
-    else:
-        return value
 
 
 def relation_set(relation_id=None, relation_settings={}, **kwargs):
@@ -222,7 +215,7 @@
         if key.endswith('-list'):
             relation[key] = relation[key].split()
     relation['__unit__'] = unit
-    return Serializable(relation)
+    return relation
 
 
 @cached
@@ -331,5 +324,8 @@
                 self.register(hook_name, decorated)
             else:
                 self.register(decorated.__name__, decorated)
+                if '_' in decorated.__name__:
+                    self.register(
+                        decorated.__name__.replace('_', '-'), decorated)
             return decorated
         return wrapper

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2013-06-20 08:53:31 +0000
+++ tests/core/test_hookenv.py	2013-06-28 17:11:26 +0000
@@ -140,17 +140,6 @@
             mock_call.assert_called_with(['juju-log', '-l', level, 'foo'])
 
     @patch('subprocess.check_output')
-    def test_gets_charm_config_as_serializable(self, check_output):
-        config_data = {'foo': 'bar'}
-        check_output.return_value = json.dumps(config_data)
-
-        result = hookenv.config()
-
-        self.assert_(isinstance(result, hookenv.Serializable))
-        self.assertEqual(result.foo, 'bar')
-        check_output.assert_called_with(['config-get', '--format=json'])
-
-    @patch('subprocess.check_output')
     def test_gets_charm_config_with_scope(self, check_output):
         config_data = 'bar'
         check_output.return_value = json.dumps(config_data)
@@ -311,15 +300,14 @@
         unit = 'foo-unit'
         raw_relation = {
             'foo': 'bar',
-            'baz-list': '1 2 3',
         }
         remote_unit.return_value = unit
         relation_get.return_value = raw_relation
 
         result = hookenv.relation_for_unit()
 
-        self.assertEqual(result.__unit__, unit)
-        self.assertEqual(getattr(result, 'baz-list'), ['1', '2', '3'])
+        self.assertEqual(result['__unit__'], unit)
+        self.assertEqual(result['foo'], 'bar')
         relation_get.assert_called_with(unit=unit, rid=None)
 
     @patch('charmhelpers.core.hookenv.remote_unit')
@@ -328,14 +316,13 @@
         unit = 'foo-unit'
         raw_relation = {
             'foo': 'bar',
-            'baz-list': '1 2 3',
         }
         relation_get.return_value = raw_relation
 
         result = hookenv.relation_for_unit(unit)
 
-        self.assertEqual(result.__unit__, unit)
-        self.assertEqual(getattr(result, 'baz-list'), ['1', '2', '3'])
+        self.assertEqual(result['__unit__'], unit)
+        self.assertEqual(result['foo'], 'bar')
         relation_get.assert_called_with(unit=unit, rid=None)
         self.assertFalse(remote_unit.called)
 
@@ -496,6 +483,34 @@
             'env': 'some-environment',
         })
 
+    @patch('charmhelpers.core.hookenv.config')
+    @patch('charmhelpers.core.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.local_unit')
+    @patch('charmhelpers.core.hookenv.relation_id')
+    @patch('charmhelpers.core.hookenv.relations')
+    @patch('charmhelpers.core.hookenv.relation_get')
+    @patch('charmhelpers.core.hookenv.os')
+    def test_gets_execution_environment_no_relation(
+        self, os_, relations_get, relations, relation_id,
+        local_unit, relation_type, config):
+        config.return_value = 'some-config'
+        relation_type.return_value = 'some-type'
+        local_unit.return_value = 'some-unit'
+        relation_id.return_value = None
+        relations.return_value = 'all-relations'
+        relations_get.return_value = 'some-relations'
+        os_.environ = 'some-environment'
+
+        result = hookenv.execution_environment()
+
+        self.assertEqual(result, {
+            'conf': 'some-config',
+            'unit': 'some-unit',
+            'rels': 'all-relations',
+            'env': 'some-environment',
+        })
+
+
     @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_relation_id(self, os_):
         os_.environ = {
@@ -515,7 +530,7 @@
         check_output.return_value = json.dumps(data)
         result = hookenv.relation_get()
 
-        self.assertEqual(result.foo, 'BAR')
+        self.assertEqual(result['foo'], 'BAR')
         check_output.assert_called_with(['relation-get', '--format=json', '-'])
 
     @patch('subprocess.check_output')
@@ -697,3 +712,18 @@
         self.assertRaises(hookenv.UnregisteredHookError, hooks.execute,
                           ['brew'])
         self.assertEqual(execs, [True])
+
+    def test_magic_underscores(self):
+        # Juju hook names use hypens as separators. Python functions use
+        # underscores. If explicit names have not been provided, hooks
+        # are registered with both the function name and the function
+        # name with underscores replaced with hypens for convenience.
+        execs = []
+        hooks = hookenv.Hooks()
+        @hooks.hook()
+        def call_me_maybe():
+            execs.append(True)
+
+        hooks.execute(['call-me-maybe'])
+        hooks.execute(['call_me_maybe'])
+        self.assertEqual(execs, [True, True])