#271 add proxy with auth, #335
Conversation
| proxy = urllib2.ProxyHandler({o.scheme: os.environ[o.scheme+'_proxy']}) | ||
| auth = urllib2.HTTPBasicAuthHandler() | ||
| opener = urllib2.build_opener(proxy, auth, urllib2.HTTPHandler) | ||
| urllib2.install_opener(opener) |
There was a problem hiding this comment.
What is the scope of this call. It looks like it will set the opener for the duration of the program. I should probably use the non persistent call or cache the OpenerDirector object from the documentation you can just call "otherwise, simply call OpenerDirector.open() instead of urlopen()." which won't override all calls.
|
Overall this looks good. I think we should look at how we can promote this to be more reusable throughout the code base so that all our tools can work behind proxies. |
|
This seems to be only for downloading the default sources (i.e. |
|
Yes, and we should also set it up to pass the proxy through to the internal invocations of apt (or other package managers) as well. |
|
Thanks for comments. BTW, does someone know how to test proxy? is there proxyserver that everyone can use for testing? |
|
Thanks for looking into this. In terms of forwarding the proxy info to installers, apt is by far the most important case, but other's seem to be used a fair amount (pip, rpm, homebrew). If you have time, maybe you could look into those as well. The most flexible approach to texting proxy is likely one or two Ubuntu VMs: https://wiki.ubuntu.com/Testing/Proxy . There are lots of free proxies out there, but I haven't seen any with authentication. |
|
@k-okada you can relatively easily setup a squid proxy on your localhost by installing the squid package. @NikolausDemmel we will need to update the abstract API to allow passing the proxy info so that will become an option for all. I agree that apt is a top priority. |
|
ok, I have setup a squid on my local machine, add basic authentication, and confirmed it is working on both |
|
I would be ok to merge this as is and open a new ticket about extending the proxy support (briefly touched upon in the discussion above). This PR seems to enhance the situation for some common scenarios, while not deteriorating other cases. Installing a global opener is not the best design, but it is for now only done for the |
|
Is there anything that blocks this to be merged? |
|
I'm ok with merging it, someone speak up if you disagree (I can revert it). |
| @@ -207,6 +207,16 @@ def check_for_sources_list_init(sources_cache_dir): | |||
| else: | |||
| return True | |||
|
|
|||
| def setup_proxy_opener(): | |||
| import urllib2 | |||
There was a problem hiding this comment.
This patch breaks the Python 3 package. @k-okada Please add unit test coverage for the new code.
There was a problem hiding this comment.
I fixed the incompatible code with #351 but it still needs to be covered by unit tests.
There was a problem hiding this comment.
does c36626a covers def setup_proxy_opener() ?
this is workaround for #271. but this code has not been tested, since i do not have test environment and our client who had this trouble somehow resolved this problem before I create this patch. I just create PR not to forget code.
if you find someone with proxy/auth trouble , please check if
https://gist.githubusercontent.com/k-okada/cf79038d1cffc943a8d3/raw/b4d21725af7de2ac318d5a27ca6003bb1e6e0d95/rosdep work
this workaround is based on http://stackoverflow.com/questions/34079/how-to-specify-an-authenticated-proxy-for-a-python-http-connection