Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Tweaked merge_hash to also affect Runner behavior #2476

Closed
wants to merge 6 commits into from

2 participants

George Miroshnykov Michael DeHaan
George Miroshnykov

This pull request should contribute to #2329.
Hashes loaded from vars and vars_files should now be merged when merge_hash is set to True in ansible.cfg.

I'm not very experienced with Python, so please let me know if I can improve something.

Michael DeHaan
Collaborator

This looks nice and clean, in queue for testing, thanks!

Michael DeHaan
Collaborator

Hi @laggyluke if you run "make tests" this causes about 13 new test failures (there is one lineinfile test that we know to be broken and are working on fixing).

Can you take a look at what is going on? Thanks!

George Miroshnykov

Sure, I just need to find some time to set up a testing infrastructure.

George Miroshnykov

I believe I've fixed it - the only test failing is the one that you've mentioned (lineinfile).
Now I have a testing environment set up properly, so I guess I don't have any excuses not to write some new tests :)
I hope to do it this weekend, so you may wish to hold back this PR until then.

George Miroshnykov

I've added some basic tests for two hash behaviors - replace and merge.

Unfortunately I spent several hours trying to come up with a clean way to test it, but eventually gave up and just rendered the hash via template into a temporary file.
If you have any suggestions how to test this better, please let me know.

Michael DeHaan
Collaborator

You have a merge commit in here, can you please resubmit this.

always use 'git pull --rebase' and not 'git pull' and never 'git merge', but always 'git rebase'.

I'd also like to have this as a single squashed commit versus the commit + the fixes, you should probably be able to force push to the same branch to replace things, but it is always a good idea to start a new branch for each feature to keep things simple.

