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

Don't rely on netloc for determining hostname and port, just use hostname and port #56270

Merged
merged 5 commits into from May 20, 2019

Conversation

Projects
None yet
4 participants
@sivel
Copy link
Member

commented May 9, 2019

SUMMARY

Don't rely on netloc for determining hostname and port, just use hostname and port. Fixes #56258

This resolves a bug when handling redirects with IPv6 addresses in the URL.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/urls.py

ADDITIONAL INFORMATION

@samdoran
Copy link
Member

left a comment

Tested and this change is good. Hooray for deleting code.

Please add unit tests to test/units/module_utils/urls/test_urls.py:78

    url = 'https://[2a00:16d8:0:7::205]:4443/'
    handler = urls.maybe_add_ssl_handler(url, True)
    assert handler.hostname == '2a00:16d8:0:7::205'
    assert handler.port == 4443

    url = 'https://[2a00:16d8:0:7::205]/'
    handler = urls.maybe_add_ssl_handler(url, True)
    assert handler.hostname == '2a00:16d8:0:7::205'
    assert handler.port == 443

Maybe add integration tests as well.

@ansibot ansibot removed the stale_ci label May 20, 2019

sivel added some commits May 20, 2019

@sivel

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

I've added unit tests, and resolved some Python2.6 issues that were causing tracebacks.

Integration tests aren't possible at this time, since the httptester container and intra-contatiner communications isn't IPv6 enabled.

@sivel sivel requested a review from samdoran May 20, 2019

@samdoran samdoran merged commit 493cf81 into ansible:devel May 20, 2019

1 check passed

Shippable Run 123637 status is SUCCESS.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.