-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Adds SSL proxy variables to default passenv #1439
Conversation
"LANG", | ||
"LANGUAGE", | ||
"LD_LIBRARY_PATH", | ||
"PATH", | ||
"PIP_INDEX_URL", | ||
"REQUESTS_CA_BUNDLE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think tox should know things about requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asottile If I remember well, even pip is using requests more recently. If we do not include it, it will likely fail to run even pre-commit -- in fact that was why drove me to add them, I was not able to tox -e lint
without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pip vendors a ca-cert bundle, which should be active without needing anything right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well not really behind a corporate proxy; as the ca-cert bundle obviously doesn't know about corporate proxy certificates. These usually are set as env-vars though.
docs/changelog/1437.bugfix.rst
Outdated
@@ -0,0 +1,2 @@ | |||
Adds CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, SSL_CERT_FILE to the default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer to see these escaped, as
``CURL_CA_BUNDLE``
Also no reason to split it across two lines.
@@ -19,7 +19,14 @@ description = run the tests with pytest under {basepython} | |||
setenv = PIP_DISABLE_VERSION_CHECK = 1 | |||
COVERAGE_FILE = {env:COVERAGE_FILE:{toxworkdir}/.coverage.{envname}} | |||
VIRTUALENV_NO_DOWNLOAD = 1 | |||
passenv = http_proxy https_proxy no_proxy SSL_CERT_FILE PYTEST_* | |||
passenv = | |||
CURL_CA_BUNDLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would you need it to set it there if it's done internally 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the tox version used to test tox itself is an older version, one that is not able to
pass these vars. I had to do this locally in order to be able to run tox-on-tox. If I would
install from source before it could have worked.
This shouls avoid SSL failures with HTTP requests from pip, requests or other libraries. Also sorts existing values in order to make it easier to update the list in the future. Fixes: tox-dev#1437
This shouls avoid SSL failures with HTTP requests from pip, requests
or other libraries.
Also sorts existing values in order to make it easier to update the
list in the future.
Fixes: #1437
Thanks for contributing a pull request!
If you are contributing for the first time or provide a trivial fix don't worry too
much about the checklist - we will help you get started.
Contribution checklist:
(also see CONTRIBUTING.rst for details)
in message body
<issue number>.<type>.rst
for example (588.bugfix.rst)<type>
is must be one ofbugfix
,feature
,deprecation
,breaking
,doc
,misc
<your username>
"superuser
."CONTRIBUTORS
(preserving alphabetical order)