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

cli: use proxy settings when downloading plugins #7747

Merged
merged 2 commits into from
May 5, 2020

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented May 5, 2020

request/requestretry used to support some set of proxy environment
variables, while node-fetch doesn't by default.

This commit add support for proxy settings when downloading plugins.

Fixes #7739

How to test

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added enhancement issues that are enhancements to current functionality - nice to haves theia-cli issues related to the theia-cli labels May 5, 2020
@paul-marechal
Copy link
Member Author

@slhultgren can you tell me if this fixes your issue? It should support all proxy_http(s) and no_proxy environment variables, but I just don't know how to test. I had to come up with these changes because the proxy issue was brought to us, but I'd like help to test :)

@paul-marechal paul-marechal force-pushed the mp/proxy-download branch 2 times, most recently from bb36391 to 5bd5c11 Compare May 5, 2020 14:16
@slhultgren
Copy link
Contributor

@marechal-p Thanks for including the lastError change as well. I tested it and it works well for me. It properly respects the url and uses HTTP_PROXY for http urls and HTTPS_PROXY for https urls. Also ALL_PROXY works as well.
I'll abandon my PR since yours is better :)

@paul-marechal
Copy link
Member Author

@vince-fugnitto @marcdumais-work just checked the new deps with ClearlyDefined, everything looks good to me.

@paul-marechal paul-marechal force-pushed the mp/proxy-download branch 2 times, most recently from a7c9135 to 8e7ac88 Compare May 5, 2020 17:24
lastError is always set to undefined at the end of each for-loop
iteration, this is not intended behavior. The lastError should persist
when exiting the loop -if set-.

This commit also simplifies stream piping and error handling.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
request/requestretry used to support some set of proxy environment
variables, while node-fetch doesn't by default.

This commit add support for proxy settings when downloading plugins.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified that the previous behavior (without proxy) still works correctly 👍
For the proxy use-case it's been confirmed to work offline as well.

@paul-marechal paul-marechal merged commit 5dfd118 into master May 5, 2020
@paul-marechal paul-marechal deleted the mp/proxy-download branch May 5, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves theia-cli issues related to the theia-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: download:plugins does not support HTTP_PROXY environment variable
3 participants