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

include the HTTP proxy environment variables in PASSENV by default #1498

Closed
pfmoore opened this issue Jan 15, 2020 · 3 comments · Fixed by #1500
Closed

include the HTTP proxy environment variables in PASSENV by default #1498

pfmoore opened this issue Jan 15, 2020 · 3 comments · Fixed by #1500
Labels
feature:new something does not exist yet, but should pr-merged

Comments

@pfmoore
Copy link

pfmoore commented Jan 15, 2020

Tox doesn't include HTTP proxy variables (HTTP_PROXY, HTTPS_PROXY and NO_PROXY) in PASSENV by default. As those variables basically say "you need to use this proxy or network access won't work" they seem like something that would be required for any network-related test.

I suggest that the variables are added to the list of ones included in PASSENV by default.

#258 was raised about this issue, before PASSENV existed as far as I can see, and appears to have been accepted with the resolution being that tox passed all environment variables. Presumably at some point, that was restricted again and PASSENV introduced, but the proxy variables weren't considered at that time?

@pfmoore pfmoore added the feature:new something does not exist yet, but should label Jan 15, 2020
@gaborbernat
Copy link
Member

the main reason we haven't don't so yet is that you only need this if you've tests that access the network, this shouldn't be that many, not?

@pfmoore
Copy link
Author

pfmoore commented Jan 15, 2020

In general, I guess so. I hit this on pip, where the lint environment uses pre-commit and pre-commit runs git to install checks. So not a common case, certainly ;-) Conversely, though, not passing the proxy vars just leaves the test environment with broken networking, which seems pointless at best. I'm happy to say it's not a high priority, but I do think it's worth doing at some point.

@gaborbernat
Copy link
Member

I've been setting this in all my projects, so could be the default 🤷‍♂ if you have time for a PR send it on 👍

@gaborbernat gaborbernat changed the title Tox should include the HTTP proxy environment variables in PASSENV by default include the HTTP proxy environment variables in PASSENV by default Jan 16, 2020
@helpr helpr bot added the pr-available label Jan 16, 2020
@helpr helpr bot added pr-merged and removed pr-available labels Jan 22, 2020
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature:new something does not exist yet, but should pr-merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants