From 0e160d5c7eca9aa5275c3f47d05c287a9e81401c Mon Sep 17 00:00:00 2001 From: Dag Wieers Date: Sun, 21 May 2017 00:25:57 +0200 Subject: [PATCH] Ensure exit_json returns failed = False This is required for modules that may return a non-zero `rc` value for a successful run, similar to #24865 for Windows fixing **win_chocolatey**. We also disable the dependency on `rc` value only, even if `failed` was set. Adapted unit and integration tests to the new scheme. Updated raw, shell, script, expect to take `rc` into account. --- lib/ansible/executor/task_executor.py | 11 +++++++---- lib/ansible/module_utils/basic.py | 3 +++ lib/ansible/module_utils/powershell.ps1 | 8 ++++++++ lib/ansible/modules/commands/command.py | 8 +++++++- lib/ansible/modules/commands/expect.py | 13 +++++++------ lib/ansible/plugins/action/raw.py | 4 ++++ lib/ansible/plugins/action/script.py | 4 ++++ .../integration/roles/test_ec2_eip/tasks/main.yml | 15 --------------- .../integration/targets/ec2_elb_lb/tasks/main.yml | 5 ----- test/integration/targets/ec2_group/tasks/main.yml | 5 ----- test/integration/targets/ec2_key/tasks/main.yml | 11 ----------- .../targets/failed_when/tasks/main.yml | 2 +- test/integration/targets/fetch/tasks/main.yml | 2 +- test/integration/targets/get_url/tasks/main.yml | 9 ++++----- test/integration/targets/win_find/tasks/main.yml | 8 ++++++++ .../targets/win_reg_stat/tasks/main.yml | 13 +++++++++++++ test/units/module_utils/basic/test_exit_json.py | 3 +-- 17 files changed, 68 insertions(+), 56 deletions(-) diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index ff6b747cb2..cc8276ec0d 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -570,10 +570,13 @@ class TaskExecutor: vars_copy.update(result['ansible_facts']) vars_copy.update({'ansible_facts': result['ansible_facts']}) - # set the failed property if the result has a non-zero rc. This will be - # overridden below if the failed_when property is set - if result.get('rc', 0) != 0: - result['failed'] = True + # set the failed property if it was missing. + if 'failed' not in result: + result['failed'] = False + + # set the changed property if it was missing. + if 'changed' not in result: + result['changed'] = False # if we didn't skip this task, use the helpers to evaluate the changed/ # failed_when properties diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index ac10d61294..973b5c6edf 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -2023,6 +2023,9 @@ class AnsibleModule(object): assert 'msg' in kwargs, "implementation error -- msg to explain the error is required" kwargs['failed'] = True + if not 'changed' in kwargs: + kwargs['changed'] = False + self.do_cleanup_files() self._return_formatted(kwargs) sys.exit(1) diff --git a/lib/ansible/module_utils/powershell.ps1 b/lib/ansible/module_utils/powershell.ps1 index bdcded3974..0823e98573 100644 --- a/lib/ansible/module_utils/powershell.ps1 +++ b/lib/ansible/module_utils/powershell.ps1 @@ -62,6 +62,10 @@ Function Exit-Json($obj) $obj = @{ } } + if (-not $obj.ContainsKey('changed')) { + Set-Attr $obj "changed" $false + } + echo $obj | ConvertTo-Json -Compress -Depth 99 Exit } @@ -88,6 +92,10 @@ Function Fail-Json($obj, $message = $null) Set-Attr $obj "msg" $message Set-Attr $obj "failed" $true + if (-not $obj.ContainsKey('changed')) { + Set-Attr $obj "changed" $false + } + echo $obj | ConvertTo-Json -Compress -Depth 99 Exit 1 } diff --git a/lib/ansible/modules/commands/command.py b/lib/ansible/modules/commands/command.py index 0f5037e1d2..01a83982bf 100644 --- a/lib/ansible/modules/commands/command.py +++ b/lib/ansible/modules/commands/command.py @@ -204,7 +204,7 @@ def main(): if err is None: err = b('') - module.exit_json( + result = dict( cmd = args, stdout = out.rstrip(b("\r\n")), stderr = err.rstrip(b("\r\n")), @@ -216,5 +216,11 @@ def main(): warnings = warnings ) + if rc != 0: + module.fail_json(msg='non-zero return code', **result) + + module.exit_json(**result) + + if __name__ == '__main__': main() diff --git a/lib/ansible/modules/commands/expect.py b/lib/ansible/modules/commands/expect.py index 2101e0562c..b7d3e08b06 100644 --- a/lib/ansible/modules/commands/expect.py +++ b/lib/ansible/modules/commands/expect.py @@ -221,7 +221,7 @@ def main(): if out is None: out = '' - ret = dict( + result = dict( cmd=args, stdout=out.rstrip('\r\n'), rc=rc, @@ -231,11 +231,12 @@ def main(): changed=True, ) - if rc is not None: - module.exit_json(**ret) - else: - ret['msg'] = 'command exceeded timeout' - module.fail_json(**ret) + if rc is None: + module.fail_json(msg='command exceeded timeout', **result) + elif rc != 0: + module.fail_json(msg='non-zero return code', **result) + + module.exit_json(**result) if __name__ == '__main__': diff --git a/lib/ansible/plugins/action/raw.py b/lib/ansible/plugins/action/raw.py index 34677457e7..cf43ee6994 100644 --- a/lib/ansible/plugins/action/raw.py +++ b/lib/ansible/plugins/action/raw.py @@ -42,4 +42,8 @@ class ActionModule(ActionBase): result['changed'] = True + if 'rc' in result and result['rc'] != 0: + result['failed'] = True + result['msg'] = 'non-zero return code' + return result diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index 2111246aca..9327ab204a 100644 --- a/lib/ansible/plugins/action/script.py +++ b/lib/ansible/plugins/action/script.py @@ -93,4 +93,8 @@ class ActionModule(ActionBase): result['changed'] = True + if 'rc' in result and result['rc'] != 0: + result['failed'] = True + result['msg'] = 'non-zero return code' + return result diff --git a/test/integration/roles/test_ec2_eip/tasks/main.yml b/test/integration/roles/test_ec2_eip/tasks/main.yml index 9f17f557e3..cb92902133 100644 --- a/test/integration/roles/test_ec2_eip/tasks/main.yml +++ b/test/integration/roles/test_ec2_eip/tasks/main.yml @@ -111,7 +111,6 @@ - name: assert new EIP was assigned assert: that: - - '"failed" not in result' - '"public_ip" in result' @@ -128,10 +127,6 @@ instance_id={{ instance_id }} register: result -- name: assert success disassociate EIP associated with instance - assert: - that: - - '"failed" not in result' # eip allocated:1 assigned:0 @@ -152,7 +147,6 @@ - name: assert new EIP was assigned assert: that: - - '"failed" not in result' - '"public_ip" in result' @@ -176,11 +170,6 @@ ec2_secret_key={{ ec2_secret_key }} register: result -- name: assert EIP deactivated - assert: - that: - - '"failed" not in result' - # eip allocated:0 assigned:0 @@ -205,7 +194,6 @@ - name: assert provision EIP with instance_id assert: that: - - '"failed" not in result' - '"public_ip" in result' @@ -245,7 +233,6 @@ - name: assert VPC EIP creation assert: that: - - '"failed" not in result' - '"public_ip" in result' @@ -268,7 +255,6 @@ #- name: assert new VPC EIP was assigned # assert: # that: -# - '"failed" not in result' # - '"public_ip" in result' # # @@ -307,7 +293,6 @@ - name: assert environment variable EC2_REGION assert: that: - - '"failed" not in result' - '"public_ip" in result' diff --git a/test/integration/targets/ec2_elb_lb/tasks/main.yml b/test/integration/targets/ec2_elb_lb/tasks/main.yml index 1e933c2fe5..728fef7b51 100644 --- a/test/integration/targets/ec2_elb_lb/tasks/main.yml +++ b/test/integration/targets/ec2_elb_lb/tasks/main.yml @@ -60,7 +60,6 @@ - assert: that: - 'info.changed' - - '"failed" not in info' - 'info.elb.status == "created"' - '"us-east-1c" in info.elb.zones' - '"us-east-1d" in info.elb.zones' @@ -124,7 +123,6 @@ - assert: that: - - '"failed" not in info' - 'info.elb.status == "ok"' - 'info.changed' - 'info.elb.zones[0] == "us-east-1b"' @@ -154,7 +152,6 @@ - assert: that: - - '"failed" not in info' - 'info.changed' - 'info.elb.status == "ok"' - '"us-east-1b" in info.elb.zones' @@ -187,7 +184,6 @@ - assert: that: - - '"failed" not in info' - 'info.elb.status == "ok"' - 'info.changed' - '[80, 81, "HTTP", "HTTP"] in info.elb.listeners' @@ -220,7 +216,6 @@ - assert: that: - - '"failed" not in info' - 'info.elb.status == "ok"' - 'info.changed' - '[80, 81, "HTTP", "HTTP"] in info.elb.listeners' diff --git a/test/integration/targets/ec2_group/tasks/main.yml b/test/integration/targets/ec2_group/tasks/main.yml index 4e5ae3f991..b4ff567d94 100644 --- a/test/integration/targets/ec2_group/tasks/main.yml +++ b/test/integration/targets/ec2_group/tasks/main.yml @@ -188,11 +188,6 @@ state=absent register: result - - name: assert state=absent - assert: - that: - - '"failed" not in result' - # ============================================================ - name: test state=present (expected changed=true) ec2_group: diff --git a/test/integration/targets/ec2_key/tasks/main.yml b/test/integration/targets/ec2_key/tasks/main.yml index 637d67ab0e..1881b5cfa6 100644 --- a/test/integration/targets/ec2_key/tasks/main.yml +++ b/test/integration/targets/ec2_key/tasks/main.yml @@ -157,11 +157,6 @@ state=absent register: result - - name: assert state=absent with key_material - assert: - that: - - '"failed" not in result' - # ============================================================ - name: test state=present without key_material ec2_key: @@ -177,7 +172,6 @@ assert: that: - 'result.changed' - - '"failed" not in result' - '"key" in result' - '"name" in result.key' - '"fingerprint" in result.key' @@ -200,7 +194,6 @@ assert: that: - 'result.changed' - - '"failed" not in result' - '"key" in result' - 'result.key == None' @@ -220,7 +213,6 @@ - name: assert state=present with key_material assert: that: - - '"failed" not in result' - 'result.changed == True' - '"key" in result' - '"name" in result.key' @@ -246,7 +238,6 @@ assert: that: - 'result.changed' - - '"failed" not in result' - '"key" in result' - 'result.key == None' @@ -324,7 +315,6 @@ assert: that: - 'result.changed' - - '"failed" not in result' - '"key" in result' - 'result.key == None' @@ -345,6 +335,5 @@ assert: that: - 'not result.changed' - - '"failed" not in result' - '"key" in result' - 'result.key == None' diff --git a/test/integration/targets/failed_when/tasks/main.yml b/test/integration/targets/failed_when/tasks/main.yml index a4a73665b4..3f8ae545f2 100644 --- a/test/integration/targets/failed_when/tasks/main.yml +++ b/test/integration/targets/failed_when/tasks/main.yml @@ -23,7 +23,7 @@ - assert: that: - - "'failed' not in result" + - "'failed' in result and not result.failed" - name: command rc 0 failed_when_result False shell: exit 0 diff --git a/test/integration/targets/fetch/tasks/main.yml b/test/integration/targets/fetch/tasks/main.yml index 7fb34b385e..03339dee81 100644 --- a/test/integration/targets/fetch/tasks/main.yml +++ b/test/integration/targets/fetch/tasks/main.yml @@ -95,7 +95,7 @@ - name: check fetch directory result assert: that: - - "failed_fetch_dir['failed']" + - "failed_fetch_dir|failed" - "fetch_dir.msg" - name: create symlink to a file that we can fetch diff --git a/test/integration/targets/get_url/tasks/main.yml b/test/integration/targets/get_url/tasks/main.yml index 5b87dffa13..f7a9fa505f 100644 --- a/test/integration/targets/get_url/tasks/main.yml +++ b/test/integration/targets/get_url/tasks/main.yml @@ -111,7 +111,7 @@ - name: Assert that the file was not downloaded assert: that: - - "result.failed == true" + - "result|failed" - "'Failed to validate the SSL certificate' in result.msg" - "stat_result.stat.exists == false" @@ -150,14 +150,13 @@ assert: that: - 'data_result.rc == 0' - - '"failed" not in get_url_result' when: "{{ python_has_ssl_context }}" # If the client doesn't support SNI then get_url should have failed with a certificate mismatch - name: Assert that hostname verification failed because SNI is not supported on this version of python assert: that: - - 'get_url_result["failed"]' + - 'get_url_result|failed' when: "{{ not python_has_ssl_context }}" # These tests are just side effects of how the site is hosted. It's not @@ -178,14 +177,14 @@ assert: that: - 'data_result.rc == 0' - - '"failed" not in get_url_result' + - 'not get_url_result|failed' when: "{{ python_has_ssl_context }}" # If the client doesn't support SNI then get_url should have failed with a certificate mismatch - name: Assert that hostname verification failed because SNI is not supported on this version of python assert: that: - - 'get_url_result["failed"]' + - 'get_url_result|failed' when: "{{ not python_has_ssl_context }}" # End hacky SNI test section diff --git a/test/integration/targets/win_find/tasks/main.yml b/test/integration/targets/win_find/tasks/main.yml index 524cfd890a..9d85310bc5 100644 --- a/test/integration/targets/win_find/tasks/main.yml +++ b/test/integration/targets/win_find/tasks/main.yml @@ -155,6 +155,7 @@ expected: changed: False examined: 5 + failed: False files: - { isarchive: True, attributes: Archive, @@ -237,6 +238,7 @@ expected: changed: False examined: 11 + failed: False files: - { isarchive: True, attributes: "Hidden, Archive", @@ -278,6 +280,7 @@ expected: changed: False examined: 5 + failed: False files: - { isarchive: True, attributes: Archive, @@ -315,6 +318,7 @@ expected: changed: False examined: 8 + failed: False files: - { isarchive: True, attributes: Archive, @@ -383,6 +387,7 @@ expected: changed: False examined: 10 + failed: False files: - { isarchive: True, attributes: Archive, @@ -465,6 +470,7 @@ expected: changed: False examined: 2 + failed: False files: - { isarchive: False, attributes: Directory, @@ -529,6 +535,7 @@ expected: changed: False examined: 5 + failed: False files: - { isarchive: True, attributes: Archive, @@ -571,6 +578,7 @@ expected: changed: False examined: 5 + failed: False files: - { isarchive: True, attributes: Archive, diff --git a/test/integration/targets/win_reg_stat/tasks/main.yml b/test/integration/targets/win_reg_stat/tasks/main.yml index eb4486011d..6793e75976 100644 --- a/test/integration/targets/win_reg_stat/tasks/main.yml +++ b/test/integration/targets/win_reg_stat/tasks/main.yml @@ -52,6 +52,7 @@ expected: changed: false exists: true + failed: false properties: binary: { raw_value: ["0x01", "0x16"], type: 'REG_BINARY', value: [1, 22] } dword: { raw_value: 1, type: 'REG_DWORD', value: 1 } @@ -79,6 +80,7 @@ expected: changed: false exists: true + failed: false properties: none: { raw_value: [], type: 'REG_NONE', value: [] } none1: { raw_value: ["0x00"], type: 'REG_NONE', value: [0] } @@ -101,6 +103,7 @@ expected: changed: false exists: true + failed: false properties: {} sub_keys: [] register: expected @@ -120,6 +123,7 @@ expected: changed: false exists: false + failed: false - name: validate test assert: @@ -137,6 +141,7 @@ expected: changed: false exists: true + failed: false raw_value: 'test' type: 'REG_SZ' value: 'test' @@ -157,6 +162,7 @@ expected: changed: false exists: true + failed: false raw_value: '%windir%\dir' type: 'REG_EXPAND_SZ' value: "{{win_dir_value.stdout_lines[0]}}\\dir" @@ -177,6 +183,7 @@ expected: changed: false exists: true + failed: false raw_value: ['a, b', 'c'] type: 'REG_MULTI_SZ' value: ['a, b', 'c'] @@ -197,6 +204,7 @@ expected: changed: false exists: true + failed: false raw_value: ["0x01", "0x16"] type: 'REG_BINARY' value: [1, 22] @@ -217,6 +225,7 @@ expected: changed: false exists: true + failed: false raw_value: 1 type: 'REG_DWORD' value: 1 @@ -237,6 +246,7 @@ expected: changed: false exists: true + failed: false raw_value: 1 type: 'REG_QWORD' value: 1 @@ -257,6 +267,7 @@ expected: changed: false exists: true + failed: false raw_value: [] type: 'REG_NONE' value: [] @@ -277,6 +288,7 @@ expected: changed: false exists: true + failed: false raw_value: ["0x00"] type: 'REG_NONE' value: [0] @@ -297,6 +309,7 @@ expected: changed: false exists: false + failed: false - name: validate test assert: diff --git a/test/units/module_utils/basic/test_exit_json.py b/test/units/module_utils/basic/test_exit_json.py index 6f52100a86..5e4d626c3a 100644 --- a/test/units/module_utils/basic/test_exit_json.py +++ b/test/units/module_utils/basic/test_exit_json.py @@ -81,7 +81,7 @@ class TestAnsibleModuleExitJson(unittest.TestCase): else: self.assertEquals(ctx.exception.code, 1) return_val = json.loads(self.fake_stream.getvalue()) - self.assertEquals(return_val, dict(msg="message", failed=True, invocation=empty_invocation)) + self.assertEquals(return_val, dict(msg="message", changed=False, failed=True, invocation=empty_invocation)) def test_exit_json_proper_changed(self): with self.assertRaises(SystemExit) as ctx: @@ -143,7 +143,6 @@ class TestAnsibleModuleExitValuesRemoved(unittest.TestCase): self.maxDiff = None for args, return_val, expected in self.dataset: expected = copy.deepcopy(expected) - del expected['changed'] expected['failed'] = True params = dict(ANSIBLE_MODULE_ARGS=args) params = json.dumps(params)