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

Support including port part in trusted-host #6909

Merged
merged 2 commits into from Aug 25, 2019

Conversation

@frostming
Copy link
Contributor

commented Aug 23, 2019

Closes #6886
Closes #6710

This PR proposes official support for including port part in trusted-host option, for both HTTP and HTTPS indexes. So:

--trusted-host example.com trusts all these indexes:

  • http://example.com
  • http://example.com:8080
  • https://example.com
  • https://example.com:8080

While --trusted-host example.com:8080 only trusts the indexes with port 8080 exactly:

  • http://example.com:8080
  • https://example.com:8080
@cjerdonek
Copy link
Member

left a comment

Thanks! A couple comments. Also, the command-line help should be updated to reflect the change.

src/pip/_internal/download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Show resolved Hide resolved

@frostming frostming force-pushed the frostming:feature/trusted-host-port branch 6 times, most recently Aug 23, 2019

@cjerdonek
Copy link
Member

left a comment

Thanks for the updates. Looking a lot better! Some more comments.

src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved

@frostming frostming force-pushed the frostming:feature/trusted-host-port branch 3 times, most recently Aug 24, 2019

@cjerdonek
Copy link
Member

left a comment

Thanks for your changes. One more comment that I can see. Almost there!

src/pip/_internal/utils/misc.py Outdated Show resolved Hide resolved
@cjerdonek

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Oh, and there is still the comment from before re: updating the command-line help to say that host-port pair is also acceptable. That code is in cmdoptions.py.

@frostming frostming force-pushed the frostming:feature/trusted-host-port branch to 72cb4db Aug 24, 2019

@cjerdonek

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

I came across another thought: what if the index URL is HTTPS and contains auth part? ...

@frostming By the way, regarding the comment quoted above, would you mind filing that as a separate issue so we don't lose track of it? It would be much appreciated..

@cjerdonek

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

FYI, I made some minor formatting tweaks in preparation for merging.

@cjerdonek cjerdonek merged commit 8ac2214 into pypa:master Aug 25, 2019

22 checks passed

Linux Build #20190825.3 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190825.3 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x86) Test Primary Python27-x86 succeeded
Details
Windows (Test Primary Python37-x64) Test Primary Python37-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190825.3 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
@cjerdonek

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Thanks again, @frostming! 🎉

@frostming frostming deleted the frostming:feature/trusted-host-port branch Aug 27, 2019

@frostming

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@cjerdonek Thanks for that, I created #6931 to track the remaining issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.