jaemk / self_update

Self updates for rust executables
MIT License
798 stars 70 forks source link

feat: support http(s) proxy #96

Closed Itsusinn closed 2 years ago

Itsusinn commented 2 years ago

Respect http(s)_proxy and all_proxy environment variables

Itsusinn commented 2 years ago

Or..... should we let this behaviour configurable?

jaemk commented 2 years ago

Thanks for the contribution! I think it's good as is, but it wouldn't hurt to add an option on the Download struct to configure whether the proxy vars should be ignored. It could default to false so the proxy is used by default, which I think people would expect to be the default.

Itsusinn commented 2 years ago

Thanks for the contribution! I think it's good as is, but it wouldn't hurt to add an option on the Download struct to configure whether the proxy vars should be ignored. It could default to false so the proxy is used by default, which I think people would expect to be the default.

Here comes another question.How can we allow users to modify this option? Should I add a get_ignore_env_proxy here? https://github.com/jaemk/self_update/blob/e7200ba179bb7084cac976c1aed7be606861fcf8/src/update.rs#L72-L113 https://github.com/jaemk/self_update/blob/e7200ba179bb7084cac976c1aed7be606861fcf8/src/backends/github.rs#L440

jaemk commented 2 years ago

Hm, yeah it's going to be a much bigger change to respect the proxy env vars everywhere. The trait doesn't need to be modified, but every implementation github/gitlab/s3 will need to have their builders updated with a similar option, and every http request needs to be converted to a client + proxy + request like you did here. I'd be okay with merging and releasing what you've already got here if you don't have the time to update everything right now

Itsusinn commented 2 years ago

I discovered the embarrassing fact that reqwest itself supports reading system proxies

https://github.com/seanmonstar/reqwest/blob/f11e9584339e3bc298949a3eb4f142c5d4949312/src/async_impl/client.rs#L94

https://github.com/seanmonstar/reqwest/blob/f11e9584339e3bc298949a3eb4f142c5d4949312/src/async_impl/client.rs#L161

https://github.com/seanmonstar/reqwest/blob/f11e9584339e3bc298949a3eb4f142c5d4949312/src/async_impl/client.rs#L213-L217

Itsusinn commented 2 years ago

It looks like reading proxies from environment variables has become the default behavior of reqwest since last year

Itsusinn commented 2 years ago

How about we offer three options? Use no proxy, or use the system proxy, or use a custom proxy. Otherwise the pull request will become meaningless