launchpad-reviewers team mailing list archive

[Quentin Debhi <mp+474261@xxxxxxxxxxxxxxxxxx>]

Quentin Debhi has proposed merging ~ruinedyourlife/launchpad:delete-craft-recipe-builds-and-jobs-when-deleting-recipes into launchpad:master with ~ruinedyourlife/launchpad:implement-craft-recipe-requests-builds-job as a prerequisite.

Commit message:
Delete craft recipe and jobs when deleting recipes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/474261
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:delete-craft-recipe-builds-and-jobs-when-deleting-recipes into launchpad:master.

diff --git a/lib/lp/crafts/interfaces/craftrecipe.py b/lib/lp/crafts/interfaces/craftrecipe.py
index 34090aa..0b9b69b 100644
--- a/lib/lp/crafts/interfaces/craftrecipe.py
+++ b/lib/lp/crafts/interfaces/craftrecipe.py
@@ -577,6 +577,9 @@ class ICraftRecipeSet(Interface):
     def getByName(owner, project, name):
         """Returns the appropriate `ICraftRecipe` for the given objects."""
 
+    def exists(owner, project, name):
+        """Check to see if a matching craft recipe exists."""
+
     def isValidInformationType(information_type, owner, git_ref=None):
         """Whether the information type context is valid."""
 
diff --git a/lib/lp/crafts/model/craftrecipe.py b/lib/lp/crafts/model/craftrecipe.py
index 1deff6b..a0af625 100644
--- a/lib/lp/crafts/model/craftrecipe.py
+++ b/lib/lp/crafts/model/craftrecipe.py
@@ -14,12 +14,14 @@ import yaml
 from lazr.lifecycle.event import ObjectCreatedEvent
 from storm.databases.postgres import JSON
 from storm.locals import (
+    And,
     Bool,
     DateTime,
     Int,
     Join,
     Or,
     Reference,
+    Select,
     Store,
     Unicode,
 )
@@ -34,6 +36,8 @@ from lp.app.enums import (
     InformationType,
 )
 from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.errors import GitRepositoryBlobNotFound, GitRepositoryScanFault
 from lp.code.model.gitcollection import GenericGitCollection
 from lp.code.model.gitrepository import GitRepository
@@ -57,12 +61,14 @@ from lp.crafts.interfaces.craftrecipe import (
     ICraftRecipeSet,
     MissingSourcecraftYaml,
     NoSourceForCraftRecipe,
+    NoSuchCraftRecipe,
 )
 from lp.crafts.interfaces.craftrecipebuild import ICraftRecipeBuildSet
 from lp.crafts.interfaces.craftrecipejob import (
     ICraftRecipeRequestBuildsJobSource,
 )
 from lp.crafts.model.craftrecipebuild import CraftRecipeBuild
+from lp.crafts.model.craftrecipejob import CraftRecipeJob
 from lp.registry.errors import PrivatePersonLinkageError
 from lp.registry.interfaces.person import IPersonSet, validate_public_person
 from lp.registry.model.distribution import Distribution
@@ -76,6 +82,7 @@ from lp.services.database.interfaces import IPrimaryStore, IStore
 from lp.services.database.stormbase import StormBase
 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
 from lp.services.propertycache import cachedproperty, get_property_cache
 from lp.soyuz.model.distroarchseries import DistroArchSeries, PocketChroot
@@ -324,7 +331,46 @@ class CraftRecipe(StormBase):
 
     def destroySelf(self):
         """See `ICraftRecipe`."""
-        IStore(CraftRecipe).remove(self)
+        store = IStore(self)
+        # Remove build jobs. There won't be many queued builds, so we can
+        # afford to do this the safe but slow way via BuildQueue.destroySelf
+        # rather than in bulk.
+        buildqueue_records = store.find(
+            BuildQueue,
+            BuildQueue._build_farm_job_id
+            == CraftRecipeBuild.build_farm_job_id,
+            CraftRecipeBuild.recipe == self,
+        )
+        for buildqueue_record in buildqueue_records:
+            buildqueue_record.destroySelf()
+        build_farm_job_ids = list(
+            store.find(
+                CraftRecipeBuild.build_farm_job_id,
+                CraftRecipeBuild.recipe == self,
+            )
+        )
+        store.execute(
+            """
+            DELETE FROM CraftFile
+            USING CraftRecipeBuild
+            WHERE
+                CraftFile.build = CraftRecipeBuild.id AND
+                CraftRecipeBuild.recipe = ?
+            """,
+            (self.id,),
+        )
+        store.find(CraftRecipeBuild, CraftRecipeBuild.recipe == self).remove()
+        affected_jobs = Select(
+            [CraftRecipeJob.job_id],
+            And(CraftRecipeJob.job == Job.id, CraftRecipeJob.recipe == self),
+        )
+        store.find(Job, Job.id.is_in(affected_jobs)).remove()
+        # XXX ruinedyourlife 2024-10-02: we need to remove webhooks once
+        # implemented getUtility(IWebhookSet).delete(self.webhooks)
+        store.remove(self)
+        store.find(
+            BuildFarmJob, BuildFarmJob.id.is_in(build_farm_job_ids)
+        ).remove()
 
     def _checkRequestBuild(self, requester):
         """May `requester` request builds of this craft recipe?"""
