Merge pull request #3114 from laggyluke/roles_combine_vars

Added basic support for hash_behaviour=merge in roles

lib/ansible/playbook/play.py

@@ -77,11 +77,11 @@ class Play(object):
         # tasks/handlers as they may have inventory scope overrides
         _tasks    = ds.pop('tasks', [])
         _handlers = ds.pop('handlers', [])
-        ds = template(basedir, ds, self.vars) 
+        ds = template(basedir, ds, self.vars)
         ds['tasks'] = _tasks
         ds['handlers'] = _handlers
 
-        self._ds = ds         
+        self._ds = ds
 
         hosts = ds.get('hosts')
         if hosts is None:
@@ -114,7 +114,7 @@ class Play(object):
 
         if self.sudo_user != 'root':
             self.sudo = True
-        
+
 
     # *************************************************
 
@@ -146,7 +146,7 @@ class Play(object):
         # flush handlers after pre_tasks
         new_tasks.append(dict(meta='flush_handlers'))
 
-        # variables if the role was parameterized (i.e. given as a hash) 
+        # variables if the role was parameterized (i.e. given as a hash)
         has_dict = {}
 
         for orig_path in roles:
@@ -178,14 +178,14 @@ class Play(object):
                 raise errors.AnsibleError("found role at %s, but cannot find %s or %s or %s or %s" % (path, task, handler, vars_file, library))
             if os.path.isfile(task):
                 nt = dict(include=task, vars=has_dict)
-                if when: 
+                if when:
                     nt['when'] = when
                 if with_items:
                     nt['with_items'] = with_items
                 new_tasks.append(nt)
             if os.path.isfile(handler):
                 nt = dict(include=handler, vars=has_dict)
-                if when: 
+                if when:
                     nt['when'] = when
                 if with_items:
                     nt['with_items'] = with_items
@@ -242,7 +242,7 @@ class Play(object):
                 if x['meta'] == 'flush_handlers':
                     results.append(Task(self,x))
                     continue
- 
+
             task_vars = self.vars.copy()
             task_vars.update(vars)
             if original_file:
@@ -269,7 +269,7 @@ class Play(object):
                         raise errors.AnsibleError("parse error: task includes cannot be used with other directives: %s" % k)
 
                 if 'vars' in x:
-                    task_vars.update(x['vars'])
+                    task_vars = utils.combine_vars(task_vars, x['vars'])
                 if 'only_if' in x:
                     included_additional_conditions.append(x['only_if'])
 
@@ -281,7 +281,7 @@ class Play(object):
                         mv[k] = template(self.basedir, v, mv)
                     dirname = self.basedir
                     if original_file:
-                        dirname = os.path.dirname(original_file)     
+                        dirname = os.path.dirname(original_file)
                     include_file = template(dirname, tokens[0], mv)
                     include_filename = utils.path_dwim(dirname, include_file)
                     data = utils.parse_yaml_from_file(include_filename)

lib/ansible/utils/__init__.py

@@ -335,24 +335,23 @@ def parse_kv(args):
     return options
 
 def merge_hash(a, b):
-    ''' merges hash b into a
-    this means that if b has key k, the resulting has will have a key k
-    which value comes from b
-    said differently, all key/value combination from b will override a's '''
+    ''' recursively merges hash b into a
+    keys from b take precedende over keys from a '''
 
-    # and iterate over b keys
+    result = copy.deepcopy(a)
+
+    # next, iterate over b keys and values
     for k, v in b.iteritems():
-        if k in a and isinstance(a[k], dict):
-            # if this key is a hash and exists in a
-            # we recursively call ourselves with
-            # the key value of b
-            a[k] = merge_hash(a[k], v)
+        # if there's already such key in a
+        # and that key contains dict
+        if k in result and isinstance(result[k], dict):
+            # merge those dicts recursively
+            result[k] = merge_hash(a[k], v)
         else:
-            # k is not in a, no need to merge b, we just deecopy
-            # or k is not a dictionnary, no need to merge b either, we just deecopy it
-            a[k] = v
-    # finally, return the resulting hash when we're done iterating keys
-    return a
+            # otherwise, just copy a value from b to a
+            result[k] = v
+
+    return result
 
 def md5s(data):
     ''' Return MD5 hex digest of data. '''
@@ -604,7 +603,7 @@ def compile_when_to_only_if(expression):
     # when: int $x in $alist
     # when: float $x > 2 and $y <= $z
     # when: str $x != $y
-    # when: jinja2_compare asdf  # implies {{ asdf }} 
+    # when: jinja2_compare asdf  # implies {{ asdf }}
 
     if type(expression) not in [ str, unicode ]:
         raise errors.AnsibleError("invalid usage of when_ operator: %s" % expression)
