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.

Sign up for GitHub

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

Jump to bottom

Try to make reqwest use system proxies. #547

Merged
merged 26 commits into from
Jul 1, 2019
Merged

Try to make reqwest use system proxies. #547

merged 26 commits into from
Jul 1, 2019

Conversation

WindSoilder
Copy link
Contributor

@WindSoilder WindSoilder commented Jun 17, 2019 •
edited
Loading

Try to resolve issue #403
The core function for this feature is get_proxies(), which is a copy from python getproxies implementation.

Some consideration about the design:

After looking the getproxies implementation in python, I don't whink we need to use something like [env_proxy] crate, because it doesn't support windows system. So I implement it once again.. And this version of getproxies should work on windows and linux.

Something about testing

In windows system, because the proxy setting is configured from registry. I just do manually testing for this scenario. (I have implemented it in another place first)
I'm afraid the testing code may break windows registry setting. It can be a very big problem. So I just leave it away :(

@WindSoilder
Copy link
Contributor Author

WindSoilder commented Jun 17, 2019 •
edited
Loading

Oh, no...The tests for get_proxies is not stable...
It seems that there are some problems in my testing code..I will try to figure out why

ghost
ghost suggested changes Jun 17, 2019
View reviewed changes
src/client.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
@WindSoilder
Copy link
Contributor Author

WindSoilder commented Jun 18, 2019 •
edited
Loading

@seanmonstar Sorry for the disturb :( I should check my testing code more carefully.

And finally it's ready to review. I hope it can help to solve the issue.

ghost
ghost suggested changes Jun 18, 2019
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Just some small things I would change.
Also you might wanna run rusts formater fmt and check clippy.

Otherwise thanks for implementing features 😃

src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
@WindSoilder
Copy link
Contributor Author

Just some small things I would change.
Also you might wanna run rusts formater fmt and check clippy.

Otherwise thanks for implementing features 😃

Thanks for your suggestion, especially clippy give me a new world! I have changed some code according to your Suggestions.

But I don't think I need to run rusts formatter fmt, or this PR will contains too much file changes information (It will lead to too much file changes to review code).

Also notice that disable_all_formatting = true is used in the .rustfmt.toml file. And this is another reason why I don't use formatter.

seanmonstar
seanmonstar requested changes Jun 19, 2019
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Excellent progress, thank you! I've left some thoughts inline.

src/client.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/proxy.rs Outdated Show resolved Hide resolved
tests/client.rs Outdated Show resolved Hide resolved
seanmonstar
seanmonstar reviewed Jun 20, 2019
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Looks good! And I think the defaults are correct, for now. Suddenly using the system proxies in a patch upgrade could be surprising. I think we can change the default to looking for system proxies in the next breaking change.

src/lib.rs Outdated Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
@indygreg indygreg mentioned this pull request Jun 28, 2019
Closed
WindSoilder
WindSoilder commented Jun 28, 2019
View reviewed changes
src/client.rs Outdated Show resolved Hide resolved
ghost
ghost suggested changes Jun 28, 2019
View reviewed changes
src/async_impl/client.rs Show resolved Hide resolved
seanmonstar
seanmonstar reviewed Jun 28, 2019
View reviewed changes
src/client.rs Outdated Show resolved Hide resolved
seanmonstar
seanmonstar approved these changes Jul 1, 2019
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Woo! Thanks for keeping with this to the end, this'll be great!

@seanmonstar
Copy link
Owner

Ah, would you be able to rebase master onto this branch? I can't merge because there are conflicts...

@WindSoilder
built-in proxy support. For now reqwest will make a request with prox…
ea20717 
…y by default.
@WindSoilder
fix documentation.
61035ed
@WindSoilder
add unit tests for get_proxies.
a56f7c1
@WindSoilder
make get_proxies public.
be1a0e7
@WindSoilder
add testing for system proxy setting.
afad4b1
@WindSoilder
fix proxy tests.
8556f46
@WindSoilder
add extern crate winreg.
2acb6ef
@WindSoilder
move get_proxy tests from unit test into integration proxy test, and …
7dbbde1 
…try to use lock to avoid env operation problem.
@WindSoilder
add proxy env check in the testing code.
29f83ce
@WindSoilder
fix get_proxy tests.
697f95f
@WindSoilder
make code more readable.
f085115
@WindSoilder
test get_proxy is integration test.
8cd2df3
@WindSoilder
make code more rust style.
ecf3dfd
@WindSoilder
test for get_proxy envolve environment variables, so should make test…
f1ab75e 
…s run un-parallel.
@WindSoilder
fix typo, and add comments.
9fec27b
@WindSoilder
use system_proxy method instead of add parameter to ClientBuilder::ne…
cf4a7de 
…w, and add no_proxy method
@WindSoilder
add tests for use_system_proxy and no_proxy
be5343e
@WindSoilder
Copy link
Contributor Author

Ah, would you be able to rebase master onto this branch? I can't merge because there are conflicts...

I have rebase it, would you please try again ? I'm happy to have a chance to contribute it :D

@seanmonstar seanmonstar merged commit 577d06c into seanmonstar:master Jul 1, 2019
This was referenced Jul 4, 2019
Merged
Closed
@Mazwak Mazwak mentioned this pull request Oct 1, 2019
Closed
@jefflloyd jefflloyd mentioned this pull request Aug 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asonix asonix asonix left review comments

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WindSoilder @seanmonstar @asonix