Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing HTTPError case of fetch_url for Python 3 compatibility. #45628

Merged
merged 4 commits into from
Sep 19, 2018
Merged

Fixing HTTPError case of fetch_url for Python 3 compatibility. #45628

merged 4 commits into from
Sep 19, 2018

Conversation

felixfontein
Copy link
Contributor

SUMMARY

This is a follow-up to #33792 which modified fetch_url to store headers lower-case (as Python 2 is doing) also for Python 3. There was one case missing: in case a HTTPError is raised and its info() method is available, its headers have not been converted to lower-case.

CC @sivel

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/urls.py

ANSIBLE VERSION
2.8.0

@felixfontein felixfontein mentioned this pull request Sep 13, 2018
Merged
sivel
sivel requested changes Sep 13, 2018
View reviewed changes
Copy link
Member

@sivel sivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog fragment, and both unit and integration tests

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. python3 small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 13, 2018
@ansibot ansibot added the test This PR relates to tests. label Sep 13, 2018
@felixfontein
Copy link
Contributor Author

@sivel I cannot figure out how to get httpbin to return headers with an error status code. Without that, I don't see how to add a usable integration test. Do you have any idea for that?

@sivel
Copy link
Member

sivel commented Sep 13, 2018

@sivel I cannot figure out how to get httpbin to return headers with an error status code. Without that, I don't see how to add a usable integration test. Do you have any idea for that?

Any status code >=400 should exercise that code.

Making a request to http://httpbin.org/status/400 will return HTTP headers still. The only thing that I'll note here, is that it seems that the uri module will lowercase them too, so using uri as the integration test, wouldn't be a good test.

@felixfontein
Copy link
Contributor Author

Ok, but then I cannot pass something on my own :)

About integration tests: is there any way to call fetch_url as directly as possible (without going through some module which does it highly indirect)? If not, I don't see how I can add an integration test for this. (Except that maybe #45603 would be an "integration test" for this, since at least the acme_certificate tests should fail when the module tries to download the certificate, and cannot find the correct Content-Type header.)

@felixfontein
Copy link
Contributor Author

Restarting unrelated failing tests.

@felixfontein felixfontein reopened this Sep 13, 2018
@sivel
Copy link
Member

sivel commented Sep 13, 2018

We don't have decent support for doing http tests via direct python as integration tests.

We can just rely on this fixing the broken test for acme_certificate.

I see you have added units.

@felixfontein
Copy link
Contributor Author

Actually I will improve the acme_certificate code to not break at this exact position, but there will be another test later on which will definitely break if content-type isn't in the returned info (as a specific value is expected, and None doesn't count :) ).

@felixfontein
Copy link
Contributor Author

(I'll add that to rebased #45603 once this is merged.)

@felixfontein
Copy link
Contributor Author

@sivel There's still one unrelated test failing: https://app.shippable.com/github/ansible/ansible/runs/83900/34/tests Can you restart it?

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Sep 13, 2018
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Sep 17, 2018
@felixfontein
Copy link
Contributor Author

@sivel (or whoever did that) Thanks for restarting it! :)

How shall we proceed with this PR? Does this require more reviews, or can it be merged?

@sivel sivel merged commit bc69aec into ansible:devel Sep 19, 2018
@sivel
Copy link
Member

sivel commented Sep 19, 2018 •
edited

This has been merged to devel for the 2.8 release. Please create backport PRs for stable-2.6 and stable-2.7:

https://docs.ansible.com/ansible/latest/community/development_process.html#backport-pull-request-process

(Note: stable-2.5 does not have any of the lowercasing changes, so this should not be backported there)

@felixfontein felixfontein deleted the fix-fetch-url branch September 19, 2018 15:58
felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 19, 2018
@felixfontein
No longer necessary since ansible#33792 and ansible#45628.
a244f1b
felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 19, 2018
@felixfontein
Fixing HTTPError case of fetch_url for Python 3 compatibility. (ansib…
385e548 
…le#45628)

* Fixing HTTPError case of fetch_url for Python 3 compatibility.

* Adding unit test.

* PEP8.

* Changelog.
felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 19, 2018
@felixfontein
Fixing HTTPError case of fetch_url for Python 3 compatibility. (ansib…
f3c865d 
…le#45628)

* Fixing HTTPError case of fetch_url for Python 3 compatibility.

* Adding unit test.

* PEP8.

* Changelog.
This was referenced Sep 19, 2018
Merged
Merged
mattclay pushed a commit that referenced this pull request Sep 25, 2018
@felixfontein @mattclay
Fixing HTTPError case of fetch_url for Python 3 compatibility. (#45628)
1a14c50 
* Fixing HTTPError case of fetch_url for Python 3 compatibility.

* Adding unit test.

* PEP8.

* Changelog.
felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 28, 2018
@felixfontein @gundalow
No longer necessary since ansible#33792 and ansible#45628. (ansible#4…
e76596a 
…5603)
abadger pushed a commit that referenced this pull request Oct 9, 2018
@felixfontein @abadger
Fixing HTTPError case of fetch_url for Python 3 compatibility. (#45628)
ff1aa50 
* Fixing HTTPError case of fetch_url for Python 3 compatibility.

* Adding unit test.

* PEP8.

* Changelog.
@dagwieers dagwieers added the net_tools Net-tools category label Mar 3, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. net_tools Net-tools category python3 support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@felixfontein @sivel @dagwieers @samdoran @ansibot