launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> 
> === modified file 'lib/lp/archivepublisher/archivesigningkey.py'
> --- lib/lp/archivepublisher/archivesigningkey.py	2015-09-13 18:30:51 +0000
> +++ lib/lp/archivepublisher/archivesigningkey.py	2016-02-24 01:32:53 +0000
> @@ -105,9 +105,8 @@
>  
>          algorithm = GPGKeyAlgorithm.items[pub_key.algorithm]

Unused now?

>          key_owner = getUtility(ILaunchpadCelebrities).ppa_key_guard
> -        self.archive.signing_key = getUtility(IGPGKeySet).new(
> -            key_owner, pub_key.keyid, pub_key.fingerprint, pub_key.keysize,
> -            algorithm, active=True, can_encrypt=pub_key.can_encrypt)
> +        self.archive.signing_key, _ = getUtility(IGPGKeySet).activate(
> +            key_owner, pub_key, pub_key.can_encrypt)
>  
>      def signRepository(self, suite):
>          """See `IArchiveSigningKey`."""
> 
> === modified file 'lib/lp/services/gpg/handler.py'
> --- lib/lp/services/gpg/handler.py	2016-02-24 01:32:53 +0000
> +++ lib/lp/services/gpg/handler.py	2016-02-24 01:32:53 +0000
> @@ -699,6 +711,29 @@
>              raise ValueError("%r not registered.")
>          self.write_hooks.remove(hook_callable)
>  
> +    def addKeyForTest(self, owner_id, key):
> +        """See IGPGClient."""
> +        document = {'keys': [{
> +            'owner': owner_id,
> +            'id': key.keyid,
> +            'fingerprint': key.fingerprint,
> +            'size': key.keysize,
> +            'algorithm': key.algorithm.name,
> +            'enabled': key.active,
> +            'can_encrypt': key.can_encrypt}]}
> +        path = '/test/add_keys'
> +        resp = self._request('post', path, document)
> +        if resp.status_code == http_codes['NOT_FOUND']:
> +            raise RuntimeError(
> +                "gpgservice was not configured with test endpoints enabled.")
> +        elif resp.status_code != http_codes['OK']:
> +            self.raise_for_error(resp)
> +
> +    def getOwnerIdForKey(self, lp_key):
> +        """See IGPGClient."""
> +        return lp_key.owner.account.openid_identifiers.order_by(
> +            OpenIdIdentifier.identifier).first().identifier

These are the only parts of the client that touch the LP DB, and so they will hurt your goal of extracting it. Perhaps move getOwnerIdForKey to GPGKeySet, and do $something with addKeyForTest?

> +
>      def _notify_writes(self):
>          errors = []
>          for hook in self.write_hooks:
> 
> === modified file 'lib/lp/services/gpg/interfaces.py'
> --- lib/lp/services/gpg/interfaces.py	2016-02-24 01:32:53 +0000
> +++ lib/lp/services/gpg/interfaces.py	2016-02-24 01:32:53 +0000
> @@ -474,3 +482,18 @@
>  
>          :raises ValueError: if hook_callable was not registered.
>          """
> +
> +    def addKeyForTest(owner_id, key):
> +        """Add a key to the gpgservice without checking the keyserver.
> +
> +        This method is to be used for TESTING purposes only. The running
> +        gpgservice instance must have its test methods configured - something
> +        that should not be done in production. If this requirement is not met
> +        a RuntimeError will be raised.
> +
> +        :param key: An IGPGKey instance.
> +
> +        """
> +
> +    def getOwnerIdForKey(key):
> +        """Given an IGPGKey instance, retrieve its owner id."""

I'd have this take a Person directly. In a branch or two there will be no IGPGKey before the key is in the service -- there'll be no database table to refer to.



-- 
https://code.launchpad.net/~thomir/launchpad/devel-start-integration/+merge/286972
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.