Code review comment for lp:~hodgestar/software-properties/configurable-key-server

Hi Simon, thanks very much for the contribution to Ubuntu! I agree this could be a useful addition to the code. It doesn't happen often (these days, IME) but I have seen failures from the ubuntu keyserver, so in those cases it would be nice to have a fallback.

I have some comments on your patch. You're not too far off, but with a few changes, I think it would be a solid addition.

In add-apt-repository, I don't think you need the 'dest' argument to add_option(). The long version of the switch will be taken as the default dest. Also note the misspelling of 'Default' in the help string.

It looks like you have multiple places where 'hkp://keyserver.ubuntu.com:80/' is hardcoded. I'd refactor this so that it's defined exactly once, in a module global constant (upper-cased) in ppa.py. Then in apt-add-repository, I'd change the import at line 9 to be:

    from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line

(i.e. DEFAULT_KEYSERVER being the module global). Use that variable in the add_option() call.

Interestingly enough, the SoftwareProperties class already takes an `options` keyword in its __init__(), so I don't think you need to extend that method's signature to take a keyserver argument. In add-apt-repository, just create the instance like this:

    sp = SoftwareProperties(options=options)

I guess the only concern here is whether the `options` that SoftwareProperties takes is intended to be the command line options or something else. Since it's undocumented and untested, let's assume 'yes' although the use of self.options.massive_debug has me scratching my head. Anyway, I'd just pass in options to the constructor.

Now when you instantiate AddPPASigningKeyThread, pass in something like this:

    worker = AddPPASigningKeyThread(parsed_uri.path, options and options.keyserver)

Now in ppa.py, where you add a keyserver argument to AddPPASigningKeyThread.__init__(), use a default value of None instead of the hardcoded path, and do something like this in the __init__():

    self.keyserver = (keyserver if keyserver is not None else DEFAULT_KEYSERVER)

I hope that all makes sense!

I wish this were testable, because then I'd be sure to request that you add a test for your new code :). If you feel like giving it a shot, go for it, but as the rest of the code isn't well tested, I wouldn't block your branch on that.

Marking as needs-fixing for now, but I'm happy to review an update if you push it.