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.

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

[WIP] Append header duplicate headers when http error #54722

Closed
wants to merge 5 commits into from

Conversation

@Lunik
Copy link
Contributor

@Lunik Lunik commented Apr 2, 2019

SUMMARY

When http return error, headers are not concatenated when receiv multiple with the same name

Fixe #54721

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils/urls

ADDITIONAL INFORMATION

# Don't be lossy, append header values for duplicate headers
# In Py2 there is nothing that needs done, py2 does this for us
@Lunik
Copy link
Contributor Author

@Lunik Lunik commented Apr 3, 2019

ready_for_review

Loading

@sivel sivel self-assigned this Apr 3, 2019
@sivel sivel removed the needs_triage label Apr 3, 2019
Copy link
Member

@sivel sivel left a comment

This should also be accompanied by a changelog fragment in changelog/fragments

Loading

lib/ansible/module_utils/urls.py Outdated Show resolved Hide resolved
Loading
@@ -1351,21 +1368,8 @@ def fetch_url(module, url, data=None, headers=None, method=None,
follow_redirects=follow_redirects, client_cert=client_cert,
client_key=client_key, cookies=cookies, use_gssapi=use_gssapi,
unix_socket=unix_socket)
# Lowercase keys, to conform to py2 behavior, so that py3 and py2 are predictable
info.update(dict((k.lower(), v) for k, v in r.info().items()))
Copy link
Member

@sivel sivel Apr 10, 2019

Choose a reason for hiding this comment

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

I believe we still need this info.update here. There are cases where r.info() is more than just headers iirc.

And I had completely forgotten that my normalize_headers didn't do anything with lowercasing. So we'll need to add lowercasing back in. Although I'd recommend not doing the lowercasing in normalize_headers. I'd still do that on info, since it is a specific implementation detail of fetch_url.

Loading

Copy link
Contributor Author

@Lunik Lunik Apr 10, 2019

Choose a reason for hiding this comment

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

We can keep lowercase in normalize_headers function and add an extra argument to the function to activate lowercasing (false by default) ?

Loading

@ansibot ansibot removed the stale_ci label Apr 10, 2019
@Lunik Lunik changed the title Append header duplicate headers when http error [WIP] Append header duplicate headers when http error Apr 10, 2019
@ansibot
Copy link
Contributor

@ansibot ansibot commented May 29, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/urls.py:921:41: undefined-variable Undefined variable 'io'

click here for bot help

Loading

@ansibot ansibot added pre_azp and removed stale_ci labels Dec 5, 2020
@Akasurde
Copy link
Member

@Akasurde Akasurde commented May 8, 2021

@Lunik Are you still working on this? Thanks.

needs_info

Loading

@ansibot
Copy link
Contributor

@ansibot ansibot commented Jun 9, 2021

@Lunik This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

Loading

@ansibot
Copy link
Contributor

@ansibot ansibot commented Jul 11, 2021

@Lunik You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.

click here for bot help

Loading

@ansibot ansibot closed this Jul 11, 2021
@ansible ansible locked and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants