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

Rely on proxy-agent for detecting environment variables #765

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Oct 12, 2021

If no URI is provided, then proxy-from-env is used to get the URI from $HTTP_PROXY, $HTTPS_PROXY and $NO_PROXY among others. (Excerpt from proxy-agent documentation)

This pull request will fix #760 by honoring the no_proxy environment variable and add compatibility for proxy types other than https as listed on this table.

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 12, 2021 22:26 Inactive
@0x2b3bfa0 0x2b3bfa0 marked this pull request as ready for review October 12, 2021 22:32
@0x2b3bfa0 0x2b3bfa0 self-assigned this Oct 12, 2021
@0x2b3bfa0 0x2b3bfa0 added bug Something isn't working p1-important High priority labels Oct 12, 2021
@0x2b3bfa0
Copy link
Member Author

Note for testers (hello, testers): you can use mitmproxy to verify whether the no_proxy environment variable is being honored or not.

Spoiler: it works as expected.

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for testers (hello, testers): you can use mitmproxy to verify whether the no_proxy environment variable is being honored or not.

Can we include a test to nt have to test it manually?

@0x2b3bfa0

This comment has been minimized.

@DavidGOrtega
Copy link
Contributor

Note for testers (hello, testers): you can use mitmproxy to verify whether the no_proxy environment variable is being honored or not.

Can we include a test to nt have to test it manually?

@0x2b3bfa0
Copy link
Member Author

Yes, we can, of course. If the official NO_PROXY tests aren't enough for you, we can replicate them, but there is probably no point in testing the functionality of all our dependencies. Speak about deffensive programming. 😄

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Oct 13, 2021

We have a simple integration test launching a proxy and passing through it already, it does not covers every proxy type on how the SCM responds to them, but at least is one. I was thinking that maybe we could avoid manual test.
I will trust your own test and the code changes.

Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed an integration test but I guess we can assume it works
👍

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Oct 13, 2021

Rest assured that it works, and that funcationality is already being tested by the official library.

If you want, I can add a test on our side, though. 🤷🏼‍♂️ We can add an invalid proxy, set NO_PROXY=example.com or similar, and make a request to example.com and other to example.org for... example.

Still, it would be slightly pointless; we already use http_proxy for all the test suite, and testing all the proxy features doesn't seem especially constructive.

process.env.http_proxy = `http://localhost:${PORT}`;

@DavidGOrtega DavidGOrtega merged commit 54c0a18 into master Oct 15, 2021
@DavidGOrtega DavidGOrtega deleted the no-proxy branch October 15, 2021 09:44
@DavidGOrtega DavidGOrtega mentioned this pull request Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1-important High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variable "no_proxy" seems to be unsupported
2 participants