Merge ~twom/launchpad:oci-remove-unnecessary-complete into launchpad:master

[Tom Wardill]

Remove first complete from OCIRegistryUploadJob

This complete appears to be masking failures, and is causing unnecessary OOPS on all RegistryUpload jobs as it causes an invalid transition of Complete -> Complete.

Without managing the transaction, this also appears to be unnecessary?

[Thiago F. Pappacena (pappacena)]

LGTM

review: Approve

diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index 1e4a7e8..20337c1 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -351,15 +351,6 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
                     self.build_uploaded = True
 
                 self.uploadManifestList(client)
-                # Force this job status to COMPLETED in the same transaction we
-                # called `getUploadedBuilds` (in the uploadManifestList call
-                # above) to release the lock already including our new status.
-                # This way, any other transaction that was blocked waiting to
-                # get the info about the upload jobs will immediately have the
-                # new status of this job, avoiding race conditions. Keep the
-                # `manage_transaction=False` to prevent the method from
-                # commiting at the wrong moment.
-                self.complete(manage_transaction=False)
 
             except OCIRegistryError as e:
                 self.error_summary = str(e)
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index 59554e6..478eb6a 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -566,6 +566,26 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory, MultiArchRecipeMixin,
         self.assertWebhookDeliveries(
             ocibuild, ["Pending", "Failed to upload"], logger)
 
+    def test_run_does_not_oops(self):
+        # The job can OOPS, but it is hidden by our exception handling
+        # Check that it's actually empty
+        build_request = self.makeBuildRequest(include_i386=False)
+        recipe = build_request.recipe
+
+        self.assertEqual(1, build_request.builds.count())
+        ocibuild = build_request.builds[0]
+        ocibuild.updateStatus(BuildStatus.FULLYBUILT)
+        self.makeWebhook(recipe)
+
+        self.assertContentEqual([], ocibuild.registry_upload_jobs)
+        job = OCIRegistryUploadJob.create(ocibuild)
+        client = FakeRegistryClient()
+        self.useFixture(ZopeUtilityFixture(client, IOCIRegistryClient))
+        with dbuser(config.IOCIRegistryUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+
+        self.assertEqual(0, len(self.oopses))
+
 
 class TestOCIRegistryUploadJobViaCelery(TestCaseWithFactory,
                                         MultiArchRecipeMixin):