Merge lp:~paulgear/bzr/proxy-environment-default into lp:bzr

[Paul Gear]

Allows launchpad APIs to use proxies by default(Paul Gear).

Allows launchpad APIs to use proxies by default, without every application needing to support it individually. This allows lp-propose-merge plugin to be used behind an egress-filtered firewall, among other things. The security considerations of this change need to be considered, however it makes the launchpad plugin follow the expected convention that Linux commands support https_proxy.

[Paul Gear (paulgear)]

Hat tip to https://code.launchpad.net/~hloeung/ for the technique.

[Martin Pool (mbp)]

Thanks for proposing a patch for this.

Generally, calling functions or capturing values from a function default
value (therefore at module load time) is an antipattern in Python - it's
better to make it None and then set it inside the function. Unless there's
something I'm missing, but then there should probably be a comment.

[Paul Gear (paulgear)]

I'm quite new to Python, so I'm completely ignorant of its patterns & antipatterns. Feel free to mangle this into something workable for you, or if you prefer I could reissue the patch and call the function from inside.

[Richard Wilbur (richard-wilbur)]

Thanks, Paul, for helping the bzr launchpad plugins get a reasonable default to support https proxies.

My understanding of Martin's comment coincides with the second option you proposed: "I could reissue the patch and call the function from inside."

I'd prefer to proceed that way: update your local branch, push it up here again, then request a new review.

Regarding security issues: this solution still allows calling with a proxy specified explicitly in the argument list, only the default behaviour is changed and that is now to use the https proxy configured for the system. I don't see a problem.

review: Needs Fixing

[Richard Wilbur (richard-wilbur)]

Good work, Paul. Your new version doesn't lengthen library load time. Thanks for the fix.

Regarding security issues: this solution still allows calling with a proxy specified explicitly in the argument list, only the default behaviour is changed and that is now to use the https proxy configured for the system. I still don't see a problem.

+1

review: Approve

[Richard Wilbur (richard-wilbur)]

Like I replied to the old merge proposal with the new patch:

Good work, Paul. Your new version doesn't lengthen library load time. Thanks for the fix.

Regarding security issues: this solution still allows calling with a proxy specified explicitly in the argument list, only the default behaviour is changed and that is now to use the https proxy configured for the system. I still don't see a problem.

+1

Now, with that in the new merge proposal, I can actually approve the correct ("unsuperseded") proposal.

review: Approve

[Richard Wilbur (richard-wilbur)]

sent to pqm by email

=== modified file 'bzrlib/plugins/launchpad/lp_api.py'
--- bzrlib/plugins/launchpad/lp_api.py	2012-07-20 15:09:13 +0000
+++ bzrlib/plugins/launchpad/lp_api.py	2014-09-21 04:45:08 +0000
@@ -23,6 +23,7 @@
 # needed by a command that uses it.
 
 
+import httplib2
 import os
 import re
 import urlparse
@@ -118,6 +119,8 @@
 
     :return: The root `Launchpad` object from launchpadlib.
     """
+    if proxy_info is None:
+        proxy_info = httplib2.proxy_info_from_environment('https')
     cache_directory = get_cache_directory()
     launchpad = Launchpad.login_with(
         'bzr', _get_api_url(service), cache_directory, timeout=timeout,