[MERGE/RFC] Add support for Launchpad API to plugin, including 'mirror branch now' command
Jonathan Lange
jml at mumak.net
Tue Jul 14 09:18:28 BST 2009
On Thu, Jul 9, 2009 at 5:56 PM, Vincent Ladeuil<v.ladeuil+lp at free.fr> wrote:
>>>>>> "jml" == Jonathan Lange <jml at mumak.net> writes:
>
> jml> On Sat, Jul 4, 2009 at 11:19 AM, Jonathan Lange<jml at mumak.net> wrote:
> >> Hello all,
> >>
> >> Attached is a bundle that adds a command 'lp-mirror' to Bazaar, that
> >> requests the branch be mirrored instantly.
>
> Yummy, that could certainly help a lot with our merge proposals
> madnesses...
>
It might, although there are other bugs on the Launchpad server that
really need to be fixed.
> >>
> >> It also adds a simple wrapper API for the Launchpad API, encapsulating
> >> the two major operations I think we'll need:
> >> 1. Log in to the Launchpad REST service
> >> 2. A function, load_branch, to get a Launchpad branch object based
> >> on a Bazaar branch
> >>
> >> The load_branch function kind of sucks, because Launchpad doesn't
> >> provide a way of getting a branch based on a URL. I've filed this as
> >> bug 395076 and will start fixing that bug asap.
> >>
> >> The bundle is missing:
> >> - a way to 'logout', or rather to invalidate the cached login data
> >> - tests
>
> right, so BB:reject :0D
>
> Kidding,
> BB:comment only as that is what you think to be asking for.
>
Thanks :)
> ...
> jml> === modified file 'bzrlib/plugins/launchpad/__init__.py'
> jml> --- bzrlib/plugins/launchpad/__init__.py 2009-06-19 15:10:17 +0000
> jml> +++ bzrlib/plugins/launchpad/__init__.py 2009-07-04 09:29:51 +0000
> jml> @@ -48,6 +48,11 @@
> jml> NotLaunchpadBranch,
> jml> )
>
> jml> +try:
> jml> + from bzrlib.plugins.launchpad import lp_api
> jml> +except ImportError:
> jml> + lp_api = None
> jml> +
>
>
> Right, so it seems my tests stop right there :-/
>
> I installed launchpadlib at first, ok, I had to do that for a
> while anyway, 'httplib2', why not and then 'lazr' caught my eye,
> but setting a breakpoint in the except clause above told me that
> the next missing one was 'oauth.oauth'.
>
> A bit too much :-/
>
> Can you clarify the dependencies ?
>
Yes.
- oauth
- lazr.uri
- launchpadlib
- wadllib
- simplejson
- httplib2
> jml> +class cmd_launchpad_mirror(Command):
> jml> + """Ask Launchpad to mirror a branch now."""
> jml> +
> jml> + aliases = ['lp-mirror']
> jml> + takes_args = ['location?']
> jml> +
> jml> + def run(self, location='.'):
> jml> + service = LaunchpadService()
> jml> + launchpad = lp_api.login(service)
> jml> + branch = _mod_branch.Branch.open(location)
> jml> + lp_branch = lp_api.load_branch(launchpad, branch)
> jml> + lp_branch.requestMirror()
>
> Hmm, without testing I'm not sure about what the default will be,
> but I'm pretty sure I will prefer to rely on a branch.conf
> variable (that can be overridden in from location.conf, etc, as
> usual).
>
In this implementation, load_branch will try the public & the push
locations of the branch to see if either of them actually refers to
bazaar.launchpad.net.
In a future implementation (see below), load_branch will ask Launchpad
if it recognizes either of the public & push locations.
> jml> === added file 'bzrlib/plugins/launchpad/lp_api.py'
>
> jml> +from lazr.uri import URI
>
> What's that ?
>
lazr.uri is a module that launchpadlib depends on. It's an OO
abstraction for URLs.
> And just a general remark, we differ from the launchpad project
> coding rules here, I think, in that we prefer to use:
>
> from lazr import uri
>
> and then
>
> uri.URI
>
Changed.
> jml> +def _get_api_url(service):
>
> jml> +def _get_credential_path(service):
>
> jml> +def login(service, timeout=None, proxy_info=None):
>
> Not a lot to say here, I can't see how to integrate that scheme
> into our existing credentials handling, which is sad :-/
>
> Or is it done upper in the stack (after all I'm connecting to
> launchpad almost every minute and all I have a section in
> authentication.conf mentioning my launchpad ID...).
>
I can't think of an easy way. I think that once this patch lands and
people start using it, we'll be better placed to figure out how to
improve the authentication situation.
> jml> +def load_branch(launchpad, branch):
> jml> + """Return the launchpadlib Branch object corresponding to 'branch'.
> jml> +
> jml> + :param launchpad: The root `Launchpad` object from launchpadlib.
> jml> + :param branch: A `bzrlib.branch.Branch`.
> jml> + :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
> jml> + `branch`.
> jml> + :return: A launchpadlib Branch object.
> jml> + """
>
>
> As far as API is concerned, I'd feel better if I could just
> provide a lp URL and get back a http or bzr+ssh URL.
>
That API is definitely useful, but this API is also necessary.
If you want to do stuff with branches on launchpadlib, you need to
actually get at launchpadlib Branch object. Once you've got the Branch
object, you can get the bzr+ssh and http URLs (I think).
At the time this patch was written, Launchpad didn't provide a public
API for getting a Branch object by URL (lp: or otherwise). Since then,
I've patched Launchpad to provide such an API.
I was planning on making the plugin use this API in a future patch,
but perhaps I should just do it in this one?
So, uhh, to summarize, I want two major branch-getting functions in this method:
load_branch :: bzrlib.branch.Branch -> launchpadlib.Branch
get_branch_at_url :: String -> launchpadlib.Branch
The first one will probably be implemented in terms of the second.
> jml> === modified file 'bzrlib/plugins/launchpad/lp_directory.py'
> jml> --- bzrlib/plugins/launchpad/lp_directory.py 2009-03-23 14:59:43 +0000
> jml> +++ bzrlib/plugins/launchpad/lp_directory.py 2009-07-03 14:24:23 +0000
> jml> @@ -63,15 +63,9 @@
> jml> _request_factory=ResolveLaunchpadPathRequest,
> jml> _lp_login=None):
> jml> """Resolve the base URL for this transport."""
> jml> + service = LaunchpadService.for_url(url)
> jml> result = urlsplit(url)
> jml> - # Perform an XMLRPC request to resolve the path
> jml> - lp_instance = result[1]
> jml> - if lp_instance == '':
> jml> - lp_instance = None
> jml> - elif lp_instance not in LaunchpadService.LAUNCHPAD_INSTANCE:
> jml> - raise errors.InvalidURL(path=url)
> jml> resolve = _request_factory(result[2].strip('/'))
> jml> - service = LaunchpadService(lp_instance=lp_instance)
>
>
> Can we get that without the rest of your patch ?
>
No! I'm holding it for ransom :P
> jml> === modified file 'bzrlib/plugins/launchpad/lp_registration.py'
>
> <snip/>
>
> jml> + @classmethod
> jml> + def for_url(cls, url, **kwargs):
> jml> + """Return the Launchpad service corresponding to the given URL."""
> jml> + result = urlsplit(url)
> jml> + lp_instance = result[1]
> jml> + if lp_instance == '':
> jml> + lp_instance = None
> jml> + elif lp_instance not in cls.LAUNCHPAD_INSTANCE:
> jml> + raise errors.InvalidURL(path=url)
> jml> + return cls(lp_instance=lp_instance, **kwargs)
> jml> +
>
> <snip/>
>
> As for testing strategy, I'd go with the dumbest lp server you
> can think of with a list of needed requests and an associated
> list of pre-canned responses.
>
Yeah, I guess so.
> You will certainly need to avoid using https and make
> CACHE_DIRECTORY and LAUNCHPAD_API_URLS overridable by tests.
>
> And while you work on that, now that you mention it, you may
> try to give https://bugs.edge.launchpad.net/bzr/+bug/186920 a
> shot, especially taking
> https://bugs.edge.launchpad.net/bzr/+bug/186920/comments/31 into
> consideration...:-P
>
Hah! Probably not. Part of the point of using launchpadlib is to get
away from xmlrpclib.
Thanks for the comments.
jml
More information about the bazaar
mailing list