[MERGE] add a command for managing the Launchpad user ID.
James Henstridge
james at jamesh.id.au
Fri Oct 12 06:06:31 BST 2007
On 12/10/2007, Martin Pool <mbp at canonical.com> wrote:
> Martin Pool has voted resubmit.
> Status is now: Resubmit
> Comment:
>
> +
> +class UnknownLaunchpadUsername(errors.BzrError):
> + _fmt = "The user name %(user)s is not registered on Launchpad."
> +
> +
> +class NoRegisteredSSHKeys(errors.BzrError):
> + _fmt = "The user %(user)s has not registered any SSH keys with
> Launchpad."
> +
>
> There are no tests for the formatting of these exceptions.
> (You might think that is too trivial to test but it is easy to
> get the format string wrong.)
I have added tests similar to those found in test_errors.py
> +def get_lp_login(config=None):
> + """Return the user's Launchpad Username"""
> + if config is None:
> + config = GlobalConfig()
> +
> + return config.get_user_option('launchpad_username')
>
> Further to Aaron's comment, I think you at least need a docstring
> about what should be passed as the config. s/Username/username/
As mentioned previously, I don't expect anything to be passed in as
config. I have renamed the variable and fixed this capitalisation
bug.
> +def check_lp_login(username, transport=None):
> + """Check whether the given Launchpad username is okay.
> +
> + This will check for both existance and whether the user has
> + uploaded SSH keys.
> + """
> +
> + if transport is None:
> + transport = get_transport(LAUNCHPAD_BASE)
> +
>
> I think you should explain what transport is meant to be for. If it
> is only for testing, it would be more consistent with other tests for
> you to hook and then reset the variable -- or at least make call it
> _transport.
This argument was also in here for testing. I've renamed it too.
> Also there is already an environment variable(?) saying Launchpad's
> address, maybe that should be used here too?
The variable is for the XMLRPC endpoint, so is not directly usable
here (I guess that's why it wasn't used in lp_indirect.py either).
I agree that it'd be good to reduce the number of hard coded domain
names here, and hope to be able to do it as we improve the APIs
Launchpad provides. I think it'd be worth adding an explicit "check
if this Launchpad user ID is okay" API for use here, but I think this
will be okay for now.
> Is there a test/guarantee in Launchpad that ~user/+sshkeys will always
> be supported?
I believe so, and I don't think this is the only thing relying on the
URL. I will check up on this and file a bug if such a test is
missing.
> Does this give a reasonable error if the server is unreachable?
It returns the standard HTTP transport connection failure messages.
At the moment, this appears to be:
bzr: ERROR: Connection error: curl connection error (Couldn't resolve
host 'launchpad.net')
on https://launchpad.net/%7Efoobar/%2Bsshkeys
> + Set the Launchpad ID of the current user::
> +
> + bzr launchpad-login $USERNAME
>
> That's not such a good example because it won't work on Windows.
I've changed the documentation text to set an explicit user name.
> @@ -129,10 +173,11 @@
> from unittest import TestSuite, TestLoader
> import test_register
> import test_lp_indirect
> + import test_account
>
> Since this was written we've decided to expose plugins as e.g.
> bzrlib.plugins.launchpad, and not to use relative imports. So can you
> change them all to fully-qualified names?
I have updated all files in the plugin to use absolute imports.
> This should be in NEWS.
I've added an item on the new command.
> + takes_options = [
> + Option('no-check',
> + "Don't check that the user name is valid."),
> + ]
> +
> + def run(self, name=None, no_check=None):
> + import account
> + check_account = not no_check
>
> Isn't there an easier way to saay this is a boolean that defaults to on?
It seems that I'd also get --check/--no-check options if I I called
the option 'check', but that would cause --check to be included in the
help output. Given that the default is to check, that would be a bit
confusing.
The above appears to be the idiom used in bzrlib/builtins.py. I agree
that it would be nice to be able to do this in a cleaner way.
> + else:
> + self.outf.write('No Launchpad user ID configured.\n')
> + else:
>
> Maybe it should return 1 for failure?
Done.
An updated bundle is attached.
James.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-lp-login.patch
Type: text/x-diff
Size: 21146 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071012/46a65758/attachment-0001.bin
More information about the bazaar
mailing list