Merge lp:~maxb/bzr/launchpadlib-service-root-api-compat into lp:bzr/2.1

Proposed by Max Bowsher
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the source branch.
Merged at revision: 4874
Proposed branch: lp:~maxb/bzr/launchpadlib-service-root-api-compat
Merge into: lp:bzr/2.1
Diff against target: 29 lines (+17/-2)
1 file modified
bzrlib/plugins/launchpad/lp_api.py (+17/-2)
To merge this branch: bzr merge lp:~maxb/bzr/launchpadlib-service-root-api-compat
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+47885@code.launchpad.net

Commit message

Fix broken communication with the Launchpad web service API when used with
launchpadlib >= 1.5.5 against production or dev.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

The approach seems reasonable, but it is it really true that lplib
exposes a constant for STAGING_SERVICE_ROOT but not for production?

--
Martin

Revision history for this message
Max Bowsher (maxb) wrote :

Yes, it's really true - earlier launchpadlib versions only offered you the choice of edge or staging.

Revision history for this message
Max Bowsher (maxb) wrote :

At some point in the future when we decide to increase our minimum launchpadlib version to 1.5.5 or later, we can tidy this up considerably.

Revision history for this message
Martin Pool (mbp) wrote :

> Yes, it's really true - earlier launchpadlib versions only offered you the choice of edge or staging.

That's a bit awful.

> At some point in the future when we decide to increase our minimum launchpadlib version to 1.5.5 or later, we can tidy this up considerably.

If you could just paste those two sentences into the comment that would probably save some head-scratching in future. Aside from that this looks good.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

So actually:

mbp@joy% rmadison python-launchpadlib
python-launchpadlib | 1.5.1-0ubuntu1 | karmic | source, all
python-launchpadlib | 1.6.0-0ubuntu1 | lucid | source, all
python-launchpadlib | 1.6.1-1 | maverick | source, all
python-launchpadlib | 1.8.0.is.1.6.2-0ubuntu1 | natty | source, all

I'm not sure about non-Ubuntu platforms, but I suspect anything that has bzr 2.1 or later will also have launchpadlib 1.6 or later. I think at least when this merges to trunk, we should just increment the launchpadlib requirement. For 2.1 it's probably better not to change it.

Revision history for this message
Max Bowsher (maxb) wrote :

> If you could just paste those two sentences into the comment that would
> probably save some head-scratching in future. Aside from that this looks
> good.

I've expanded the wording of the comment to make those points clearer.

Revision history for this message
Max Bowsher (maxb) wrote :

> I'm not sure about non-Ubuntu platforms, but I suspect anything that has bzr
> 2.1 or later will also have launchpadlib 1.6 or later. I think at least when
> this merges to trunk, we should just increment the launchpadlib requirement.
> For 2.1 it's probably better not to change it.

It depends... are we content to remove support for bzr launchpadlib features from bzr for karmic in the ~bzr PPA? Probably. But I thought I should explicitly mention that.

Revision history for this message
Martin Pool (mbp) wrote :

On 1 February 2011 21:26, Max Bowsher <email address hidden> wrote:
>> I'm not sure about non-Ubuntu platforms, but I suspect anything that has bzr
>> 2.1 or later will also have launchpadlib 1.6 or later.  I think at least when
>> this merges to trunk, we should just increment the launchpadlib requirement.
>> For 2.1 it's probably better not to change it.
>
> It depends... are we content to remove support for bzr launchpadlib features from bzr for karmic in the ~bzr PPA? Probably. But I thought I should explicitly mention that.

I think that's reasonable: it's pretty old, they're optional features,
and there could be backports of launchpadlib too.

Revision history for this message
Max Bowsher (maxb) wrote :

I believe Martin is now happy with this though didn't technically set the proposal to Approved:

23:55 * maxb wonders if poolie is still around?
23:56 < maxb> If so, could you nod at https://code.launchpad.net/~maxb/bzr/launchpadlib-service-root-api-compat/+merge/47885 and possibly send it to PQM?
00:03 < poolie> maxb: nod for 2.1
00:03 < poolie> on the phone at the moment

Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

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 2010-11-26 18:10:01 +0000
3+++ bzrlib/plugins/launchpad/lp_api.py 2011-02-01 10:26:50 +0000
4@@ -69,10 +69,25 @@
5 installed_version, installed_version)
6
7
8+# The older versions of launchpadlib only provided service root constants for
9+# edge and staging, whilst newer versions drop edge. Therefore service root
10+# URIs for which we do not always have constants are derived from the staging
11+# one, which does always exist.
12+#
13+# It is necessary to derive, rather than use hardcoded URIs because
14+# launchpadlib <= 1.5.4 requires service root URIs that end in a path of
15+# /beta/, whilst launchpadlib >= 1.5.5 requires service root URIs with no path
16+# info.
17+#
18+# Once we have a hard dependency on launchpadlib >= 1.5.4 we can replace all of
19+# bzr's local knowledge of individual Launchpad instances with use of the
20+# launchpadlib.uris module.
21 LAUNCHPAD_API_URLS = {
22- 'production': 'https://api.launchpad.net/beta/',
23+ 'production': STAGING_SERVICE_ROOT.replace('api.staging.launchpad.net',
24+ 'api.launchpad.net'),
25 'staging': STAGING_SERVICE_ROOT,
26- 'dev': 'https://api.launchpad.dev/beta/',
27+ 'dev': STAGING_SERVICE_ROOT.replace('api.staging.launchpad.net',
28+ 'api.launchpad.dev'),
29 }
30
31

Subscribers

People subscribed via source and target branches