@@ -723,13 +722,13 @@ def get_diff(diff):
         return ">> the files are different, but the diff library cannot compare unicode strings"
 
 def is_list_of_strings(items):
-    for x in items: 
+    for x in items:
         if not isinstance(x, basestring):
             return False
     return True
 
 def safe_eval(str):
-    ''' 
+    '''
     this is intended for allowing things like:
     with_items: {{ a_list_variable }}
     where Jinja2 would return a string
@@ -737,7 +736,7 @@ def safe_eval(str):
     the env is constrained)
     '''
     # FIXME: is there a more native way to do this?
-    
+
     def is_set(var):
         return not var.startswith("$") and not '{{' in var
 
@@ -776,7 +775,7 @@ def listify_lookup_plugin_terms(terms, basedir, inject):
                 if isinstance(new_terms, basestring) and new_terms.find("{{") != -1:
                     pass
                 else:
-                    terms = new_terms  
+                    terms = new_terms
             except:
                 pass
 

test/TestPlayBook.py

@@ -96,6 +96,8 @@ class TestPlaybook(unittest.TestCase):
            os.unlink('/tmp/ansible_test_data_template.out')
        if os.path.exists('/tmp/ansible_test_messages.out'):
            os.unlink('/tmp/ansible_test_messages.out')
+       if os.path.exists('/tmp/ansible_test_role_messages.out'):
+           os.unlink('/tmp/ansible_test_role_messages.out')
 
    def _prepare_stage_dir(self):
        stage_path = os.path.join(self.test_dir, 'test_data')
@@ -304,20 +306,17 @@ class TestPlaybook(unittest.TestCase):
       )
       playbook.run()
 
-      with open('/tmp/ansible_test_messages.out') as f:
-        actual = [l.strip() for l in f.readlines()]
-
-      print "**ACTUAL**"
-      print actual
-
-      expected = [
+      filename = '/tmp/ansible_test_messages.out'
+      expected_lines = [
         "goodbye: Goodbye World!"
       ]
+      self._compare_file_output(filename, expected_lines)
 
-      print "**EXPECTED**"
-      print expected
-
-      assert actual == expected
+      filename = '/tmp/ansible_test_role_messages.out'
+      expected_lines = [
+        "inside_a_role: Indeed!"
+      ]
+      self._compare_file_output(filename, expected_lines)
 
       # restore default hash behavior
       C.DEFAULT_HASH_BEHAVIOUR = saved_hash_behavior
@@ -337,21 +336,34 @@ class TestPlaybook(unittest.TestCase):
       )
       playbook.run()
 
-      with open('/tmp/ansible_test_messages.out') as f:
-        actual = [l.strip() for l in f.readlines()]
-
-      print "**ACTUAL**"
-      print actual
+      filename = '/tmp/ansible_test_messages.out'
+      expected_lines = [
+        "goodbye: Goodbye World!",
+        "hello: Hello World!"
+      ]
+      self._compare_file_output(filename, expected_lines)
 
-      expected = [
+      filename = '/tmp/ansible_test_role_messages.out'
+      expected_lines = [
+        "goodbye: Goodbye World!",
         "hello: Hello World!",
-        "goodbye: Goodbye World!"
+        "inside_a_role: Indeed!"
       ]
-
-      print "**EXPECTED**"
-      print expected
-
-      assert actual == expected
+      self._compare_file_output(filename, expected_lines)
 
       # restore default hash behavior
       C.DEFAULT_HASH_BEHAVIOUR = saved_hash_behavior
+
+   def _compare_file_output(self, filename, expected_lines):
+      actual_lines = []
+      with open(filename) as f:
+        actual_lines = [l.strip() for l in f.readlines()]
+        actual_lines = sorted(actual_lines)
+
+      print "**ACTUAL**"
+      print actual_lines
+
+      print "**EXPECTED**"
+      print expected_lines
+
+      assert actual_lines == expected_lines

test/test_hash_behavior/playbook.yml

@@ -9,3 +9,8 @@
   tasks:
     - name: generate messages
       action: template src=message.j2 dest=/tmp/ansible_test_messages.out
+
+  roles:
+    - role: hash_behavior_test_role
+      messages:
+        inside_a_role: "Indeed!"

test/test_hash_behavior/roles/hash_behavior_test_role/tasks/main.yml

@@ -0,0 +1,2 @@
+- name: generate role messages
+  action: template src=role_message.j2 dest=/tmp/ansible_test_role_messages.out

test/test_hash_behavior/roles/hash_behavior_test_role/templates/role_message.j2

@@ -0,0 +1,3 @@
+{% for k, v in messages.iteritems() %}
+{{ k }}: {{ v }}
+{% endfor %}