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

Add --no-proxy option #12011

Closed
wants to merge 0 commits into from
Closed

Add --no-proxy option #12011

wants to merge 0 commits into from

Conversation

martinezlc99
Copy link

@martinezlc99 martinezlc99 commented May 3, 2023

Fix #5378

Add --no-proxy option which sets the PipSession(requests.Session).trust_env to False and also sets PipSession(requests.Session).proxies to {"http": None, "https": None}.

Setting trust_env to False on Session will force requests to not pull proxy information from enviromental variables or system settings, which is the desired behavior for this feature.

@martinezlc99

This comment was marked as outdated.

@martinezlc99 martinezlc99 changed the title Add --no-proxy option Add --no-proxy option May 5, 2023
@martinezlc99 martinezlc99 changed the title Add --no-proxy option Add --no-proxy option May 5, 2023
@uranusjr
Copy link
Member

Still has static check issues.

@martinezlc99
Copy link
Author

pre-commit.ci autofix

@martinezlc99
Copy link
Author

@uranusjr I believe I fixed the staic analysis warnings. I still cant get tests on Windows to execute...like you said..."lets go RAMDisk"

@uranusjr
Copy link
Member

GitHub has been having issues with those Windows jobs recently. I’ll knock on the machine to see if that fixes things.

@notatallshaw
Copy link
Member

What happens when you provide a --proxy options with --no-proxy?

I ask because if you accept the command line passed proxy when one is passed it can close issues like this #9105

Also I think the name --no-proxy is misleading, it's also turning off getting ca bundles from env variables and auth from .netrc file and you're not reproducing the requests logic of getting those values.

I think it would make a lot more sense to call it --no-env-network-settings (probably too long but that sort of thing) and make sure users can still manually pass in a --proxy. My 2 cents anyway.

@martinezlc99
Copy link
Author

What happens when you provide a --proxy options with --no-proxy?

PipSession(requests.Session).proxies is set to {"http": None, "https": None}, so any proxies passed via --proxy are ignored. I had considered adding a warning if this was the case, but figured this scenario to be uncommon.

I ask because if you accept the command line passed proxy when one is passed it can close issues like this #9105

I can keep the using the proxies passed via --proxy, but agree with your below comment that the option should probably be re-worded in this case. I wonder if #9105 can be addressed via --proxy=<proxy> --isolated, although seems unlikely since --isolated only ignores PIP_* env variables?

Also I think the name --no-proxy is misleading, it's also turning off getting ca bundles from env variables and auth from .netrc file and you're not reproducing the requests logic of getting those values.

I think it would make a lot more sense to call it --no-env-network-settings (probably too long but that sort of thing) and make sure users can still manually pass in a --proxy. My 2 cents anyway.

Great point. I can update request logic to:

  • set ca bundles back to the env variables via PipSession(requests.Session).verify
  • and auth from .netrc file via PipSession(requests.Session).auth

or we can rename the option as you suggest and keep the current behavior; however, this would appear to be somewhat duplicative of the --isolated option although it would not work in the specific use case of ignoring proxies set via HTTP_PROXY and HTTPS_PROXY. Maybe renaming option to --isolated-network makes sense if we keep current behavior?

For now I'll see about getting ca bundles back from env cariables and auth from .netrc file, but open to whatever obviously.

@EFord36
Copy link

EFord36 commented Nov 29, 2023

Hi @martinezlc99 - have you been able to look at this recently?

This feature would be very useful to me too, so if it's something you don't have time to pick up at the moment, it's something that I may be able to help with if you would be amenable to that.

@martinezlc99
Copy link
Author

Hi @martinezlc99 - have you been able to look at this recently?

This feature would be very useful to me too, so if it's something you don't have time to pick up at the moment, it's something that I may be able to help with if you would be amenable to that.

Hi @EFord36 - wow, sorry just seeing this. I think I am done with this PR, just need to answer questions from @notatallshaw.

I have the holidays off, so will attempt to get his done in the next few days. I'll let you know if I get blocked for any reason.

@notatallshaw
Copy link
Member

notatallshaw commented Apr 25, 2024

FYI MacOS runners are failing due to known issue, though you do need to fix your spelling of environmental.

@notatallshaw
Copy link
Member

MacOS runners are now working, if you rebase or merge main and fix spelling error failing pre-commit this should pass CI.

@knechtionscoding
Copy link

@martinezlc99 any movement?

@martinezlc99
Copy link
Author

@knechtionscoding - yes, I was distracted after initial PR, and unfortunately never returned. In rethinking how to respond to #12011 (comment) , I think I agree --no-proxy may not be the best name for the commandline flag.

I will rethink this and submit a new PR ASAP.

@notatallshaw
Copy link
Member

I think I agree --no-proxy may not be the best name for the commandline flag.

Upon reflecting on my name comment, I think it's more important to just document what it's doing, rather than bike shedding the name too much.

@martinezlc99
Copy link
Author

PR moved to #13051

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to bypass http proxy
6 participants