Michael DeHaan mpdehaan closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
12 lib/ansible/inventory/vars_plugins/group_vars.py
View
@@ -49,11 +49,7 @@ def run(self, host):
data = utils.parse_yaml_from_file(path)
if type(data) != dict:
raise errors.AnsibleError("%s must be stored as a dictionary/hash" % path)
- if C.DEFAULT_HASH_BEHAVIOUR == "merge":
- # let data content override results if needed
- results = utils.merge_hash(results, data)
- else:
- results.update(data)
+ results = utils.combine_vars(results, data);
# load vars in inventory_dir/hosts_vars/name_of_host
path = os.path.join(basedir, "host_vars/%s" % host.name)
@@ -61,10 +57,6 @@ def run(self, host):
data = utils.parse_yaml_from_file(path)
if type(data) != dict:
raise errors.AnsibleError("%s must be stored as a dictionary/hash" % path)
- if C.DEFAULT_HASH_BEHAVIOUR == "merge":
- # let data content override results if needed
- results = utils.merge_hash(results, data)
- else:
- results.update(data)
+ results = utils.combine_vars(results, data);
return results
5 lib/ansible/playbook/play.py
View
@@ -328,8 +328,9 @@ def _update_vars_files_for_host(self, host):
if host is not None and self._has_vars_in(filename2) and not self._has_vars_in(filename3):
# running a host specific pass and has host specific variables
# load into setup cache
- self.playbook.SETUP_CACHE[host].update(new_vars)
+ self.playbook.SETUP_CACHE[host] = utils.combine_vars(
+ self.playbook.SETUP_CACHE[host], new_vars)
self.playbook.callbacks.on_import_for_host(host, filename4)
elif host is None:
# running a non-host specific pass and we can update the global vars instead
- self.vars.update(new_vars)
+ self.vars = utils.combine_vars(self.vars, new_vars)
18 lib/ansible/runner/__init__.py
View
@@ -175,7 +175,7 @@ def __init__(self,
# ensure we are using unique tmp paths
random.seed()
-
+
# *****************************************************
def _complex_args_hack(self, complex_args, module_args):
@@ -333,9 +333,9 @@ def _executor_internal(self, host):
port = self.remote_port
inject = {}
- inject.update(host_variables)
- inject.update(self.module_vars)
- inject.update(self.setup_cache[host])
+ inject = utils.combine_vars(inject, host_variables)
+ inject = utils.combine_vars(inject, self.module_vars)
+ inject = utils.combine_vars(inject, self.setup_cache[host])
inject['hostvars'] = HostVars(self.setup_cache, self.inventory)
inject['group_names'] = host_variables.get('group_names', [])
inject['groups'] = self.inventory.groups_list()
@@ -489,7 +489,7 @@ def _executor_internal_inner(self, host, module_name, module_args, inject, port,
# all modules get a tempdir, action plugins get one unless they have NEEDS_TMPPATH set to False
if getattr(handler, 'NEEDS_TMPPATH', True):
tmp = self._make_tmp_path(conn)
-
+
result = handler.run(conn, tmp, module_name, module_args, inject, complex_args)
conn.close()
@@ -622,8 +622,8 @@ def _copy_module(self, conn, tmp, module_name, module_args, inject, complex_args
module_data = f.read()
if module_common.REPLACER in module_data:
is_new_style=True
-
- complex_args_json = utils.jsonify(complex_args)
+
+ complex_args_json = utils.jsonify(complex_args)
encoded_args = "\"\"\"%s\"\"\"" % module_args.replace("\"","\\\"")
encoded_lang = "\"\"\"%s\"\"\"" % C.DEFAULT_MODULE_LANG
encoded_complex = "\"\"\"%s\"\"\"" % complex_args_json.replace("\\", "\\\\")
@@ -632,7 +632,7 @@ def _copy_module(self, conn, tmp, module_name, module_args, inject, complex_args
module_data = module_data.replace(module_common.REPLACER_ARGS, encoded_args)
module_data = module_data.replace(module_common.REPLACER_LANG, encoded_lang)
module_data = module_data.replace(module_common.REPLACER_COMPLEX, encoded_complex)
-
+
if is_new_style:
facility = C.DEFAULT_SYSLOG_FACILITY
if 'ansible_syslog_facility' in inject:
@@ -734,7 +734,7 @@ def run(self):
# run once per hostgroup, rather than pausing once per each
# host.
p = utils.plugins.action_loader.get(self.module_name, self)
-
+
if p and getattr(p, 'BYPASS_HOST_LOOP', None):
# Expose the current hostgroup to the bypassing plugins
9 lib/ansible/utils/__init__.py
View
@@ -300,7 +300,7 @@ def merge_hash(a, b):
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
+ # we recursively call ourselves with
# the key value of b
a[k] = merge_hash(a[k], v)
else:
@@ -655,8 +655,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 combine_vars(a, b):
+ if C.DEFAULT_HASH_BEHAVIOUR == "merge":
+ return merge_hash(a, b)
+ else:
+ return dict(a.items() + b.items())
69 test/TestPlayBook.py
View
@@ -10,6 +10,7 @@
import ansible.callbacks as ans_callbacks
import os
import shutil
+import ansible.constants as C
EVENTS = []
@@ -93,6 +94,8 @@ def setUp(self):
os.unlink('/tmp/ansible_test_data_copy.out')
if os.path.exists('/tmp/ansible_test_data_template.out'):
os.unlink('/tmp/ansible_test_data_template.out')
+ if os.path.exists('/tmp/ansible_test_messages.out'):
+ os.unlink('/tmp/ansible_test_messages.out')
def _prepare_stage_dir(self):
stage_path = os.path.join(self.test_dir, 'test_data')
@@ -233,3 +236,69 @@ def test_yaml_hosts_list(self):
play = ansible.playbook.Play(playbook, playbook.playbook[0], os.getcwd())
assert play.hosts == ';'.join(('host1', 'host2', 'host3'))
+ def test_playbook_hash_replace(self):
+ # save default hash behavior so we can restore it in the end of the test
+ saved_hash_behavior = C.DEFAULT_HASH_BEHAVIOUR
+ C.DEFAULT_HASH_BEHAVIOUR = "replace"
+
+ test_callbacks = TestCallbacks()
+ playbook = ansible.playbook.PlayBook(
+ playbook=os.path.join(self.test_dir, 'test_hash_behavior', 'playbook.yml'),
+ host_list='test/ansible_hosts',
+ stats=ans_callbacks.AggregateStats(),
+ callbacks=test_callbacks,
+ runner_callbacks=test_callbacks
+ )
+ playbook.run()
+
+ with open('/tmp/ansible_test_messages.out') as f:
+ actual = [l.strip() for l in f.readlines()]
+
+ print "**ACTUAL**"
+ print actual
+
+ expected = [
+ "goodbye: Goodbye World!"
+ ]
+
+ print "**EXPECTED**"
+ print expected
+
+ assert actual == expected
+
+ # restore default hash behavior
+ C.DEFAULT_HASH_BEHAVIOUR = saved_hash_behavior
+
+ def test_playbook_hash_merge(self):
+ # save default hash behavior so we can restore it in the end of the test
+ saved_hash_behavior = C.DEFAULT_HASH_BEHAVIOUR
+ C.DEFAULT_HASH_BEHAVIOUR = "merge"
+
+ test_callbacks = TestCallbacks()
+ playbook = ansible.playbook.PlayBook(
+ playbook=os.path.join(self.test_dir, 'test_hash_behavior', 'playbook.yml'),
+ host_list='test/ansible_hosts',
+ stats=ans_callbacks.AggregateStats(),
+ callbacks=test_callbacks,
+ runner_callbacks=test_callbacks
+ )
+ playbook.run()
+
+ with open('/tmp/ansible_test_messages.out') as f:
+ actual = [l.strip() for l in f.readlines()]
+
+ print "**ACTUAL**"
+ print actual
+
+ expected = [
+ "hello: Hello World!",
+ "goodbye: Goodbye World!"
+ ]
+
+ print "**EXPECTED**"
+ print expected
+
+ assert actual == expected
+
+ # restore default hash behavior
+ C.DEFAULT_HASH_BEHAVIOUR = saved_hash_behavior
3  test/test_hash_behavior/goodbye.yml
View
@@ -0,0 +1,3 @@
+---
+messages:
+ goodbye: "Goodbye World!"
3  test/test_hash_behavior/hello.yml
View
@@ -0,0 +1,3 @@
+---
+messages:
+ hello: "Hello World!"
3  test/test_hash_behavior/message.j2
View
@@ -0,0 +1,3 @@
+{% for k, v in messages.iteritems() %}
+{{ k }}: {{ v }}
+{% endfor %}
11 test/test_hash_behavior/playbook.yml
View
@@ -0,0 +1,11 @@
+---
+- hosts: all
+ connection: local
+
+ vars_files:
+ - hello.yml
+ - goodbye.yml
+
+ tasks:
+ - name: generate messages
+ action: template src=message.j2 dest=/tmp/ansible_test_messages.out
Something went wrong with that request. Please try again.