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

feat(request): add noProxy setting for configuration (#5048) #5757

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

glausmichael
Copy link

Summary

No

Proxy exclusion is currently not possible in configuration file. If proxy is defined in configuration file, no proxy exclusion can therefore be specified at the moment, because environment variable is not taken into account in this case.

Test plan

Added test 'RequestManager.request with no_proxy option' to check both http and https behavior with proxy exclusion.
Tested with corporate proxy by calling yarn install with local repository.
I had to adapt the test 'RequestManager.execute timeout error with maxRetryAttempts=1' because the string port is not a valid port for a url.

@buildsize
Copy link

buildsize bot commented Apr 30, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 910.3 KB 909.61 KB -702 bytes (0%)
yarn-[version].js 3.95 MB 3.94 MB -2.66 KB (0%)
yarn-legacy-[version].js 4.1 MB 4.1 MB -2.66 KB (0%)
yarn-v[version].tar.gz 915.25 KB 914.59 KB -679 bytes (0%)
yarn_[version]all.deb 676.1 KB 675.61 KB -502 bytes (0%)

@buildsize
Copy link

buildsize bot commented Apr 30, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 910.3 KB 909.59 KB -727 bytes (0%)
yarn-[version].js 3.95 MB 3.94 MB -3.05 KB (0%)
yarn-legacy-[version].js 4.1 MB 4.1 MB -3.06 KB (0%)
yarn-v[version].tar.gz 915.25 KB 914.94 KB -325 bytes (0%)
yarn_[version]all.deb 676.1 KB 675.74 KB -366 bytes (0%)

@arcanis
Copy link
Member

arcanis commented May 2, 2018

I got this problem a few days ago, so I sympathize 😄

A workaround is to set YARN_PROXY= YARN_HTTPS_PROXY= in the environment. It might also work with the yarnrc with something like this (haven't actually tested this one):

env:
  YARN_PROXY ""
  YARN_HTTPS_PROXY ""

As for your PR, I think ideally the best would be to support --no-proxy and --no-https-proxy from the commandline, but I think Commander doesn't support it well enough :/

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

I think in its current state this diff is doing too many things (we should try to add it as a command line switch), but I'm open to discuss it to solve this elegantly.

@glausmichael
Copy link
Author

glausmichael commented May 8, 2018

To clarify, this PR should support adding a configuration like no-proxy="127.0.0.1,mydomain.com" in .yarnrc file as it is requested by #5048 as I understood.

My change just ensures that this "no-proxy" setting is loaded from configuration file and checks if the target url is covered by this no-proxy setting. The new file proxy-exclusion.js is mainly the same implementation as the implementation in the underlying library is using to exclude a url from proxy. (https://github.com/request/request/blob/master/lib/getProxyFromURI.js)

Because request library just has a "proxy" config option but no "no-proxy" option, I see the only way to suppress proxy by passing an empty string to the request library. (as it was already done before if proxy was set to false in yarn)

This code is needed also in case of additional command line options I think.

@dicko2
Copy link

dicko2 commented Jun 18, 2018

Any progress on this?

@paztis
Copy link

paztis commented Dec 11, 2018

what is the current status ?
On my side I wait for it since a long time.
Due to a corporate nexus registry we have to define the proxy / https-proxy to null to let it be accessible, but all the postinstall fetchs always fail (node-sass, puppeteer, ...)

We don't like to ply with env var as we're on ayarn workspace where a lot of sub commands will try to access to the nexus.

@paztis
Copy link

paztis commented Mar 12, 2019

Any news ?
If this is rejected perhaps you can change the status ?

@cobolbaby
Copy link

cobolbaby commented Nov 23, 2019

Any progress ? @glausmichael

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.

None yet

5 participants