-
Notifications
You must be signed in to change notification settings - Fork 135
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
Http proxy support #276
Http proxy support #276
Conversation
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.
Looks really good, but I had a question about why we are passing the proxy settings in two places. Looks like we should be fine just with the addition of the proxies
kw arg at the transport level.
resp = transport.get(url, | ||
params=params, | ||
verify=SETTINGS.get('verify_https', True), | ||
proxy=SETTINGS.get('http_proxy'), |
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.
Is there a reason to pass these values directly in the kw
of the .get
and .post
calls?
Checking the requests documentation it looks like just setting the proxies
kw, as you did below, should be enough.
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.
Would you recommend to pass the proxies
dict as a keyword to the transport.post
and transport.get
methods? Would be fine with me but i thought the transport
module is meant to be an abstraction for the synchronous HTTP requests API. If so it should better not use requests
specific constructs. Should I refactor the PR to pass the proxies
dict directly to transport
?
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.
Oh no. I just mean, you already provide the proxies
keyword inside of the transport calls. Is there a need to also specify the proxy settings at each call site to the transport? I'm curious why we also pass proxy
proxy_user
proxy_password
to the transport.get
if the implementation of transport.get
should be providing the proxies
keyword now.
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.
The _get_proxy_cfg
method in the transport
module builds the proxies
config dict for requests. To do so it needs access to the http_proxy
, http_proxy_user
and http_proxy_password
settings. So either we access the settings in the transport
module or we need to pass along http_proxy
, http_proxy_user
and http_proxy_password
as keywords when we call a transport
method. Would you prefer another solution? The settings could be passed along as an argument as well, or stored in transport
as a thread local e.g.
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.
Ah hah. Now I get it. I apologize, I was focused on the kw being pass through to the actual session call, and missed that it is used, as you say, to build the proxies
value from the SETTINGS
. This is an interesting scenario where, as you say, the SETTINGS object might preferably be refactored to be more shareable to all modules. But that isn't necessary for this change. Thank you for your patience answering my question and work on this PR, I'll get it merged.
@cbergmiller Thank you again for your contribution. I've just finished uploading the new release with your work in version 0.14.3 to pypi. |
This is a proposal for #246
Users can set
http_proxy
(host:port),http_proxy_username
andhttp_proxy_password
.If user and password is given, basic auth is used to open the proxy.
If
http_proxy
is not set, no proxy will be used.