Merge ~cjwatson/launchpad:refactor-ci-build-upload-job into launchpad:master

[Colin Watson]

Refactor CIBuildUploadJob.run

The `CIBuildUploadJob.run` method was getting a bit long and unwieldy. Break it up with a couple of new helper methods.

`ScannedArtifact` isn't all that useful yet, but I'll be adding a couple more things to it soon so it seemed better to have this abstraction rather than a simple tuple.

[Andrey Fedoseev (andrey-fedoseev)]

Looks good

A few notes:

1. I typically prefer to place higher-level methods above the lower-level methods that they call. In this case, the order would be:

  def run():
  def _scanArtifacts()
  def _scanFile()
  def _uploadPackages()

This follows the order in which the methods are called and gives the reader a nice top-down view.

2. I suggest adding type annotations to the new methods that you've extracted.

review: Approve

[Colin Watson (cjwatson)]

1. I've always specifically preferred the opposite of that. This is partly because I learned rather less capable languages than Python first, but even in Python forward references are occasionally problematic (e.g. decorators must be declared before functions they decorate), and so I try to stick to backward references wherever possible. My mental model of programs tends to be more about building up from basic functions until I reach the overall goal that brings everything together, so I find that this suits me better when reading programs too. (Maybe it's because I used to be a mathematician, and this bottom-up order is how proofs are normally stated?)

2. Yes, I expect to need some reminding of this until I really get into the habit, sorry! Done.

[Jürgen Gmach (jugmac00)]

Not that it adds any value, but I never cared about method ordering, as I usually keep my classes short and I have IDE support for jumping from callers to implementation and back again.

diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
index e2bf402..d92edd3 100644
--- a/lib/lp/soyuz/model/archivejob.py
+++ b/lib/lp/soyuz/model/archivejob.py
@@ -1,12 +1,19 @@
 # Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from collections import OrderedDict
 import io
 import json
 import logging
 import os.path
 import tarfile
 import tempfile
+from typing import (
+    Any,
+    Dict,
+    Iterable,
+    List,
+    )
 import zipfile
 
 from lazr.delegates import delegate_to
@@ -27,7 +34,10 @@ import zstandard
 
 from lp.code.enums import RevisionStatusArtifactType
 from lp.code.interfaces.cibuild import ICIBuildSet
-from lp.code.interfaces.revisionstatus import IRevisionStatusArtifactSet
+from lp.code.interfaces.revisionstatus import (
+    IRevisionStatusArtifact,
+    IRevisionStatusArtifactSet,
+    )
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
     )
@@ -202,6 +212,15 @@ class ScanException(Exception):
     """A CI build upload job failed to scan a file."""
 
 
+class ScannedArtifact:
+
+    def __init__(
+        self, *, artifact: IRevisionStatusArtifact, metadata: Dict[str, Any]
+    ):
+        self.artifact = artifact
+        self.metadata = metadata
+
+
 @implementer(ICIBuildUploadJob)
 @provider(ICIBuildUploadJobSource)
 class CIBuildUploadJob(ArchiveJobDerived):
@@ -307,7 +326,9 @@ class CIBuildUploadJob(ArchiveJobDerived):
     def target_channel(self):
         return self.metadata["target_channel"]
 
-    def _scanCondaMetadata(self, index, about):
+    def _scanCondaMetadata(
+        self, index: Dict[Any, Any], about: Dict[Any, Any]
+    ) -> Dict[str, Any]:
         return {
             "name": index["name"],
             "version": index["version"],
@@ -322,7 +343,7 @@ class CIBuildUploadJob(ArchiveJobDerived):
             "user_defined_fields": [("subdir", index["subdir"])],
             }
 
-    def _scanFile(self, path):
+    def _scanFile(self, path: str) -> Dict[str, Any]:
         # XXX cjwatson 2022-06-10: We should probably start splitting this
         # up some more.
         name = os.path.basename(path)
@@ -375,26 +396,23 @@ class CIBuildUploadJob(ArchiveJobDerived):
         else:
             return None
 
-    def run(self):
-        """See `IRunnableJob`."""
-        build_target = self.ci_build.git_repository.target
-        if not IDistributionSourcePackage.providedBy(build_target):
-            # This should be caught by `Archive.uploadCIBuild`, but check it
-            # here as well just in case.
-            logger.warning(
-                "Source CI build is for %s, which is not a package",
-                repr(build_target))
-            return
+    def _scanArtifacts(
+        self, artifacts: Iterable[IRevisionStatusArtifact]
+    ) -> List[ScannedArtifact]:
+        """Scan an iterable of `RevisionStatusArtifact`s for metadata.
+
+        Skips log artifacts, artifacts we don't understand, and artifacts
+        not relevant to the target archive's repository format.
+
+        Returns a list of `ScannedArtifact`s containing metadata for
+        relevant artifacts.
+        """
+        allowed_binary_formats = (
+            self.binary_format_by_repository_format.get(
+                self.archive.repository_format, set()))
+        scanned = []
         with tempfile.TemporaryDirectory(prefix="ci-build-copy-job") as tmpdir:
