launchpad-reviewers team mailing list archive

[Thomi Richards <thomi.richards@xxxxxxxxxxxxx>]

Thanks for the review William.

I made a few changes, but have questions about some of your suggestions (see diff comments). Perhaps a quick call would be the easiest way to resolve these remaining issues.

Cheers,

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]

Removed.

>          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

Hi William,

I've moved getOwnerIdFOrKey to GPGKeySet. I'm not sure what $something should be for addKeyForTest. 

Even after we remove the gpgkey table and associated code from launchpad, we'll still want tests that can make gpgservice report different things, the only difference will be that we might not be using IGPGKey instances. That shouldn't be too hard a change to make later on, and we may still want a type to represent gpg keys, rather than a dictionary, as they are now.

Your thoughts on this point would be appreciated :D

> +
>      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."""

Hmmm. Above you suggested moving this to IGPGKeySet. It seems odd to have a method on IGPGKeySet that takes a Person object. Once we remove the gpgkey table, this method won't be needed any more (the equivilent will be 'my_key.owner' (or similar), so I think we can fix that problem by just deleting the code.

Thoughts?



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