From 81e34256af9c2b1f16f4f8d4a171a7c83a633094 Mon Sep 17 00:00:00 2001 From: Benoit Bayszczak Date: Wed, 12 Sep 2018 16:34:17 +0200 Subject: [PATCH] replace dict.update by a dict merge in zuul_return This fixes an issue when using zuul_return several times. zuul_return module takes a dictionary as a parameter with one root key: 'zuul' Using dict.update with two identical keys will cause dict key overriding When using zuul_return in child and parent playbooks, the parent zuul_return value overwrites the child zuul_return value Change-Id: I6308020d2af0670eb5c598a3daf0ef51066651f6 --- .../playbooks/several-zuul-return-child.yaml | 6 ++++++ .../several-zuul-return-parent-post.yaml | 6 ++++++ .../data-return/git/common-config/zuul.yaml | 18 +++++++++++++++++ .../data-return/git/org_project4/README | 1 + tests/fixtures/config/data-return/main.yaml | 1 + tests/unit/test_v3.py | 14 +++++++++++++ zuul/ansible/library/zuul_return.py | 20 +++++++++++++++++-- 7 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-child.yaml create mode 100644 tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-parent-post.yaml create mode 100644 tests/fixtures/config/data-return/git/org_project4/README diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-child.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-child.yaml new file mode 100644 index 0000000000..ee74d28844 --- /dev/null +++ b/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-child.yaml @@ -0,0 +1,6 @@ +- hosts: localhost + tasks: + - zuul_return: + data: + zuul: + child_jobs: [] diff --git a/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-parent-post.yaml b/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-parent-post.yaml new file mode 100644 index 0000000000..5e412c3665 --- /dev/null +++ b/tests/fixtures/config/data-return/git/common-config/playbooks/several-zuul-return-parent-post.yaml @@ -0,0 +1,6 @@ +- hosts: localhost + tasks: + - zuul_return: + data: + zuul: + log_url: http://example.com/test/log/url/ diff --git a/tests/fixtures/config/data-return/git/common-config/zuul.yaml b/tests/fixtures/config/data-return/git/common-config/zuul.yaml index bb789b95e0..f8cc624073 100644 --- a/tests/fixtures/config/data-return/git/common-config/zuul.yaml +++ b/tests/fixtures/config/data-return/git/common-config/zuul.yaml @@ -41,6 +41,15 @@ name: child run: playbooks/child.yaml +- job: + name: several-zuul-return-parent + post-run: playbooks/several-zuul-return-parent-post.yaml + +- job: + name: several-zuul-return-child + parent: several-zuul-return-parent + run: playbooks/several-zuul-return-child.yaml + - project: name: org/project check: @@ -84,3 +93,12 @@ - child: dependencies: - data-return-skip-all + +- project: + name: org/project4 + check: + jobs: + - several-zuul-return-child + - data-return: + dependencies: + - several-zuul-return-child diff --git a/tests/fixtures/config/data-return/git/org_project4/README b/tests/fixtures/config/data-return/git/org_project4/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/data-return/git/org_project4/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/data-return/main.yaml b/tests/fixtures/config/data-return/main.yaml index 919921e916..3d7a5c9c2d 100644 --- a/tests/fixtures/config/data-return/main.yaml +++ b/tests/fixtures/config/data-return/main.yaml @@ -9,3 +9,4 @@ - org/project1 - org/project2 - org/project3 + - org/project4 diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index d871a2d80b..e203059b9a 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -3084,6 +3084,20 @@ class TestDataReturn(AnsibleZuulTestCase): self.assertIn('data-return : SKIPPED', A.messages[-1]) self.assertIn('Build succeeded', A.messages[-1]) + def test_several_zuul_return(self): + A = self.fake_gerrit.addFakeChange('org/project4', 'master', 'A') + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + self.assertHistory([ + dict(name='several-zuul-return-child', result='SUCCESS', + changes='1,1'), + ]) + self.assertIn( + '- several-zuul-return-child http://example.com/test/log/url/', + A.messages[-1]) + self.assertIn('data-return : SKIPPED', A.messages[-1]) + self.assertIn('Build succeeded', A.messages[-1]) + class TestDiskAccounting(AnsibleZuulTestCase): config_file = 'zuul-disk-accounting.conf' diff --git a/zuul/ansible/library/zuul_return.py b/zuul/ansible/library/zuul_return.py index 4935226518..8c177195a8 100644 --- a/zuul/ansible/library/zuul_return.py +++ b/zuul/ansible/library/zuul_return.py @@ -20,6 +20,22 @@ import json import tempfile +def merge_dict(dict_a, dict_b): + """ + Add dict_a into dict_b + Merge values if possible else dict_a value replace dict_b value + """ + for key in dict_a: + if key in dict_b: + if isinstance(dict_a[key], dict) and isinstance(dict_b[key], dict): + merge_dict(dict_a[key], dict_b[key]) + else: + dict_b[key] = dict_a[key] + else: + dict_b[key] = dict_a[key] + return dict_b + + def set_value(path, new_data, new_file): workdir = os.path.dirname(path) data = None @@ -33,9 +49,9 @@ def set_value(path, new_data, new_file): if new_file: with open(new_file, 'r') as f: - data.update(json.load(f)) + merge_dict(json.load(f), data) if new_data: - data.update(new_data) + merge_dict(new_data, data) (f, tmp_path) = tempfile.mkstemp(dir=workdir) try: