Add support for HTTP proxy in network commands #10401
Conversation
|
@fnuttens i'm a total noob on the networking matter, is it possible to test such a new feature? 😋 |
|
@amtoine After that you can use any of the http subcommands to make a request through the proxy: |
| .tls_connector(std::sync::Arc::new(tls)); | ||
|
|
||
| if let Some(http_proxy) = retrieve_http_proxy_from_env() { | ||
| if let Ok(proxy) = ureq::Proxy::new(http_proxy) { |
There was a problem hiding this comment.
I'm not particularly convinced of the if let approach here, as the user won't be notified if ureq is unable to parse the proxy. This could give them the sense that they're making HTTP calls through a proxy when they're not.
On the other hand, this error seems quite unlikely to happen; I couldn't find a value that wasn't Ok for ureq to parse, even white space.
Let me know what you think: is it best to silently discard the proxy if unable to parse the HTTP_PROXY value, or should we expect a successful parsing?
|
|
||
| fn retrieve_http_proxy_from_env() -> Option<String> { | ||
| std::env::vars() | ||
| .find(|(key, _)| key == "HTTP_PROXY") |
There was a problem hiding this comment.
Hi, thanks for your pr, I think it's good to check ALL_PROXY too.
In the case if HTTP_PROXY exists, use it, or else check ALL_PROXY
There was a problem hiding this comment.
What about HTTPS_PROXY?
After doing some research on the matter, it seems like tools like curl support HTTP proxy in the lowercase variant (http_proxy). Should we look for both variables?
Regarding ALL_PROXY, don't we risk an error if using say a FTP proxy to make HTTP calls?
There was a problem hiding this comment.
@WindSoilder I put HTTP_PROXY and ALL_PROXY as fallbacks to http_proxy.
There was a problem hiding this comment.
Sorry for late reply.
I think your changes is fine. Regards to your question:
Regarding ALL_PROXY, don't we risk an error if using say a FTP proxy to make HTTP calls?
Yeah it is, so that's why we have http_proxy.
And I don't think we need to adapt HTTPS_PROXY for now
fnuttens
force-pushed
the
http_proxy
branch
from
e5eb27f
to
7ecdfb5
Compare
Closes nushell#8847 # Description If the `HTTP_PROXY` variable is found, use its value to setup ureq proxy. I haven't implemented `NO_PROXY` at the moment. # User-Facing Changes No breaking change for the user, the network commands simply use an environment variable. # Tests + Formatting The existing tests seem to run fine, although I can't think of a new test to add.
Closes #8847
Description
If the
HTTP_PROXYvariable is found, use its value to setup ureq proxy. I haven't implementedNO_PROXYat the moment.User-Facing Changes
No breaking change for the user, the network commands simply use an environment variable.
Tests + Formatting
The existing tests seem to run fine, although I can't think of a new test to add.