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.
8 years ago · 0e160d5c7e
parent 1f78715848
commit 0e160d5c7e
17 changed files with 68 additions and 56 deletions
--- 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
--- 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)
--- 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
 }
--- 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()
--- 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__':
--- 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
--- 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
--- 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'


--- 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'
--- 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:
--- 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'
--- 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
--- 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
--- 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

--- 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,
--- 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:
--- 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)