launchpad-reviewers team mailing list archive

[Colin Watson <mp+420012@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-cibuild-verifySuccessfulUpload into launchpad:master.

Commit message:
Implement CIBuild.verifySuccessfulUpload properly

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

The upload processor expects this method to return a boolean, and if it returns None then that's treated as false and causes the upload to fail.

Since this was fundamentally a type confusion, I added type annotations to all the occurrences of this method.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-cibuild-verifySuccessfulUpload into launchpad:master.

diff --git a/lib/lp/buildmaster/interfaces/packagebuild.py b/lib/lp/buildmaster/interfaces/packagebuild.py
index 3af81f7..0ff6c1e 100644
--- a/lib/lp/buildmaster/interfaces/packagebuild.py
+++ b/lib/lp/buildmaster/interfaces/packagebuild.py
@@ -73,7 +73,7 @@ class IPackageBuildView(IBuildFarmJobView):
             title=_("Distribution series"), required=True,
             description=_("Shortcut for its distribution series.")))
 
-    def verifySuccessfulUpload():
+    def verifySuccessfulUpload() -> bool:
         """Verify that the upload of this build completed successfully."""
 
     def storeUploadLog(content):
diff --git a/lib/lp/buildmaster/model/packagebuild.py b/lib/lp/buildmaster/model/packagebuild.py
index 1649d14..7c550c5 100644
--- a/lib/lp/buildmaster/model/packagebuild.py
+++ b/lib/lp/buildmaster/model/packagebuild.py
@@ -62,7 +62,7 @@ class PackageBuildMixin(BuildFarmJobMixin):
         else:
             self.dependencies = None
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         raise NotImplementedError
 
diff --git a/lib/lp/charms/model/charmrecipebuild.py b/lib/lp/charms/model/charmrecipebuild.py
index 5b5a1e6..3e39c63 100644
--- a/lib/lp/charms/model/charmrecipebuild.py
+++ b/lib/lp/charms/model/charmrecipebuild.py
@@ -385,7 +385,7 @@ class CharmRecipeBuild(PackageBuildMixin, StormBase):
         IMasterStore(CharmFile).add(charm_file)
         return charm_file
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         return not self.getFiles().is_empty()
 
diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 554ada2..a13bfc2 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -435,9 +435,10 @@ class CIBuild(PackageBuildMixin, StormBase):
 
         raise NotFoundError(filename)
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         # We have no interesting checks to perform here.
+        return True
 
     def notify(self, extra_info=None):
         """See `IPackageBuild`."""
diff --git a/lib/lp/code/model/sourcepackagerecipebuild.py b/lib/lp/code/model/sourcepackagerecipebuild.py
index 273d606..e4720cd 100644
--- a/lib/lp/code/model/sourcepackagerecipebuild.py
+++ b/lib/lp/code/model/sourcepackagerecipebuild.py
@@ -324,7 +324,7 @@ class SourcePackageRecipeBuild(SpecificBuildFarmJobSourceMixin,
             return median
         return timedelta(minutes=10)
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         return self.source_package_release is not None
 
     def notify(self, extra_info=None):
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index 14e12b0..5951daf 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -262,6 +262,13 @@ class TestCIBuild(TestCaseWithFactory):
         self.assertEqual(
             build.upload_log, build.getFileByName(build.upload_log.filename))
 
+    def test_verifySuccessfulUpload(self):
+        # verifySuccessfulUpload always returns True; the upload processor
+        # requires us to implement this, but we don't have any interesting
+        # checks to perform here.
+        build = self.factory.makeCIBuild()
+        self.assertTrue(build.verifySuccessfulUpload())
+
     def addFakeBuildLog(self, build):
         build.setLog(self.factory.makeLibraryFileAlias("mybuildlog.txt"))
 
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index 753bf64..4ad0f4f 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -400,7 +400,7 @@ class OCIRecipeBuild(PackageBuildMixin, StormBase):
         except NotFoundError:
             return None
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         layer_files = Store.of(self).find(
             OCIFile,
diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index 0279834..4ec5275 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -349,7 +349,7 @@ class SnapBuild(PackageBuildMixin, Storm):
         IMasterStore(SnapFile).add(snapfile)
         return snapfile
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         return not self.getFiles().is_empty()
 
diff --git a/lib/lp/soyuz/model/binarypackagebuild.py b/lib/lp/soyuz/model/binarypackagebuild.py
index 41bc650..9f51a22 100644
--- a/lib/lp/soyuz/model/binarypackagebuild.py
+++ b/lib/lp/soyuz/model/binarypackagebuild.py
@@ -691,7 +691,7 @@ class BinaryPackageBuild(PackageBuildMixin, SQLBase):
         else:
             assert self.buildinfo == buildinfo
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         return bool(self.binarypackages)
 
     def notify(self, extra_info=None):
diff --git a/lib/lp/soyuz/model/livefsbuild.py b/lib/lp/soyuz/model/livefsbuild.py
index 1ff91f5..fbb5578 100644
--- a/lib/lp/soyuz/model/livefsbuild.py
+++ b/lib/lp/soyuz/model/livefsbuild.py
@@ -292,7 +292,7 @@ class LiveFSBuild(PackageBuildMixin, Storm):
         IMasterStore(LiveFSFile).add(livefsfile)
         return livefsfile
 
-    def verifySuccessfulUpload(self):
+    def verifySuccessfulUpload(self) -> bool:
         """See `IPackageBuild`."""
         return not self.getFiles().is_empty()