-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
replace https with request to allow for proxy configuration #166
replace https with request to allow for proxy configuration #166
Conversation
Test failure on travis-ci was due to a timeout executing "gulp --verify". I assume, that it has nothing to do with the changes in my pull request, since it didn't fail on appveyor nor in any other nodejs versions. Would you still like me to improve something? |
We're not changing to request, sorry. It's a massive dependency that has vulnerabilities all the time. If you'd like to manually add proxy support in a new PR, we'll review. |
Hi Blaine
Ok, I'll give it a try. It will only support that kind of proxy that I
have to use at work, though, because I won't be able to test any other.
Does that sound ok?
Best regards,
Jörg
|
@chewiebug Could easily implement this behavior https://github.com/request/request#controlling-proxy-behaviour-using-environment-variables using https://github.com/TooTallNate/node-proxy-agent (http_proxy, https_proxy, and no_proxy env vars - dont think we need to fuss with the other ones) |
It looks like they broke that out as well - https://www.npmjs.com/package/tunnel-agent |
Just curious - why don't you configure your proxy across your whole OS? |
@contra Thank you very much for the hint concerning
https://github.com/TooTallNate/node-proxy-agent! I'll try that package.
Just curious - why don't you configure your proxy across your whole OS?
On my local machine, I can do that. On our ci server I can't configure
much. I need to be able to tell all tools to use the proxy.
|
I'd prefer tunnel-agent over node-proxy-agent (which adds a bunch of deps). I also think we should use add configuration options to the |
I see, that "tunnel-agent" is very small with just one dependency. But
other than that, I have troubles finding more advantages.
Looking at its github page, I have a doubts about this package:
- it feels abandoned (last commit over 5 Mar 2017), since then, no
reaction on any issues or pull requests
- to fix a vulnerability, the current github version would need to be
published to npmjs, no reaction
(request/tunnel-agent#41).
- I am not sure, if it will run on node 8
(request/tunnel-agent#23)
- CONNECT method does not seem to be used in the right way
(request/tunnel-agent#32).
- it is completely undocumented (seems to be a copy of
https://github.com/koichik/node-tunnel without reference to the original
project - license is changed)
I have tried to use "tunnel-agent" to proxy connection to the blacklist
file - I failed. This does not mean, that it is impossible, I am not
that good a Javascript developer. But still.
Using "node-proxy-agent" for connecting to gulpjs.com was easy.
Do you insist on using "tunnel-agent"? Then I'll need help using it. The
examples on https://github.com/koichik/node-tunnel didn't help. I keep
getting "tunneling socket could not be established, cause=connect
ECONNREFUSED". Am I maybe experiencing the issue documented in issue 32?
|
@chewiebug proxy-agent pulls together a ton of different modules but we're just using it for https - so you'd want to look at the https://github.com/TooTallNate/node-https-proxy-agent subdependency. |
@phated: I have added an implementation using https-proxy-agent. Is this
the right direction?
chewiebug@236948e
It is still using process.env.http(s)_proxy to configure the proxy. You
mentioned, that you would prefer to use configuration via .gulp.js
(https://github.com/gulpjs/gulp-cli#configuration). Does this mean, that
a new flag (e.g. flags.proxy) should be introduced and "opts" (or
opts.proxy) be passed to "getBlacklist()"?
Should configuration via .gulp.js and via cli flag be supported?
|
I see, that adding a "proxy" option to "cli-options.js" is quite easy:
chewiebug@cf79848
What do you think?
|
@chewiebug I think proxy configuration should be added not to cli option but To pass configurations specified in |
Thank you very much for he detailed explanation!
I suppose, I'll have to add the new flag to https://github.com/gulpjs/gulp-cli/blob/master/lib/shared/config/cli-flags.js, before I can use it, as you described.
I'll look into configuration via .gulp.js next week. I am on holidays for a few days, now.
|
@chewiebug Since |
@sttk thank you very much for the additional clarification! I'll give it a try when I am back.
|
I have pushed a branch using https-proxy-agent supporting .gulp.* for configuration of the proxy: master...chewiebug:feature/add-proxy-support-for-verify I see one big catch: https-proxy-agent:2.2.1 requires node >= 4. So this won't run on node 0.10 / 0.12 (a lot of unittests fail). I see the following options to address this:
What do you think? Do you see other options, that would be a better fit? |
I would like to use "gulp --verify" behind a proxy server. The current implementation does not seem to be able to support a proxy configuration.
Solution: Replace "https" by "request" in get-blacklist.js to allow for proxy configuration.