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

Proposed by Paul Gear on 2014-09-02
Status: Superseded
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-02 Approve on 2014-09-21
Review via email: mp+232964@code.launchpad.net

This proposal has been superseded by a proposal from 2014-09-21.

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 :

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

Martin Pool (mbp) wrote :

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 :

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 :

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
6600. By Paul Gear on 2014-09-12

Move defaulting of proxy_info inside login

Richard Wilbur (richard-wilbur) wrote :

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

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/plugins/launchpad/lp_api.py'
2--- bzrlib/plugins/launchpad/lp_api.py 2012-07-20 15:09:13 +0000
3+++ bzrlib/plugins/launchpad/lp_api.py 2014-09-12 10:18:19 +0000
4@@ -23,6 +23,7 @@
5 # needed by a command that uses it.
6
7
8+import httplib2
9 import os
10 import re
11 import urlparse
12@@ -118,6 +119,8 @@
13
14 :return: The root `Launchpad` object from launchpadlib.
15 """
16+ if proxy_info is None:
17+ proxy_info = httplib2.proxy_info_from_environment('https')
18 cache_directory = get_cache_directory()
19 launchpad = Launchpad.login_with(
20 'bzr', _get_api_url(service), cache_directory, timeout=timeout,