launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-delete-with-build-requests into lp:launchpad.

Commit message:
Delete associated build requests when deleting a snap.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1802089 in Launchpad itself: "OOPS when deleting snap build with snap jobs"
  https://bugs.launchpad.net/launchpad/+bug/1802089

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-delete-with-build-requests/+merge/358440
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-delete-with-build-requests into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2018-10-09 09:25:19 +0000
+++ lib/lp/snappy/model/snap.py	2018-11-07 15:02:24 +0000
@@ -50,10 +50,7 @@
     DateTimeFormatterAPI,
     )
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
-from lp.app.errors import (
-    IncompatibleArguments,
-    NotFoundError,
-    )
+from lp.app.errors import IncompatibleArguments
 from lp.app.interfaces.security import IAuthorization
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
@@ -117,6 +114,7 @@
     )
 from lp.services.features import getFeatureFlag
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.model.job import Job
 from lp.services.librarian.model import (
     LibraryFileAlias,
     LibraryFileContent,
@@ -162,6 +160,7 @@
 from lp.snappy.interfaces.snappyseries import ISnappyDistroSeriesSet
 from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
 from lp.snappy.model.snapbuild import SnapBuild
+from lp.snappy.model.snapjob import SnapJob
 from lp.soyuz.interfaces.archive import ArchiveDisabled
 from lp.soyuz.model.archive import (
     Archive,
@@ -879,6 +878,9 @@
                 SnapBuild.snap = ?
             """, (self.id,))
         store.find(SnapBuild, SnapBuild.snap == self).remove()
+        affected_jobs = Select(
+            [SnapJob.job_id], And(SnapJob.job == Job.id, SnapJob.snap == self))
+        store.find(Job, Job.id.is_in(affected_jobs)).remove()
         getUtility(IWebhookSet).delete(self.webhooks)
         store.remove(self)
         store.find(

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2018-10-09 09:25:19 +0000
+++ lib/lp/snappy/tests/test_snap.py	2018-11-07 15:02:24 +0000
@@ -111,6 +111,7 @@
 from lp.snappy.model.snap import SnapSet
 from lp.snappy.model.snapbuild import SnapFile
 from lp.snappy.model.snapbuildjob import SnapBuildJob
+from lp.snappy.model.snapjob import SnapJob
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -961,6 +962,65 @@
             other_build, getUtility(ISnapBuildSet).getByID(other_build.id))
         self.assertIsNotNone(other_build.buildqueue_record)
 
+    def test_delete_with_build_requests(self):
+        # A snap package with build requests can be deleted.
+        owner = self.factory.makePerson()
+        distroseries = self.factory.makeDistroSeries()
+        processor = self.factory.makeProcessor(supports_virtualized=True)
+        das = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag=processor.name,
+            processor=processor)
+        das.addOrUpdateChroot(
+            self.factory.makeLibraryFileAlias(
+                filename="fake_chroot.tar.gz", db_only=True))
+        self.useFixture(GitHostingFixture(blob=dedent("""\
+            architectures:
+              - build-on: %s
+            """ % processor.name)))
+        [git_ref] = self.factory.makeGitRefs()
+        condemned_snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, distroseries=distroseries,
+            name="condemned", git_ref=git_ref)
+        other_snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, distroseries=distroseries,
+            git_ref=git_ref)
+        self.assertTrue(getUtility(ISnapSet).exists(owner, "condemned"))
+        with person_logged_in(owner):
+            requests = []
+            jobs = []
+            for snap in (condemned_snap, other_snap):
+                requests.append(snap.requestBuilds(
+                    owner, distroseries.main_archive,
+                    PackagePublishingPocket.UPDATES))
+                jobs.append(removeSecurityProxy(requests[-1])._job)
+            with dbuser(config.ISnapRequestBuildsJobSource.dbuser):
+                JobRunner(jobs).runAll()
+            for job in jobs:
+                self.assertEqual(JobStatus.COMPLETED, job.job.status)
+            for request in requests:
+                self.assertNotEqual([], request.builds)
+        store = Store.of(condemned_snap)
+        store.flush()
+        job_ids = [job.job_id for job in jobs]
+        build_ids = [
+            [build.id for build in request.builds] for request in requests]
+        with person_logged_in(condemned_snap.owner):
+            condemned_snap.destroySelf()
+        flush_database_caches()
+        # The deleted snap, its build requests, and its builds are gone.
+        self.assertFalse(getUtility(ISnapSet).exists(owner, "condemned"))
+        self.assertIsNone(store.get(SnapJob, job_ids[0]))
+        for build_id in build_ids[0]:
+            self.assertIsNone(getUtility(ISnapBuildSet).getByID(build_id))
+        # Unrelated build requests and builds are still present.
+        self.assertEqual(
+            removeSecurityProxy(jobs[1]).context,
+            store.get(SnapJob, job_ids[1]))
+        other_builds = [
+            getUtility(ISnapBuildSet).getByID(build_id)
+            for build_id in build_ids[1]]
+        self.assertEqual(list(requests[1].builds), other_builds)
+
     def test_related_webhooks_deleted(self):
         owner = self.factory.makePerson()
         snap = self.factory.makeSnap(registrant=owner, owner=owner)