launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/delete-builds into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #665301 SourcePackageRecipe.destroySelf does not clean up builds properly
  https://bugs.launchpad.net/bugs/665301


= Summary =
Fix bug# 665301: SourcePackageRecipe.destroySelf does not clean up builds
properly.

== Proposed fix ==
Implement PackageBuild.destroySelf, and have
SourcePackageRecipeBuild.destroySelf call it.

== Pre-implementation notes ==
None

== Implementation details ==
Can't think of anything

== Tests ==
bin/test -t test_destroySelf_destroys_referenced -t
test_destroySelf_removes_BuildFarmJob

== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/tests/test_sourcepackagerecipebuild.py
  lib/lp/buildmaster/tests/test_packagebuild.py
  database/schema/security.cfg
  lib/lp/buildmaster/model/packagebuild.py
  lib/lp/code/model/sourcepackagerecipebuild.py

./database/schema/security.cfg
     702: Line exceeds 78 characters.
     703: Line exceeds 78 characters.
     704: Line exceeds 78 characters.
     730: Line exceeds 78 characters.
     734: Line exceeds 78 characters.
     789: Line exceeds 78 characters.
     802: Line exceeds 78 characters.
     803: Line exceeds 78 characters.
     820: Line exceeds 78 characters.
     821: Line exceeds 78 characters.
     822: Line exceeds 78 characters.
     823: Line exceeds 78 characters.
     824: Line exceeds 78 characters.
     876: Line exceeds 78 characters.
     877: Line exceeds 78 characters.
     878: Line exceeds 78 characters.
     908: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~abentley/launchpad/delete-builds/+merge/39184
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/delete-builds into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-10-22 20:30:20 +0000
+++ database/schema/security.cfg	2010-10-22 20:56:28 +0000
@@ -124,6 +124,7 @@
 public.bugtrackercomponent              = SELECT, INSERT, UPDATE
 public.bugtrackercomponentgroup         = SELECT, INSERT, UPDATE
 public.bugwatchactivity                 = SELECT, INSERT, UPDATE
+public.buildfarmjob                     = DELETE
 public.codeimport                       = SELECT, INSERT, UPDATE, DELETE
 public.codeimportevent                  = SELECT, INSERT, UPDATE
 public.codeimporteventdata              = SELECT, INSERT
@@ -207,6 +208,7 @@
 public.openidrpconfig                   = SELECT, INSERT, UPDATE, DELETE
 public.packagebugsupervisor             = SELECT, INSERT, UPDATE, DELETE
 public.packagecopyrequest               = SELECT, INSERT, UPDATE
+public.packagebuild                     = DELETE
 public.packagediff                      = SELECT, INSERT, UPDATE, DELETE
 public.packagediff                      = SELECT, INSERT, UPDATE, DELETE
 public.packageset                       = SELECT, INSERT, UPDATE, DELETE

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-10-02 11:41:43 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-10-22 20:56:28 +0000
@@ -124,6 +124,12 @@
         store.add(package_build)
         return package_build
 
+    def destroySelf(self):
+        build_farm_job = self.build_farm_job
+        store = Store.of(self)
+        store.remove(self)
+        store.remove(build_farm_job)
+
     @property
     def current_component(self):
         """See `IPackageBuild`."""

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2010-10-02 11:41:43 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2010-10-22 20:56:28 +0000
@@ -34,6 +34,7 @@
     IPackageBuildSet,
     IPackageBuildSource,
     )
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.registry.interfaces.pocket import (
@@ -202,6 +203,19 @@
             self.package_build.id, self.package_build.build_farm_job.id)
         self.assertEquals(expected_cookie, cookie)
 
+    def test_destroySelf_removes_BuildFarmJob(self):
+        # Destroying a packagebuild also destroys the BuildFarmJob it
+        # references.
+        naked_build = removeSecurityProxy(self.package_build)
+        store = Store.of(self.package_build)
+        # Ensure build_farm_job_id is set.
+        store.flush()
+        build_farm_job_id = naked_build.build_farm_job_id
+        naked_build.destroySelf()
+        result = store.find(
+            BuildFarmJob, BuildFarmJob.id == build_farm_job_id)
+        self.assertIs(None, result.one())
+
 
 class TestPackageBuildSet(TestPackageBuildBase):
 

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-10-06 11:46:51 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-10-22 20:56:28 +0000
@@ -228,7 +228,9 @@
             SourcePackageRelease.source_package_recipe_build == self.id)
         for release in releases:
             release.source_package_recipe_build = None
+        package_build = self.package_build
         store.remove(self)
+        package_build.destroySelf()
 
     @classmethod
     def getById(cls, build_id):

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-10-06 11:46:51 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-10-22 20:56:28 +0000
@@ -26,6 +26,8 @@
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
+from lp.buildmaster.model.buildfarmjob import BuildFarmJob
+from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.buildmaster.tests.test_packagebuild import (
     TestGetUploadMethodsMixin,
@@ -302,6 +304,23 @@
         self.assertIs(None, release.source_package_recipe_build)
         transaction.commit()
 
+    def test_destroySelf_destroys_referenced(self):
+        # Destroying a sourcepackagerecipebuild also destroys the
+        # PackageBuild and BuildFarmJob it references.
+        build = self.factory.makeSourcePackageRecipeBuild()
+        store = Store.of(build)
+        naked_build = removeSecurityProxy(build)
+        # Ensure database ids are set.
+        store.flush()
+        package_build_id = naked_build.package_build_id
+        build_farm_job_id = naked_build.package_build.build_farm_job_id
+        build.destroySelf()
+        result = store.find(PackageBuild, PackageBuild.id == package_build_id)
+        self.assertIs(None, result.one())
+        result = store.find(
+            BuildFarmJob, BuildFarmJob.id == build_farm_job_id)
+        self.assertIs(None, result.one())
+
     def test_cancelBuild(self):
         # ISourcePackageRecipeBuild should make sure to remove jobs and build
         # queue entries and then invalidate itself.