-            releases = {
-                (release.binarypackagename, release.binpackageformat): release
-                for release in self.ci_build.binarypackages}
-            allowed_binary_formats = (
-                self.binary_format_by_repository_format.get(
-                    self.archive.repository_format, set()))
-            binaries = {}
-            for artifact in getUtility(
-                    IRevisionStatusArtifactSet).findByCIBuild(self.ci_build):
+            for artifact in artifacts:
                 if artifact.artifact_type == RevisionStatusArtifactType.LOG:
                     continue
                 name = artifact.library_file.filename
@@ -403,37 +421,65 @@ class CIBuildUploadJob(ArchiveJobDerived):
                 copy_and_close(artifact.library_file, open(contents, "wb"))
                 metadata = self._scanFile(contents)
                 if metadata is None:
-                    logger.info("No upload handler for %s" % name)
+                    logger.info("No upload handler for %s", name)
                     continue
-                binpackageformat = metadata["binpackageformat"]
-                if binpackageformat not in allowed_binary_formats:
+                if metadata["binpackageformat"] not in allowed_binary_formats:
                     logger.info(
-                        "Skipping %s (not relevant to %s archives)" % (
-                            name, self.archive.repository_format))
+                        "Skipping %s (not relevant to %s archives)",
+                        name, self.archive.repository_format)
                     continue
-                logger.info(
-                    "Uploading %s to %s %s (%s)" % (
-                        name, self.archive.reference,
-                        self.target_distroseries.getSuite(self.target_pocket),
-                        self.target_channel))
-                metadata["binarypackagename"] = bpn = (
-                    getUtility(IBinaryPackageNameSet).ensure(metadata["name"]))
-                del metadata["name"]
-                filetype = self.filetype_by_format[binpackageformat]
-                bpr = releases.get((bpn, binpackageformat))
-                if bpr is None:
-                    bpr = self.ci_build.createBinaryPackageRelease(**metadata)
-                for bpf in bpr.files:
-                    if (bpf.libraryfile == artifact.library_file and
-                            bpf.filetype == filetype):
-                        break
-                else:
-                    bpr.addFile(artifact.library_file, filetype=filetype)
-                # The publishBinaries interface was designed for .debs,
-                # which need extra per-binary "override" information
-                # (component, etc.).  None of this is relevant here.
-                binaries[bpr] = (None, None, None, None)
-            if binaries:
-                getUtility(IPublishingSet).publishBinaries(
-                    self.archive, self.target_distroseries, self.target_pocket,
-                    binaries, channel=self.target_channel)
+                scanned.append(
+                    ScannedArtifact(artifact=artifact, metadata=metadata))
+        return scanned
+
+    def _uploadBinaries(self, scanned: Iterable[ScannedArtifact]) -> None:
+        """Upload an iterable of `ScannedArtifact`s to an archive."""
+        releases = {
+            (release.binarypackagename, release.binpackageformat): release
+            for release in self.ci_build.binarypackages}
+        binaries = OrderedDict()
+        for scanned_artifact in scanned:
+            library_file = scanned_artifact.artifact.library_file
+            metadata = dict(scanned_artifact.metadata)
+            binpackageformat = metadata["binpackageformat"]
+            logger.info(
+                "Uploading %s to %s %s (%s)",
+                library_file.filename, self.archive.reference,
+                self.target_distroseries.getSuite(self.target_pocket),
+                self.target_channel)
+            metadata["binarypackagename"] = bpn = (
+                getUtility(IBinaryPackageNameSet).ensure(metadata["name"]))
+            del metadata["name"]
+            filetype = self.filetype_by_format[binpackageformat]
+            bpr = releases.get((bpn, binpackageformat))
+            if bpr is None:
+                bpr = self.ci_build.createBinaryPackageRelease(**metadata)
+            for bpf in bpr.files:
+                if (bpf.libraryfile == library_file and
+                        bpf.filetype == filetype):
+                    break
+            else:
+                bpr.addFile(library_file, filetype=filetype)
+            # The publishBinaries interface was designed for .debs,
+            # which need extra per-binary "override" information
+            # (component, etc.).  None of this is relevant here.
+            binaries[bpr] = (None, None, None, None)
+        getUtility(IPublishingSet).publishBinaries(
+            self.archive, self.target_distroseries, self.target_pocket,
+            binaries, channel=self.target_channel)
+
+    def run(self) -> None:
+        """See `IRunnableJob`."""
+        build_target = self.ci_build.git_repository.target
+        if not IDistributionSourcePackage.providedBy(build_target):
+            # This should be caught by `Archive.uploadCIBuild`, but check it
+            # here as well just in case.
+            logger.warning(
+                "Source CI build is for %s, which is not a package",
+                repr(build_target))
+            return
+        artifacts = getUtility(IRevisionStatusArtifactSet).findByCIBuild(
+            self.ci_build)
+        scanned = self._scanArtifacts(artifacts)
+        if scanned:
+            self._uploadBinaries(scanned)