Launchpad itself

Merge lp:~wgrant/launchpad/bug-549340-fix-buildqueue-destruction into lp:launchpad

  1. bug-549340-fix-buildqueue-destruction
  2. Merge into devel
Proposed by William Grant
Status: Rejected
Rejected by: Michael Nelson
Proposed branch: lp:~wgrant/launchpad/bug-549340-fix-buildqueue-destruction
Merge into: lp:launchpad
Prerequisite: lp:~wgrant/launchpad/refactor-slavestatus
Diff against target: 152 lines (+52/-2)
7 files modified
database/schema/security.cfg (+1/-1)
lib/lp/buildmaster/model/buildqueue.py (+1/-1)
lib/lp/code/model/sourcepackagerecipebuild.py (+4/-0)
lib/lp/code/tests/test_sourcepackagerecipebuild.py (+18/-0)
lib/lp/soyuz/interfaces/buildfarmbuildjob.py (+6/-0)
lib/lp/soyuz/model/buildpackagejob.py (+5/-0)
lib/lp/translations/tests/test_translationtemplatesbuildjob.py (+17/-0)
To merge this branch: bzr merge lp:~wgrant/launchpad/bug-549340-fix-buildqueue-destruction
Reviewer Review Type Date Requested Status
Julian Edwards (community) Disapprove
Review via email: mp+22286@code.launchpad.net

Commit message

Fix destruction of translation template build farm jobs.

Description of the change

This branch fixes BuildQueue.destroySelf to not assume that IBuildFarmJobs are Storm objects. TranslationTemplatesBuildJob isn't -- its DB object is actually a BranchJob.

I've replaced the direct store removal with a call to IBFJ.destroySelf, and implemented it on the two classes that needed it. It actually lives on IBuildFarmBuildJob for now, since TTBJ has security declarations for both IBuildFarmJob and IBranchJob, the latter already defining destroySelf -- this results in a Zope permission definition conflict, even though the permissions are actually the same.

I've also added tests for destruction of TTBJ and SPRBJ BuildQueues. That required the granting of DELETE on SPRBJ to launchpad_main.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

I do not like this fix, we should not be adding more SQLObject methods. Instead, there should be a real TranslationTemplatesBuildJob table so that build history is preserved.

I'm a bit flummoxed as to how its DB object is a BranchJob?

review: Disapprove

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2010-03-15 01:26:43 +0000
+++ database/schema/security.cfg 2010-03-27 11:11:27 +0000
@@ -280,7 +280,7 @@
280public.sourcepackageformatselection = SELECT280public.sourcepackageformatselection = SELECT
281public.sourcepackagerecipe = SELECT, INSERT, UPDATE, DELETE281public.sourcepackagerecipe = SELECT, INSERT, UPDATE, DELETE
282public.sourcepackagerecipebuild = SELECT, INSERT, UPDATE, DELETE282public.sourcepackagerecipebuild = SELECT, INSERT, UPDATE, DELETE
283public.sourcepackagerecipebuildjob = SELECT, INSERT, UPDATE283public.sourcepackagerecipebuildjob = SELECT, INSERT, UPDATE, DELETE
284public.sourcepackagerecipedata = SELECT, INSERT, UPDATE, DELETE284public.sourcepackagerecipedata = SELECT, INSERT, UPDATE, DELETE
285public.sourcepackagerecipedatainstruction = SELECT, INSERT, UPDATE, DELETE285public.sourcepackagerecipedatainstruction = SELECT, INSERT, UPDATE, DELETE
286public.specificationbranch = SELECT, INSERT, UPDATE, DELETE286public.specificationbranch = SELECT, INSERT, UPDATE, DELETE
287287
=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py 2010-03-17 05:58:22 +0000
+++ lib/lp/buildmaster/model/buildqueue.py 2010-03-27 11:11:27 +0000
@@ -133,7 +133,7 @@
133 job = self.job133 job = self.job
134 specific_job = self.specific_job134 specific_job = self.specific_job
135 SQLBase.destroySelf(self)135 SQLBase.destroySelf(self)
136 Store.of(specific_job).remove(specific_job)136 specific_job.destroySelf()
137 job.destroySelf()137 job.destroySelf()
138138
139 def manualScore(self, value):139 def manualScore(self, value):
140140
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py 2010-03-25 15:24:39 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py 2010-03-27 11:11:27 +0000
@@ -216,3 +216,7 @@
216 store = IMasterStore(SourcePackageRecipeBuildJob)216 store = IMasterStore(SourcePackageRecipeBuildJob)
217 store.add(specific_job)217 store.add(specific_job)
218 return specific_job218 return specific_job
219
220 def destroySelf(self):
221 """See `IBuildFarmJob`."""
222 Store.of(self).remove(self)
219223
=== modified file 'lib/lp/code/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/tests/test_sourcepackagerecipebuild.py 2010-03-25 15:29:31 +0000
+++ lib/lp/code/tests/test_sourcepackagerecipebuild.py 2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
11import transaction11import transaction
12from zope.component import getUtility12from zope.component import getUtility
1313
14from canonical.launchpad.interfaces.lpstorm import IStore
14from canonical.testing.layers import DatabaseFunctionalLayer15from canonical.testing.layers import DatabaseFunctionalLayer
1516
16from lp.buildmaster.interfaces.buildbase import IBuildBase17from lp.buildmaster.interfaces.buildbase import IBuildBase
@@ -18,6 +19,8 @@
18from lp.code.interfaces.sourcepackagerecipebuild import (19from lp.code.interfaces.sourcepackagerecipebuild import (
19 ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,20 ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
20 ISourcePackageRecipeBuildSource)21 ISourcePackageRecipeBuildSource)
22from lp.code.model.sourcepackagerecipebuild import (
23 SourcePackageRecipeBuildJob)
21from lp.testing import TestCaseWithFactory24from lp.testing import TestCaseWithFactory
2225
2326
@@ -92,6 +95,21 @@
92 self.assertEqual(95 self.assertEqual(
93 datetime.timedelta(minutes=2), spb.estimateDuration())96 datetime.timedelta(minutes=2), spb.estimateDuration())
9497
98 def test_BuildQueue_destruction(self):
99 # When destroying the related BuildQueue, the SPRBJ is also
100 # destroyed.
101 spb = self.makeSourcePackageRecipeBuild()
102 bq = spb.queueBuild()
103 self.assertEqual(
104 1,
105 IStore(SourcePackageRecipeBuildJob).find(
106 SourcePackageRecipeBuildJob, build=spb).count())
107 bq.destroySelf()
108 self.assertEqual(
109 0,
110 IStore(SourcePackageRecipeBuildJob).find(
111 SourcePackageRecipeBuildJob, build=spb).count())
112
95113
96def test_suite():114def test_suite():
97 return unittest.TestLoader().loadTestsFromName(__name__)115 return unittest.TestLoader().loadTestsFromName(__name__)
98116
=== modified file 'lib/lp/soyuz/interfaces/buildfarmbuildjob.py'
--- lib/lp/soyuz/interfaces/buildfarmbuildjob.py 2010-03-16 05:08:47 +0000
+++ lib/lp/soyuz/interfaces/buildfarmbuildjob.py 2010-03-27 11:11:27 +0000
@@ -19,3 +19,9 @@
19 build = Reference(19 build = Reference(
20 IBuild, title=_("Build"), required=True, readonly=True,20 IBuild, title=_("Build"), required=True, readonly=True,
21 description=_("Build record associated with this job."))21 description=_("Build record associated with this job."))
22
23 # XXX wgrant 2010-03-27: This should be on IBuildFarmJob, but
24 # IBranchJob has this as well so TTBJ security dies with a conflict.
25 def destroySelf():
26 """Destroy this job."""
27
2228
=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py 2010-03-25 14:38:25 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py 2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
11import pytz11import pytz
1212
13from storm.locals import Int, Reference, Storm13from storm.locals import Int, Reference, Storm
14from storm.store import Store
1415
15from zope.interface import implements16from zope.interface import implements
16from zope.component import getUtility17from zope.component import getUtility
@@ -164,6 +165,10 @@
164 """See `IBuildFarmJob`."""165 """See `IBuildFarmJob`."""
165 return self.build.is_virtualized166 return self.build.is_virtualized
166167
168 def destroySelf(self):
169 """See `IBuildFarmJob`."""
170 Store.of(self).remove(self)
171
167 @staticmethod172 @staticmethod
168 def addCandidateSelectionCriteria(processor, virtualized):173 def addCandidateSelectionCriteria(processor, virtualized):
169 """See `IBuildFarmCandidateJobSelection`."""174 """See `IBuildFarmCandidateJobSelection`."""
170175
=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildjob.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2010-03-06 00:21:57 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2010-03-27 11:11:27 +0000
@@ -11,6 +11,7 @@
1111
12from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities12from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
13from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet13from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
14from canonical.launchpad.interfaces.lpstorm import IStore
14from canonical.launchpad.webapp.testing import verifyObject15from canonical.launchpad.webapp.testing import verifyObject
15from canonical.launchpad.webapp.interfaces import (16from canonical.launchpad.webapp.interfaces import (
16 DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)17 DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
@@ -107,6 +108,22 @@
107 # future however the scoring system is to be revisited.108 # future however the scoring system is to be revisited.
108 self.assertEqual(1000, self.specific_job.score())109 self.assertEqual(1000, self.specific_job.score())
109110
111 def test_BuildQueue_destruction(self):
112 # A TTBJ's BuildQueue can destroy itself, which also destroys
113 # the related BranchJob.
114 self.assertEqual(
115 1,
116 IStore(BranchJob).find(
117 BranchJob, BranchJob.branch == self.branch).count())
118
119 buildqueue = getUtility(IBuildQueueSet).getByJob(
120 self.specific_job.job)
121 buildqueue.destroySelf()
122
123 self.assertEqual(
124 0,
125 IStore(BranchJob).find(BranchJob, branch=self.branch).count())
126
110127
111class FakeTranslationTemplatesJobSource(TranslationTemplatesBuildJob):128class FakeTranslationTemplatesJobSource(TranslationTemplatesBuildJob):
112 """Fake utility class.129 """Fake utility class.