Add support for reading http.proxy setting #639
Conversation
|
Hi @ibigbug, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
Oh ci complains. |
| @@ -115,9 +115,11 @@ function installTools(goVersion: SemVersion, missing?: string[]) { | |||
|
|
|||
| outputChannel.appendLine(''); // Blank line for spacing. | |||
|
|
|||
| let http_proxy = vscode.workspace.getConfiguration('http').get('proxy'); | |||
| let https_proxy = http_proxy; // default proxy for http & https | |||
There was a problem hiding this comment.
@ibigbug Sorry for the very late review.
Have you tested the case where http_proxy is not set in settings , but is set in the environment variable?
In that case will the value set in the environment variable make its way to the child process running the go get command?
There was a problem hiding this comment.
@ramya-rao-a Yeah. I'm sure http_proxy in environment variables and open VS code from this environment using $ code can make go get work through proxy, though it's limited use case. User may often not open VS code through terminal and for OS X users (like me) and other Linux users, environment variables for terminal and GUI are always isolated, this is the main consideration for this PR.
There was a problem hiding this comment.
@ibigbug I understand the use case you are trying to solve. My question was for the use case where a user might have set the env var for http_proxy, but not in settings. What happens then, with your code changes?
I tested your code in 2 scenarios in Windows, this is what I see for the env that gets passed to the child process running the go get
-
When
http_proxyis set in env var but not in VS Code settings
-
When different values are set for
http_proxyin env var and VS Code settings
You will have to do 2 changes I believe
- Use upper case while setting the http_proxy variable
- If there is no VS Code setting for http_proxy, then do not change the process.env
1. set both lower and upper case proxy environment variable 2. do not touch process.env when http.proxy setting is not present
| let env = process.env; | ||
| if (httpProxy) { | ||
| env = Object.assign({}, process.env, { | ||
| http_proxy: httpProxy, |
There was a problem hiding this comment.
@ramya-rao-a Thanks for pointing out the problems which I did't test carefully.
- I believe golang follows both lower and upper case proxy env vars: https://github.com/golang/go/blob/master/src/net/http/transport.go#L246 , do you think that it's OK to set both lower and upper case env vars here?
- I've added an detection if http.proxy was present.
There was a problem hiding this comment.
any benefits of setting both upper and lower case?
Setting just the upper case should do so that if any that was set outside vs code get overridden.
There was a problem hiding this comment.
Yes I believe there are.
In case go get are calling some external tools such as wget to download third party resources or other tools like wget which read lowercase setting from environment variables: https://www.gnu.org/software/wget/manual/wget.html#Proxies
So I think setting both can be safer.
There was a problem hiding this comment.
Sounds good. Only thing pending is the failing tests in travis. Can you debug and see if it fails in your local set up as well?
There was a problem hiding this comment.
I found the failed test case is Test diffUtils.getEdits however I have no idea how does this commit make a difference on that. I'll have a local debug to see what happened.
There was a problem hiding this comment.
@ramya-rao-a It's interesting that running 'npm t` on current master branch also gets 6 failed tests. I wonder if there are any dependencies have had a breaking change?
There was a problem hiding this comment.
I have fixed the tests in master. Re-running the travis tests for this PR keeps using the version of master before my fix.
So will go ahead and merge this without waiting for tests to turn green for this PR.
I was out on winter holidays, so couldn't help wrap this up sooner.
Thanks for your PR and I look forward to more in the future
* 'master' of github.com:mattetti/vscode-go: (128 commits) Add telemetry support for the Go extension Add dlv path to error msg when dlv not found Fixes microsoft#465 outline offset problem in No-English language (microsoft#699) fix coverage display when editor changes to undefined (microsoft#693) Add support for reading http.proxy setting (microsoft#639) Fixing flaky unit tests Refactor for corner cases Reduce noise in console log due to errors from hover Changelog for next update 0.6.51 Remove space after brackets in snippet as per microsoft#628 Fixes microsoft#647 Strip version from autocompleted pkgs from gopkg.in (microsoft#659) Fixes microsoft#647 Support vendor pkgs from root of GOPATH/src (microsoft#660) Fixes microsoft#640 Use spawn to handle big data from gocode (microsoft#661) Fixed wrong workspace when one gopath is the substring of another. (microsoft#658) Fix "gotests" when generates test for current function for type methods (microsoft#657) Add benchmark function snippet (microsoft#648) Fix "Go to Definition" when running Go HEAD (microsoft#655) Fixes microsoft#642 Only add -d once to formatflags (microsoft#644) Updating Changelog and package.json for 0.6.50 update Log errors to console ...
Make
go gethonor http.proxy setting so that user can not only provide their proxy settings via environment variables