@@ -511,7 +557,7 @@ class CraftRecipeSet:
 
         if git_ref is None:
             raise NoSourceForCraftRecipe
-        if self.getByName(owner, project, name) is not None:
+        if self.exists(owner, project, name):
             raise DuplicateCraftRecipeName
 
         # The relevant validators will do their own checks as well, but we
@@ -544,14 +590,24 @@ class CraftRecipeSet:
 
         return recipe
 
-    def getByName(self, owner, project, name):
-        """See `ICraftRecipeSet`."""
+    def _getByName(self, owner, project, name):
         return (
             IStore(CraftRecipe)
             .find(CraftRecipe, owner=owner, project=project, name=name)
             .one()
         )
 
+    def exists(self, owner, project, name):
+        """See `ICraftRecipeSet."""
+        return self._getByName(owner, project, name) is not None
+
+    def getByName(self, owner, project, name):
+        """See `ICraftRecipeSet`."""
+        recipe = self._getByName(owner, project, name)
+        if recipe is None:
+            raise NoSuchCraftRecipe(name)
+        return recipe
+
     def isValidInformationType(self, information_type, owner, git_ref=None):
         """See `ICraftRecipeSet`."""
         private = information_type not in PUBLIC_INFORMATION_TYPES
diff --git a/lib/lp/crafts/tests/test_craftrecipe.py b/lib/lp/crafts/tests/test_craftrecipe.py
index 47a44f6..afd74b3 100644
--- a/lib/lp/crafts/tests/test_craftrecipe.py
+++ b/lib/lp/crafts/tests/test_craftrecipe.py
@@ -25,6 +25,7 @@ from lp.buildmaster.interfaces.processor import (
     IProcessorSet,
     ProcessorNotFound,
 )
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.code.tests.helpers import GitHostingFixture
 from lp.crafts.interfaces.craftrecipe import (
@@ -38,18 +39,33 @@ from lp.crafts.interfaces.craftrecipe import (
     ICraftRecipeSet,
     NoSourceForCraftRecipe,
 )
-from lp.crafts.interfaces.craftrecipebuild import ICraftRecipeBuild
+from lp.crafts.interfaces.craftrecipebuild import (
+    ICraftRecipeBuild,
+    ICraftRecipeBuildSet,
+)
 from lp.crafts.interfaces.craftrecipejob import (
     ICraftRecipeRequestBuildsJobSource,
 )
+from lp.crafts.model.craftrecipebuild import CraftFile
+from lp.crafts.model.craftrecipejob import CraftRecipeJob
+from lp.services.config import config
 from lp.services.database.constants import ONE_DAY_AGO, UTC_NOW
 from lp.services.database.interfaces import IStore
-from lp.services.database.sqlbase import get_transaction_timestamp
+from lp.services.database.sqlbase import (
+    flush_database_caches,
+    get_transaction_timestamp,
+)
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.runner import JobRunner
 from lp.services.webapp.snapshot import notify_modified
 from lp.testing import TestCaseWithFactory, admin_logged_in, person_logged_in
-from lp.testing.layers import DatabaseFunctionalLayer, LaunchpadZopelessLayer
+from lp.testing.dbuser import dbuser
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    LaunchpadZopelessLayer,
+)
 
 
 class TestCraftRecipeFeatureFlags(TestCaseWithFactory):
@@ -118,13 +134,13 @@ class TestCraftRecipe(TestCaseWithFactory):
         recipe = self.factory.makeCraftRecipe(
             registrant=owner, owner=owner, project=project, name="condemned"
         )
-        self.assertIsNotNone(
-            getUtility(ICraftRecipeSet).getByName(owner, project, "condemned")
+        self.assertTrue(
+            getUtility(ICraftRecipeSet).exists(owner, project, "condemned")
         )
         with person_logged_in(recipe.owner):
             recipe.destroySelf()
-        self.assertIsNone(
-            getUtility(ICraftRecipeSet).getByName(owner, project, "condemned")
+        self.assertFalse(
+            getUtility(ICraftRecipeSet).exists(owner, project, "condemned")
         )
 
     def makeBuildableDistroArchSeries(
@@ -444,13 +460,15 @@ class TestCraftRecipe(TestCaseWithFactory):
                     """\
             base: ubuntu@20.04
             platforms:
-                sparc:
-                avr:
+                amd64:
+                armhf:
             """
                 )
             )
         )
-        job = self.makeRequestBuildsJob("20.04", ["sparc", "avr", "mips64el"])
+        job = self.makeRequestBuildsJob(
+            "20.04", ["amd64", "armhf", "mips64el"]
+        )
         self.assertEqual(
             get_transaction_timestamp(IStore(job.recipe)), job.date_created
         )
@@ -460,7 +478,7 @@ class TestCraftRecipe(TestCaseWithFactory):
                 job.build_request, channels=removeSecurityProxy(job.channels)
             )
         self.assertRequestedBuildsMatch(
-            builds, job, "20.04", ["sparc", "avr"], job.channels
+            builds, job, "20.04", ["amd64", "armhf"], job.channels
         )
 
     def test_requestBuildsFromJob_architectures_parameter(self):
