[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