From 8f4f3750fe24ec2d1bd295b797b1b71e0d0ddd29 Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Thu, 23 May 2019 10:17:17 -0400 Subject: [PATCH] Ensure uri module always returns status even on failure (#56240) - Also return url and update docs for other values to indicate they are only returned on success. - Add integration tests - Use info variable for common return values - Use -1 as default status rather than None. This is lines up with with existing code in urls.py - Add unit tests to ensure status and url are returned on failure --- changelogs/fragments/55897-uri-correct-return-values.yaml | 2 ++ lib/ansible/module_utils/urls.py | 8 ++++---- lib/ansible/modules/net_tools/basics/uri.py | 4 ++-- test/integration/targets/uri/tasks/main.yml | 7 +++++-- test/units/module_utils/urls/test_fetch_url.py | 6 ++++++ 5 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 changelogs/fragments/55897-uri-correct-return-values.yaml diff --git a/changelogs/fragments/55897-uri-correct-return-values.yaml b/changelogs/fragments/55897-uri-correct-return-values.yaml new file mode 100644 index 00000000000..4b6d3bd8e5a --- /dev/null +++ b/changelogs/fragments/55897-uri-correct-return-values.yaml @@ -0,0 +1,2 @@ +bugfixes: + - uri - always return a value for status even during failure (https://github.com/ansible/ansible/issues/55897) diff --git a/lib/ansible/module_utils/urls.py b/lib/ansible/module_utils/urls.py index 895ae71487c..fdde2ee2611 100644 --- a/lib/ansible/module_utils/urls.py +++ b/lib/ansible/module_utils/urls.py @@ -1429,7 +1429,7 @@ def fetch_url(module, url, data=None, headers=None, method=None, cookies = cookiejar.LWPCookieJar() r = None - info = dict(url=url) + info = dict(url=url, status=-1) try: r = open_url(url, data=data, headers=headers, method=method, use_proxy=use_proxy, force=force, last_mod_time=last_mod_time, timeout=timeout, @@ -1471,11 +1471,11 @@ def fetch_url(module, url, data=None, headers=None, method=None, except NoSSLError as e: distribution = get_distribution() if distribution is not None and distribution.lower() == 'redhat': - module.fail_json(msg='%s. You can also install python-ssl from EPEL' % to_native(e)) + module.fail_json(msg='%s. You can also install python-ssl from EPEL' % to_native(e), **info) else: - module.fail_json(msg='%s' % to_native(e)) + module.fail_json(msg='%s' % to_native(e), **info) except (ConnectionError, ValueError) as e: - module.fail_json(msg=to_native(e)) + module.fail_json(msg=to_native(e), **info) except urllib_error.HTTPError as e: try: body = e.read() diff --git a/lib/ansible/modules/net_tools/basics/uri.py b/lib/ansible/modules/net_tools/basics/uri.py index f1c0c747b63..eea77d8a858 100644 --- a/lib/ansible/modules/net_tools/basics/uri.py +++ b/lib/ansible/modules/net_tools/basics/uri.py @@ -314,7 +314,7 @@ RETURN = r''' # The return information includes all the HTTP headers in lower-case. elapsed: description: The number of seconds that elapsed while performing the download - returned: always + returned: on success type: int sample: 23 msg: @@ -324,7 +324,7 @@ msg: sample: OK (unknown bytes) redirected: description: Whether the request was redirected - returned: always + returned: on success type: bool sample: false status: diff --git a/test/integration/targets/uri/tasks/main.yml b/test/integration/targets/uri/tasks/main.yml index 6210cc23bde..2473bcf7057 100644 --- a/test/integration/targets/uri/tasks/main.yml +++ b/test/integration/targets/uri/tasks/main.yml @@ -102,9 +102,12 @@ - name: Assert that the file was not downloaded assert: that: - - "result.failed == true" + - result.failed == true - "'Failed to validate the SSL certificate' in result.msg or ( result.msg is match('hostname .* doesn.t match .*'))" - - "stat_result.stat.exists == false" + - stat_result.stat.exists == false + - result.status is defined + - result.status == -1 + - result.url == 'https://' ~ badssl_host ~ '/' - name: Clean up any cruft from the results directory file: diff --git a/test/units/module_utils/urls/test_fetch_url.py b/test/units/module_utils/urls/test_fetch_url.py index 0af03e74fb4..93a9399c91c 100644 --- a/test/units/module_utils/urls/test_fetch_url.py +++ b/test/units/module_utils/urls/test_fetch_url.py @@ -157,6 +157,8 @@ def test_fetch_url_nossl(open_url_mock, fake_ansible_module, mocker): fetch_url(fake_ansible_module, 'http://ansible.com/') assert 'python-ssl' in excinfo.value.kwargs['msg'] + assert'http://ansible.com/' == excinfo.value.kwargs['url'] + assert excinfo.value.kwargs['status'] == -1 def test_fetch_url_connectionerror(open_url_mock, fake_ansible_module): @@ -165,12 +167,16 @@ def test_fetch_url_connectionerror(open_url_mock, fake_ansible_module): fetch_url(fake_ansible_module, 'http://ansible.com/') assert excinfo.value.kwargs['msg'] == 'TESTS' + assert'http://ansible.com/' == excinfo.value.kwargs['url'] + assert excinfo.value.kwargs['status'] == -1 open_url_mock.side_effect = ValueError('TESTS') with pytest.raises(FailJson) as excinfo: fetch_url(fake_ansible_module, 'http://ansible.com/') assert excinfo.value.kwargs['msg'] == 'TESTS' + assert'http://ansible.com/' == excinfo.value.kwargs['url'] + assert excinfo.value.kwargs['status'] == -1 def test_fetch_url_httperror(open_url_mock, fake_ansible_module):