@@ -473,16 +491,14 @@ class TestCraftRecipe(TestCaseWithFactory):
                     """\
             base: ubuntu@20.04
             platforms:
-                sparc:
-                i386:
-                avr:
+                armhf:
                 riscv64:
             """
                 )
             )
         )
         job = self.makeRequestBuildsJob(
-            "20.04", ["avr", "mips64el", "riscv64"]
+            "20.04", ["armhf", "mips64el", "riscv64"]
         )
         self.assertEqual(
             get_transaction_timestamp(IStore(job.recipe)), job.date_created
@@ -492,10 +508,10 @@ class TestCraftRecipe(TestCaseWithFactory):
             builds = job.recipe.requestBuildsFromJob(
                 job.build_request,
                 channels=removeSecurityProxy(job.channels),
-                architectures={"avr", "riscv64"},
+                architectures={"armhf", "riscv64"},
             )
         self.assertRequestedBuildsMatch(
-            builds, job, "20.04", ["avr", "riscv64"], job.channels
+            builds, job, "20.04", ["armhf", "riscv64"], job.channels
         )
 
 
@@ -675,3 +691,106 @@ class TestCraftRecipeSet(TestCaseWithFactory):
             self.assertSqlAttributeEqualsDate(
                 recipe, "date_last_modified", UTC_NOW
             )
+
+
+class TestCraftRecipeDeleteWithBuilds(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super().setUp()
+        self.useFixture(FeatureFixture({CRAFT_RECIPE_ALLOW_CREATE: "on"}))
+
+    def test_delete_with_builds(self):
+        # A craft recipe with build requests and builds can be deleted.
+        # Doing so deletes all its build requests, their builds, and their
+        # files.
+        owner = self.factory.makePerson()
+        project = self.factory.makeProduct()
+        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
+            )
+        )
+        sourcecraft_yaml = (
+            dedent(
+                """\
+            base: %s@%s
+            platforms:
+                %s:
+            """
+            )
+            % (
+                distroseries.distribution.name,
+                distroseries.version,
+                processor.name,
+            )
+        )
+        self.useFixture(GitHostingFixture(blob=sourcecraft_yaml))
+        [git_ref] = self.factory.makeGitRefs()
+        condemned_recipe = self.factory.makeCraftRecipe(
+            registrant=owner,
+            owner=owner,
+            project=project,
+            name="condemned",
+            git_ref=git_ref,
+        )
+        other_recipe = self.factory.makeCraftRecipe(
+            registrant=owner, owner=owner, project=project, git_ref=git_ref
+        )
+        self.assertTrue(
+            getUtility(ICraftRecipeSet).exists(owner, project, "condemned")
+        )
+        with person_logged_in(owner):
+            requests = []
+            jobs = []
+            for recipe in (condemned_recipe, other_recipe):
+                requests.append(recipe.requestBuilds(owner))
+                jobs.append(removeSecurityProxy(requests[-1])._job)
+            with dbuser(config.ICraftRecipeRequestBuildsJobSource.dbuser):
+                JobRunner(jobs).runAll()
+            for job in jobs:
+                self.assertEqual(JobStatus.COMPLETED, job.job.status)
+            [build] = requests[0].builds
+            [other_build] = requests[1].builds
+            craft_file = self.factory.makeCraftFile(build=build)
+            other_craft_file = self.factory.makeCraftFile(build=other_build)
+        store = Store.of(condemned_recipe)
+        store.flush()
+        job_ids = [job.job_id for job in jobs]
+        build_id = build.id
+        build_queue_id = build.buildqueue_record.id
+        build_farm_job_id = removeSecurityProxy(build).build_farm_job_id
+        craft_file_id = removeSecurityProxy(craft_file).id
+        with person_logged_in(condemned_recipe.owner):
+            condemned_recipe.destroySelf()
+        flush_database_caches()
+        # The deleted recipe, its build requests, and its are gone.
+        self.assertFalse(
+            getUtility(ICraftRecipeSet).exists(owner, project, "condemned")
+        )
+        self.assertIsNone(store.get(CraftRecipeJob, job_ids[0]))
+        self.assertIsNone(getUtility(ICraftRecipeBuildSet).getByID(build_id))
+        self.assertIsNone(store.get(BuildQueue, build_queue_id))
+        self.assertIsNone(store.get(BuildFarmJob, build_farm_job_id))
+        self.assertIsNone(store.get(CraftFile, craft_file_id))
+        # Unrelated build requests, build jobs and builds are still present.
+        self.assertEqual(
+            removeSecurityProxy(jobs[1]).context,
+            store.get(CraftRecipeJob, job_ids[1]),
+        )
+        self.assertEqual(
+            other_build,
+            getUtility(ICraftRecipeBuildSet).getByID(other_build.id),
+        )
+        self.assertIsNotNone(other_build.buildqueue_record)
+        self.assertIsNotNone(
+            store.get(CraftFile, removeSecurityProxy(other_craft_file).id)
+        )