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

Proposed by Paul Gear on 2014-09-21
Status: Merged
Approved by: Richard Wilbur on 2014-09-22
Approved revision: 6600
Merged at revision: 6599
Proposed branch: lp:~paulgear/bzr/proxy-environment-default
Merge into: lp:bzr
Diff against target: 20 lines (+3/-0)
1 file modified
bzrlib/plugins/launchpad/lp_api.py (+3/-0)
To merge this branch: bzr merge lp:~paulgear/bzr/proxy-environment-default
Reviewer Review Type Date Requested Status
Richard Wilbur 2014-09-21 Approve on 2014-09-21
Review via email: mp+235395@code.launchpad.net

This proposal supersedes a proposal from 2014-09-02.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Paul Gear (paulgear) wrote : Posted in a previous version of this proposal

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

Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote :

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) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
23# needed by a command that uses it.23# needed by a command that uses it.
2424
2525
26import httplib2
26import os27import os
27import re28import re
28import urlparse29import urlparse
@@ -118,6 +119,8 @@
118119
119 :return: The root `Launchpad` object from launchpadlib.120 :return: The root `Launchpad` object from launchpadlib.
120 """121 """
122 if proxy_info is None:
123 proxy_info = httplib2.proxy_info_from_environment('https')
121 cache_directory = get_cache_directory()124 cache_directory = get_cache_directory()
122 launchpad = Launchpad.login_with(125 launchpad = Launchpad.login_with(
123 'bzr', _get_api_url(service), cache_directory, timeout=timeout,126 'bzr', _get_api_url(service), cache_directory, timeout=timeout,