-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: Add support for proxy config #229
base: main
Are you sure you want to change the base?
Conversation
Added proxy_url to shorebird.yaml to use with reqwest if specified
library/src/network.rs
Outdated
Ok(client) | ||
} | ||
|
||
fn read_config() -> anyhow::Result<YamlConfig> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typically config is set through https://github.com/shorebirdtech/updater/blob/main/library/src/config.rs#L91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, looks like the request callbacks would now need to be config-specifc. 🤔
I'm open to this change. The use case of "we want to use Shorebird behind a corporate network" makes sense. It is unfortunate that we can't (easily) fix this by using whatever existing network interface was already correctly configured. It feels wrong to have to configure Shorebird's https client specifically. I think the only thing blocking this landing is one of us fiddling with how to plumb the shorebird.yaml reading through to where it's normally done (and also probably change how the network callbacks are created). |
Removed the proposed yaml config var in favor of reqwest pulling the system proxy for us. |
If this change works, it makes a lot of sense and would be easy for us to land. Thoughts @bryanoltman? |
My guess is this won't work, unless reqwest has iOS or Android specific code to read the proxy config (which I doubt it has). We also presumably would have to add the "socks" option to our reqwest import: |
Description
Added
proxy_url
to shorebird.yaml to use with reqwest client innetwork.rs
(if defined)Type of Change