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

Fix parsing of verify_ssl=false in urls with authentication in Pipfile #2656

Merged
merged 3 commits into from
Aug 1, 2018

Conversation

Enzime
Copy link
Contributor

@Enzime Enzime commented Jul 27, 2018

pipenv breaks when the first specified source in the Pipfile contains a username and password.

@Enzime
Copy link
Contributor Author

Enzime commented Jul 29, 2018

Can someone post what the buildkite says is failing?

@techalchemy
Copy link
Member

Sorry I’m out of town due to a family emergency and only me and Kenneth have accesss. If you remind me in a day I can check for you or you can run the build on a Linux vm if you didn’t already to find out. I can tell you that it builds python 2.7 first and that’s likely what failed

@Enzime
Copy link
Contributor Author

Enzime commented Jul 30, 2018

@techalchemy are you able to post the error output now?

@uranusjr
Copy link
Member

Not from Buildkite, but these are the errors I got

================================================ ERRORS =================================================
_______________________________ ERROR collecting tests/unit/test_utils.py _______________________________
tests/unit/test_utils.py:129: in <module>
    class TestUtils:
tests/unit/test_utils.py:378: in TestUtils
    verify_ssl: False,
E   NameError: name 'verify_ssl' is not defined
_______________________________ ERROR collecting tests/unit/test_utils.py _______________________________
tests/unit/test_utils.py:129: in <module>
    class TestUtils:
tests/unit/test_utils.py:378: in TestUtils
    verify_ssl: False,
E   NameError: name 'verify_ssl' is not defined
_______________________________ ERROR collecting tests/unit/test_utils.py _______________________________
tests/unit/test_utils.py:129: in <module>
    class TestUtils:
tests/unit/test_utils.py:378: in TestUtils
    verify_ssl: False,
E   NameError: name 'verify_ssl' is not defined
_______________________________ ERROR collecting tests/unit/test_utils.py _______________________________
tests/unit/test_utils.py:129: in <module>
    class TestUtils:
tests/unit/test_utils.py:378: in TestUtils
    verify_ssl: False,
E   NameError: name 'verify_ssl' is not defined

Looks like you missed some quotes in your test fixture.

@Enzime Enzime force-pushed the fix/basic-auth-in-repo-URLs branch from 1cb191c to 7f72b94 Compare July 31, 2018 08:07
@Enzime
Copy link
Contributor Author

Enzime commented Jul 31, 2018

@uranusjr Thanks, fixed.

@Enzime Enzime force-pushed the fix/basic-auth-in-repo-URLs branch from 7f72b94 to f78637f Compare July 31, 2018 08:10
@techalchemy
Copy link
Member

Thanks for the contribution! I don't really have any concerns here, looks pretty good to me

@@ -0,0 +1 @@
Fixed a bug which sometimes caused pipenv to parse the ``trusted_host`` argument to pip incorrectly when parsing source URLs which specify ``verify_ssl = false``.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bug wasn't a result of verify_ssl being false it was because the source contained a username and password.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to rewrite the fragment if you feel it is incorrect!

Copy link
Member

Choose a reason for hiding this comment

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

I mean, while the bug may not be caused by verify_ssl being false (I didn't say that it was caused by setting verify_ssl = false, only that it sometimes occurred when parsing URLs which specified this option), it can certainly only apply to URLs that specify verify_ssl = false as I mention in the news fragment, wouldn't you say?

Fixed a bug which sometimes caused pipenv to parse the trusted_host argument to pip incorrectly

This is the nature of the bug, no?

parsing source URLs which specify verify_ssl = false.

This is a condition under which the bug occurs, right? I didn't want to get super detailed, as it's most likely an edge case that most users won't encounter -- there is a bug parsing the trusted host argument sometimes if you specify the argument via your pipfile. There are other ways to specify trusted hosts, but this is the only one that is handled by pipenv directly, so it seems relevant to caveat this by highlighting the specific context without delving into the low level, 'if you first type x, then y, then z, then put a thing here, etc'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I got totally confused about the nature of the bug, after taking a look at the code again and remembering what the bug is. I totally agree with what you wrote.

@techalchemy techalchemy changed the title Properly handle the first source having user/pass Fix parsing of verify_ssl=false in urls with authentication in Pipfile Aug 1, 2018
@uranusjr uranusjr merged commit 641e889 into pypa:master Aug 1, 2018
@Enzime Enzime deleted the fix/basic-auth-in-repo-URLs branch August 4, 2018 03:02
@Enzime Enzime restored the fix/basic-auth-in-repo-URLs branch September 21, 2018 01:20
@Enzime Enzime deleted the fix/basic-auth-in-repo-URLs branch September 21, 2018 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants