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

Add option to proxy requests #32

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Add option to proxy requests #32

merged 1 commit into from
Feb 21, 2024

Conversation

Lcchy
Copy link
Contributor

@Lcchy Lcchy commented Feb 17, 2024

Resolves #30

Copy link
Owner

@wezm wezm left a comment

Choose a reason for hiding this comment

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

Thanks for this. Some minor changes requested.

README.md Outdated Show resolved Hide resolved
Comment on lines +109 to +110
.or(env::var("http_proxy").ok())
.or(env::var("HTTPS_PROXY").ok());
Copy link
Owner

Choose a reason for hiding this comment

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

This works for now but I think the intention with these env vars is that they can both be set and point to different proxy servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how this works in conjunction, should it decide to use a certain one depending on the request itself being http/https?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's a remnant of the early internet where you could have different proxies for different protocols. Here's Netscape on Mac OS 8.1

proxy-settings

So yes, it would direct the request to depending on the protocol of the request. The example on https://docs.rs/reqwest/latest/reqwest/struct.Proxy.html demonstrates this: It uses Proxy::http with a https proxy url to "intercept all HTTP requests, and make use of the proxy at https://secure.example".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see! Thanks for the interesting context, let me know if you think it's useful/needed and I could open another PR. I personally don't need it for my use.

Copy link
Owner

Choose a reason for hiding this comment

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

I think I’ll tweak it to match those semantics before publishing a release but no expectation for you to do it. I can do it.

src/main.rs Outdated Show resolved Hide resolved
@Lcchy
Copy link
Contributor Author

Lcchy commented Feb 19, 2024

Updated the PR with your suggestions, thanks!

@wezm wezm merged commit 1b82072 into wezm:main Feb 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding the option to route requests through a proxy
2 participants