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 bare hostname in proxy #1118

Closed
wants to merge 0 commits into from
Closed

Support bare hostname in proxy #1118

wants to merge 0 commits into from

Conversation

j178
Copy link
Member

@j178 j178 commented Aug 2, 2020

Closes #1082

  • Auto prepend http scheme for bare hostname in proxy
  • Fix two internal use of deprcating .getlist()

@tomchristie
Copy link
Member

Fantastic, loving this, thanks! 😃

One interesting question here...
I think we probably only want to support this case wrt. bare environment variables, rather than supporting it in Proxy itself. (?)

Eg. If get_environement_proxies was changed from...

for scheme in ("http", "https", "all"):
    if proxy_info.get(scheme):
        mounts[scheme] = proxy_info[scheme]

to instead be this...

for scheme in ("http", "https", "all"):
    if proxy_info.get(scheme):
        hostname = proxy_info[scheme]
        mounts[scheme] = hostname if "://" in hostname else f"http://{hostname}"

Then we wouldn't change the code in Proxy at all, and our programmatic usage would remain a bit neater and more strict.

client = httpx.Client(proxies="http://localhost:1234")  # Okay
client = httpx.Client(proxies="localhost:1234")         # Invalid, raises an error

While still handling the fuzzy ALL_PROXY=localhost:1234 case gracefully.

Thoughts?

httpx/_models.py Outdated Show resolved Hide resolved
httpx/_models.py Outdated Show resolved Hide resolved
@florimondmanca
Copy link
Member

florimondmanca commented Aug 2, 2020

Great stuff! Agreed with ^. :-) Also, if we go the "env vars only" path, we can probably update the Environment Variables: HTTP_PROXY, HTTPS_PROXY, ALL_PROXY docs as part of this PR? (Could be a follow-up task too, don't mind.)

@tomchristie
Copy link
Member

Great catch on the get_list/getlist - I've dropped those changes out of this PR.

We can treat them separately (Because I'd probably prefer that we pragma: nocover the to-be-deprecated lines, rather than include them in the test cases like that.)

@tomchristie
Copy link
Member

@florimondmanca Yes, good call. General note: We're also missing NO_PROXY there.

@florimondmanca
Copy link
Member

Yup. I'm working on a revamp of proxy docs anyway so I can include that stuff there as well.

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.

Should we support bare hostnames in ALL_PROXY,HTTP_PROXY, HTTPS_PROXY?
3 participants