launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/archivepublisher/signing.py'
> --- lib/lp/archivepublisher/signing.py	2016-05-26 10:46:04 +0000
> +++ lib/lp/archivepublisher/signing.py	2016-05-26 15:57:07 +0000
> @@ -14,6 +14,7 @@
>  __metaclass__ = type
>  
>  __all__ = [
> +    "UefiUpload",
>      "SigningUpload",
>      ]

Please sort.

>  
> @@ -61,7 +62,10 @@
>      publisher configuration.  In this directory, the private key is
>      "uefi.key" and the certificate is "uefi.crt".
>      """
> -    custom_type = "signing"
> +    custom_type = "raw-signing"

custom_type shouldn't have the "raw-" prefix; none of the other implementations have that.

> +
> +    def distsDirectory(self):
> +        return 'signed'

A method is overkill.  This could just be a simple class attribute: "dists_directory = 'signed'"

>  
>      @staticmethod
>      def parsePath(tarfile_path):
> @@ -302,3 +306,11 @@
>  
>      def shouldInstall(self, filename):
>          return filename.startswith("%s/" % self.version)
> +
> +
> +class UefiUpload(SigningUpload):
> +    """Legacy UEFI Signing custom upload."""

May be worth a slightly longer explanation of why this exists (preferably in the description part of a multi-line docstring; see PEP 257), since presumably we eventually want to be able to remove publication support for the old type somewhere down the line, but it will likely be years away so we should record institutional memory in the codebase.

> +    custom_type = "raw-uefi"

See above.

> +
> +    def distsDirectory(self):
> +        return 'uefi'
> 
> === modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
> --- lib/lp/archiveuploader/nascentuploadfile.py	2016-05-06 09:16:30 +0000
> +++ lib/lp/archiveuploader/nascentuploadfile.py	2016-05-26 15:57:07 +0000
> @@ -309,7 +313,8 @@
>          """Return whether this custom upload can be automatically approved."""
>          # Signing uploads will be signed, and must therefore be approved
>          # by a human.
> -        if self.custom_type == PackageUploadCustomFormat.SIGNING:
> +        if self.custom_type in (PackageUploadCustomFormat.UEFI,
> +            PackageUploadCustomFormat.SIGNING):

Ugly and somewhat confusing indentation.  Prefer one of:

        if self.custom_type in (PackageUploadCustomFormat.UEFI,
                                PackageUploadCustomFormat.SIGNING):
            ...

        if self.custom_type in (
                PackageUploadCustomFormat.UEFI,
                PackageUploadCustomFormat.SIGNING):
            ...

>              return False
>          return True
>  
> 
> === modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
> --- lib/lp/soyuz/scripts/custom_uploads_copier.py	2016-05-06 09:16:30 +0000
> +++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2016-05-26 15:57:07 +0000
> @@ -63,7 +67,8 @@
>              return True
>          # Signing uploads will be signed, and must therefore be approved
>          # by a human.
> -        if custom.customformat == PackageUploadCustomFormat.SIGNING:
> +        if custom.customformat in (PackageUploadCustomFormat.UEFI,
> +            PackageUploadCustomFormat.SIGNING):

Indentation: see above.

>              return False
>          return True
>  


-- 
https://code.launchpad.net/~apw/launchpad/signing-reinstate-raw-uefi-custom-upload/+merge/295848
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.