launchpad-reviewers team mailing list archive

[Colin Watson <mp+419518@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-ciupload-path into launchpad:master.

Commit message:
Fix upload path handling for CI uploads

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419518

`BuildFarmJobBehaviourBase.handleSuccess` downloads files to a subdirectory of what `CIUpload` sees as `upload_path`, despite a misleading coincidence of local variable names.  Cope with this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-ciupload-path into launchpad:master.

diff --git a/lib/lp/archiveuploader/ciupload.py b/lib/lp/archiveuploader/ciupload.py
index 99a9d6d..ce5b92d 100644
--- a/lib/lp/archiveuploader/ciupload.py
+++ b/lib/lp/archiveuploader/ciupload.py
@@ -34,14 +34,24 @@ class CIUpload:
 
         # collect all artifacts
         artifacts = {}
+        # The upload path is structured as
+        # .../incoming/<BUILD_COOKIE>/<ARCHIVE_ID>/<DISTRIBUTION_NAME>.
+        # This is historical and doesn't necessarily make a lot of sense for
+        # CI builds, but we need to fit into how the rest of the build farm
+        # works.
+        upload_path = os.path.join(
+            self.upload_path, str(build.archive.id), build.distribution.name)
         # we assume first level directories are job directories
-        job_directories = [
-            d.name for d in os.scandir(self.upload_path) if d.is_dir()
-        ]
+        if os.path.isdir(upload_path):
+            job_directories = [
+                d.name for d in os.scandir(upload_path) if d.is_dir()
+            ]
+        else:
+            job_directories = []
         for job_directory in job_directories:
             artifacts[job_directory] = []
             for dirpath, _, filenames in os.walk(os.path.join(
-                self.upload_path, job_directory
+                upload_path, job_directory
             )):
                 for filename in filenames:
                     artifacts[job_directory].append(os.path.join(
@@ -52,7 +62,7 @@ class CIUpload:
             report = build.getOrCreateRevisionStatusReport(job_id)
 
             # attach log file
-            log_file = os.path.join(self.upload_path, job_id + ".log")
+            log_file = os.path.join(upload_path, job_id + ".log")
             try:
                 with open(log_file, mode="rb") as f:
                     report.setLog(f.read())
diff --git a/lib/lp/archiveuploader/tests/test_ciupload.py b/lib/lp/archiveuploader/tests/test_ciupload.py
index 853cabb..2d00fef 100644
--- a/lib/lp/archiveuploader/tests/test_ciupload.py
+++ b/lib/lp/archiveuploader/tests/test_ciupload.py
@@ -55,6 +55,25 @@ class TestCIBuildUploads(TestUploadProcessorBase):
             UploadStatusEnum.REJECTED, result
         )
 
+    def test_requires_upload_path(self):
+        removeSecurityProxy(self.build).results = {
+            'build:0': {'result': 'SUCCEEDED'},
+        }
+        os.makedirs(os.path.join(self.incoming_folder, "test"))
+        handler = UploadHandler.forProcessor(
+            self.uploadprocessor,
+            self.incoming_folder,
+            "test",
+            self.build,
+        )
+
+        result = handler.processCIResult(self.log)
+
+        # we explicitly provided no log file, which causes a rejected upload
+        self.assertEqual(
+            UploadStatusEnum.REJECTED, result
+        )
+
     def test_requires_log_file(self):
         removeSecurityProxy(self.build).results = {
             'build:0': {'result': 'SUCCEEDED'},
@@ -66,6 +85,10 @@ class TestCIBuildUploads(TestUploadProcessorBase):
             "test",
             self.build,
         )
+        upload_path = os.path.join(
+            self.incoming_folder, "test", str(self.build.archive.id),
+            self.build.distribution.name)
+        os.makedirs(upload_path)
 
         result = handler.processCIResult(self.log)
 
@@ -81,21 +104,22 @@ class TestCIBuildUploads(TestUploadProcessorBase):
                 'result': 'SUCCEEDED',
             },
         }
+        upload_path = os.path.join(
+            self.incoming_folder, "test", str(self.build.archive.id),
+            self.build.distribution.name)
 
         # create log file
-        path = os.path.join(self.incoming_folder, "test", "build:0.log")
+        path = os.path.join(upload_path, "build:0.log")
         content = "some log content"
         write_file(path, content.encode("utf-8"))
 
         # create artifact
-        path = os.path.join(
-            self.incoming_folder, "test", "build:0", "ci.whl")
+        path = os.path.join(upload_path, "build:0", "ci.whl")
         content = b"abc"
         write_file(path, content)
 
         # create artifact in a sub-directory
-        path = os.path.join(
-            self.incoming_folder, "test", "build:0", "sub", "test.whl")
+        path = os.path.join(upload_path, "build:0", "sub", "test.whl")
         content = b"abc"
         write_file(path, content)
 
@@ -128,15 +152,17 @@ class TestCIBuildUploads(TestUploadProcessorBase):
                 'result': 'SUCCEEDED',
             },
         }
+        upload_path = os.path.join(
+            self.incoming_folder, "test", str(self.build.archive.id),
+            self.build.distribution.name)
 
         # create log file
-        path = os.path.join(self.incoming_folder, "test", "build:0.log")
+        path = os.path.join(upload_path, "build:0.log")
         content = "some log content"
         write_file(path, content.encode("utf-8"))
 
         # create artifact
-        path = os.path.join(
-            self.incoming_folder, "test", "build:0", "ci.whl")
+        path = os.path.join(upload_path, "build:0", "ci.whl")
         content = b"abc"
         write_file(path, content)