launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:oci-fix-job-events into launchpad:master.

Commit message:
Fix event notifications in OCIRegistryUploadJob.create

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This modifies a build's registry upload status, rather than creating a build, so issue the correct event notifications that correspond to that.  This affects which webhook deliveries are made.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-fix-job-events into launchpad:master.

diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index bd34c19..fc84c17 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -17,7 +17,6 @@ from lazr.enum import (
     DBEnumeratedType,
     DBItem,
     )
-from lazr.lifecycle.event import ObjectCreatedEvent
 import six
 from storm.databases.postgres import JSON
 from storm.locals import (
@@ -26,7 +25,6 @@ from storm.locals import (
     )
 import transaction
 from zope.component import getUtility
-from zope.event import notify
 from zope.interface import (
     implementer,
     provider,
@@ -161,12 +159,16 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
     @classmethod
     def create(cls, build):
         """See `IOCIRegistryUploadJobSource`"""
-        oci_build_job = OCIRecipeBuildJob(
-            build, cls.class_job_type, {})
-        job = cls(oci_build_job)
-        job.celeryRunOnCommit()
-        del get_property_cache(build).last_registry_upload_job
-        notify(ObjectCreatedEvent(build))
+        edited_fields = set()
+        with notify_modified(build, edited_fields) as before_modification:
+            oci_build_job = OCIRecipeBuildJob(
+                build, cls.class_job_type, {})
+            job = cls(oci_build_job)
+            job.celeryRunOnCommit()
+            del get_property_cache(build).last_registry_upload_job
+            upload_status = build.registry_upload_status
+            if upload_status != before_modification.registry_upload_status:
+                edited_fields.add("registry_upload_status")
         return job
 
     # Ideally we'd just override Job._set_status or similar, but
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index c0926a4..ef3af8a 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -131,15 +131,6 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         expected_payloads = [{
             "recipe_build": Equals(
                 canonical_url(ocibuild, force_local_path=True)),
-            "action": Equals("created"),
-            "recipe": Equals(
-                canonical_url(ocibuild.recipe, force_local_path=True)),
-            "build_request": Is(None),
-            "status": Equals("Successfully built"),
-            "registry_upload_status": Equals("Pending")}]
-        expected_payloads += [{
-            "recipe_build": Equals(
-                canonical_url(ocibuild, force_local_path=True)),
             "action": Equals("status-changed"),
             "recipe": Equals(
                 canonical_url(ocibuild.recipe, force_local_path=True)),
@@ -184,8 +175,7 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         self.assertContentEqual([job], ocibuild.registry_upload_jobs)
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
-        self.assertWebhookDeliveries(
-            ocibuild, ["Uploaded"], logger)
+        self.assertWebhookDeliveries(ocibuild, ["Pending", "Uploaded"], logger)
 
     def test_run_failed(self):
         # A failed run sets the registry upload status to FAILED.
@@ -203,4 +193,4 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         self.assertEqual("An upload failure", job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
-            ocibuild, ["Failed to upload"], logger)
+            ocibuild, ["Pending", "Failed to upload"], logger)