Initial commit for proxy support#1936
Conversation
| # If we have proxy env vars, we need to use the requests transport | ||
| # to ensure that we use the correct proxy settings. |
There was a problem hiding this comment.
Have you considered using requests inconditionally here? Without any proxy tests, requests support could be subtly broken.
There was a problem hiding this comment.
I considered it - my aim here though was to try and retain as much current behaviour when not using proxies as possible. I was aiming to get in a set of proxy tests to cover the proxy use case.
There was a problem hiding this comment.
What would happen with any of these variables set to empty value? Example:
http_proxy=There was a problem hiding this comment.
requests would be used, but requests likely would not use the proxy. Requests actually looks at any env var ending with _proxy - we can check if there is a value if you feel that is necessary, though i don't want to start doing any other validation on the input, since that will automatically be up to the client that we are using (requests for synchronus, aiohttp for async).
fressi-elastic
left a comment
There was a problem hiding this comment.
Please add unit test(s) for this fix so we can test it without having to deal with actually making requests to a server. Also please validate what would happen with any of these variables set as an empty value.
| # If we have proxy env vars, we need to use the requests transport | ||
| # to ensure that we use the correct proxy settings. |
There was a problem hiding this comment.
What would happen with any of these variables set to empty value? Example:
http_proxy=| # If we have proxy env vars, we need to use the requests transport | ||
| # to ensure that we use the correct proxy settings. | ||
| self.logging.warning("Proxy settings detected. Using requests transport.") | ||
| super().__init__(*args, node_class=RequestsHttpNode, **kwargs) |
There was a problem hiding this comment.
If kwargs already contains "node_class" key, then this would raise an exception. I think you should add node_class as an expecific parameter at the beginning of this method so that in case is None then we can replace its value. Example:
def __init__(self, node_class = None, **kwargs):
# ...
if node_class is None and any(x in os.environ for x in proxy_env_vars):
node_class = RequestsHttpNode
super().__init__(*args, node_class=node_class, **kwargs)
Initial support for proxies.
The aim is to rely on existing support from requests or aiohttp.
Next steps: