Merge lp:~james-w/launchpad/create-source-recipe-build into lp:launchpad
- create-source-recipe-build
- Merge into devel
Proposed by
James Westby
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~james-w/launchpad/create-source-recipe-build |
Merge into: | lp:launchpad |
Prerequisite: | lp:~wgrant/launchpad/sprb-createbuildqueueentry-creation |
Diff against target: |
268 lines (+132/-13) 6 files modified
lib/lp/archiveuploader/permission.py (+19/-10) lib/lp/archiveuploader/tests/test_permission.py (+8/-0) lib/lp/archiveuploader/uploadprocessor.py (+0/-3) lib/lp/soyuz/interfaces/sourcepackagerecipe.py (+10/-0) lib/lp/soyuz/model/sourcepackagerecipe.py (+26/-0) lib/lp/soyuz/tests/test_sourcepackagerecipe.py (+69/-0) |
To merge this branch: | bzr merge lp:~james-w/launchpad/create-source-recipe-build |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Julian Edwards (community) | code | Approve | |
Review via email: mp+17435@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote : | # |
Revision history for this message
Julian Edwards (julian-edwards) wrote : | # |
Looks good, just a few small tweaks. Landing is conditional on fixing these and your TODO!
* Please document the fact that requestBuild raises exceptions in the interface docstring.
* it's "targeted" not "targetted"
* Our coding standards say that you should format method invocations like this:
method(
arg, arg, arg)
i.e. newline after the opening parens.
* Also indent 4 chars only
Nice tests!
review:
Approve
(code)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/archiveuploader/permission.py' | |||
2 | --- lib/lp/archiveuploader/permission.py 2009-11-20 22:22:53 +0000 | |||
3 | +++ lib/lp/archiveuploader/permission.py 2010-01-15 01:34:29 +0000 | |||
4 | @@ -20,24 +20,21 @@ | |||
5 | 20 | from lp.soyuz.interfaces.archive import ArchivePurpose | 20 | from lp.soyuz.interfaces.archive import ArchivePurpose |
6 | 21 | 21 | ||
7 | 22 | 22 | ||
9 | 23 | class CannotUploadToArchive: | 23 | class CannotUploadToArchive(Exception): |
10 | 24 | """A reason for not being able to upload to an archive.""" | 24 | """A reason for not being able to upload to an archive.""" |
11 | 25 | 25 | ||
12 | 26 | _fmt = '%(person)s has no upload rights to %(archive)s.' | 26 | _fmt = '%(person)s has no upload rights to %(archive)s.' |
13 | 27 | 27 | ||
14 | 28 | def __init__(self, **args): | 28 | def __init__(self, **args): |
15 | 29 | """Construct a `CannotUploadToArchive`.""" | 29 | """Construct a `CannotUploadToArchive`.""" |
23 | 30 | self._message = self._fmt % args | 30 | Exception.__init__(self, self._fmt % args) |
24 | 31 | 31 | ||
25 | 32 | def __str__(self): | 32 | |
26 | 33 | return self._message | 33 | class CannotUploadToPocket(Exception): |
20 | 34 | |||
21 | 35 | |||
22 | 36 | class CannotUploadToPocket: | ||
27 | 37 | """Returned when a pocket is closed for uploads.""" | 34 | """Returned when a pocket is closed for uploads.""" |
28 | 38 | 35 | ||
29 | 39 | def __init__(self, distroseries, pocket): | 36 | def __init__(self, distroseries, pocket): |
31 | 40 | super(CannotUploadToPocket, self).__init__( | 37 | Exception.__init__(self, |
32 | 41 | "Not permitted to upload to the %s pocket in a series in the " | 38 | "Not permitted to upload to the %s pocket in a series in the " |
33 | 42 | "'%s' state." % (pocket.name, distroseries.status.name)) | 39 | "'%s' state." % (pocket.name, distroseries.status.name)) |
34 | 43 | 40 | ||
35 | @@ -72,7 +69,7 @@ | |||
36 | 72 | "Signer is not permitted to upload to the component '%(component)s'.") | 69 | "Signer is not permitted to upload to the component '%(component)s'.") |
37 | 73 | 70 | ||
38 | 74 | def __init__(self, component): | 71 | def __init__(self, component): |
40 | 75 | super(NoRightsForComponent, self).__init__(component=component.name) | 72 | CannotUploadToArchive.__init__(self, component=component.name) |
41 | 76 | 73 | ||
42 | 77 | 74 | ||
43 | 78 | class InvalidPocketForPPA(CannotUploadToArchive): | 75 | class InvalidPocketForPPA(CannotUploadToArchive): |
44 | @@ -87,6 +84,15 @@ | |||
45 | 87 | _fmt = "Partner uploads must be for the RELEASE or PROPOSED pocket." | 84 | _fmt = "Partner uploads must be for the RELEASE or PROPOSED pocket." |
46 | 88 | 85 | ||
47 | 89 | 86 | ||
48 | 87 | class ArchiveDisabled(CannotUploadToArchive): | ||
49 | 88 | """Uploading to a disabled archive is not allowed.""" | ||
50 | 89 | |||
51 | 90 | _fmt = ("%(archive_name)s is disabled.") | ||
52 | 91 | |||
53 | 92 | def __init__(self, archive_name): | ||
54 | 93 | CannotUploadToArchive.__init__(self, archive_name=archive_name) | ||
55 | 94 | |||
56 | 95 | |||
57 | 90 | def components_valid_for(archive, person): | 96 | def components_valid_for(archive, person): |
58 | 91 | """Return the components that 'person' can upload to 'archive'. | 97 | """Return the components that 'person' can upload to 'archive'. |
59 | 92 | 98 | ||
60 | @@ -190,6 +196,9 @@ | |||
61 | 190 | :return: CannotUploadToArchive if 'person' cannot upload to the archive, | 196 | :return: CannotUploadToArchive if 'person' cannot upload to the archive, |
62 | 191 | None otherwise. | 197 | None otherwise. |
63 | 192 | """ | 198 | """ |
64 | 199 | if not archive.enabled: | ||
65 | 200 | return ArchiveDisabled(archive.displayname) | ||
66 | 201 | |||
67 | 193 | # For PPAs... | 202 | # For PPAs... |
68 | 194 | if archive.is_ppa: | 203 | if archive.is_ppa: |
69 | 195 | if not archive.canUpload(person): | 204 | if not archive.canUpload(person): |
70 | 196 | 205 | ||
71 | === modified file 'lib/lp/archiveuploader/tests/test_permission.py' | |||
72 | --- lib/lp/archiveuploader/tests/test_permission.py 2009-11-20 22:22:53 +0000 | |||
73 | +++ lib/lp/archiveuploader/tests/test_permission.py 2010-01-15 01:34:29 +0000 | |||
74 | @@ -241,6 +241,14 @@ | |||
75 | 241 | self.assertCanUpload( | 241 | self.assertCanUpload( |
76 | 242 | person, spn, archive, component_a, strict_component=False) | 242 | person, spn, archive, component_a, strict_component=False) |
77 | 243 | 243 | ||
78 | 244 | def test_cannot_upload_to_disabled_archive(self): | ||
79 | 245 | spn = self.factory.makeSourcePackageName() | ||
80 | 246 | archive = self.factory.makeArchive() | ||
81 | 247 | removeSecurityProxy(archive).disable() | ||
82 | 248 | component = self.factory.makeComponent() | ||
83 | 249 | self.assertCannotUpload(u"%s is disabled." % (archive.displayname), | ||
84 | 250 | archive.owner, spn, archive, component) | ||
85 | 251 | |||
86 | 244 | 252 | ||
87 | 245 | def test_suite(): | 253 | def test_suite(): |
88 | 246 | return unittest.TestLoader().loadTestsFromName(__name__) | 254 | return unittest.TestLoader().loadTestsFromName(__name__) |
89 | 247 | 255 | ||
90 | === modified file 'lib/lp/archiveuploader/uploadprocessor.py' | |||
91 | --- lib/lp/archiveuploader/uploadprocessor.py 2009-06-24 23:33:29 +0000 | |||
92 | +++ lib/lp/archiveuploader/uploadprocessor.py 2010-01-15 01:34:29 +0000 | |||
93 | @@ -587,9 +587,6 @@ | |||
94 | 587 | # Upload path does not match anything we support. | 587 | # Upload path does not match anything we support. |
95 | 588 | raise UploadPathError("Path format mismatch.") | 588 | raise UploadPathError("Path format mismatch.") |
96 | 589 | 589 | ||
97 | 590 | if not archive.enabled: | ||
98 | 591 | raise PPAUploadPathError("%s is disabled." % archive.displayname) | ||
99 | 592 | |||
100 | 593 | if archive.distribution != distribution: | 590 | if archive.distribution != distribution: |
101 | 594 | raise PPAUploadPathError( | 591 | raise PPAUploadPathError( |
102 | 595 | "%s only supports uploads to '%s' distribution." | 592 | "%s only supports uploads to '%s' distribution." |
103 | 596 | 593 | ||
104 | === modified file 'lib/lp/soyuz/interfaces/sourcepackagerecipe.py' | |||
105 | --- lib/lp/soyuz/interfaces/sourcepackagerecipe.py 2010-01-15 01:34:24 +0000 | |||
106 | +++ lib/lp/soyuz/interfaces/sourcepackagerecipe.py 2010-01-15 01:34:29 +0000 | |||
107 | @@ -77,6 +77,16 @@ | |||
108 | 77 | def getReferencedBranches(): | 77 | def getReferencedBranches(): |
109 | 78 | """An iterator of the branches referenced by this recipe.""" | 78 | """An iterator of the branches referenced by this recipe.""" |
110 | 79 | 79 | ||
111 | 80 | def requestBuild(self, archive, distroseries, requester, pocket): | ||
112 | 81 | """Request that the recipe be built in to the specified archive. | ||
113 | 82 | |||
114 | 83 | :param archive: The IArchive which you want the build to end up in. | ||
115 | 84 | :param distroseries: The DistroSeries that the package should be | ||
116 | 85 | targetted to. | ||
117 | 86 | :param requester: the person requesting the build. | ||
118 | 87 | :param pocket: the pocket that should be targetted. | ||
119 | 88 | """ | ||
120 | 89 | |||
121 | 80 | 90 | ||
122 | 81 | class ISourcePackageRecipeSource(Interface): | 91 | class ISourcePackageRecipeSource(Interface): |
123 | 82 | """A utility of this interface can be used to create and access recipes. | 92 | """A utility of this interface can be used to create and access recipes. |
124 | 83 | 93 | ||
125 | === modified file 'lib/lp/soyuz/model/sourcepackagerecipe.py' | |||
126 | --- lib/lp/soyuz/model/sourcepackagerecipe.py 2010-01-15 01:34:24 +0000 | |||
127 | +++ lib/lp/soyuz/model/sourcepackagerecipe.py 2010-01-15 01:34:29 +0000 | |||
128 | @@ -10,16 +10,26 @@ | |||
129 | 10 | 10 | ||
130 | 11 | from storm.locals import Int, Reference, Store, Storm, Unicode | 11 | from storm.locals import Int, Reference, Store, Storm, Unicode |
131 | 12 | 12 | ||
132 | 13 | from zope.component import getUtility | ||
133 | 13 | from zope.interface import classProvides, implements | 14 | from zope.interface import classProvides, implements |
134 | 14 | 15 | ||
135 | 15 | from canonical.database.datetimecol import UtcDateTimeCol | 16 | from canonical.database.datetimecol import UtcDateTimeCol |
136 | 16 | from canonical.launchpad.interfaces.lpstorm import IMasterStore | 17 | from canonical.launchpad.interfaces.lpstorm import IMasterStore |
137 | 17 | 18 | ||
138 | 19 | from lp.soyuz.interfaces.archive import ArchivePurpose | ||
139 | 20 | from lp.soyuz.interfaces.component import IComponentSet | ||
140 | 21 | from lp.archiveuploader.permission import check_upload_to_archive | ||
141 | 18 | from lp.soyuz.interfaces.sourcepackagerecipe import ( | 22 | from lp.soyuz.interfaces.sourcepackagerecipe import ( |
142 | 19 | ISourcePackageRecipe, ISourcePackageRecipeSource) | 23 | ISourcePackageRecipe, ISourcePackageRecipeSource) |
143 | 24 | from lp.soyuz.interfaces.sourcepackagerecipebuild import ( | ||
144 | 25 | ISourcePackageRecipeBuildSource) | ||
145 | 20 | from lp.soyuz.model.sourcepackagerecipedata import _SourcePackageRecipeData | 26 | from lp.soyuz.model.sourcepackagerecipedata import _SourcePackageRecipeData |
146 | 21 | 27 | ||
147 | 22 | 28 | ||
148 | 29 | class NonPPABuildRequest(Exception): | ||
149 | 30 | """A build was requested to a non-PPA and this is currently unsupported.""" | ||
150 | 31 | |||
151 | 32 | |||
152 | 23 | class SourcePackageRecipe(Storm): | 33 | class SourcePackageRecipe(Storm): |
153 | 24 | """See `ISourcePackageRecipe` and `ISourcePackageRecipeSource`.""" | 34 | """See `ISourcePackageRecipe` and `ISourcePackageRecipeSource`.""" |
154 | 25 | 35 | ||
155 | @@ -82,3 +92,19 @@ | |||
156 | 82 | sprecipe.name = name | 92 | sprecipe.name = name |
157 | 83 | store.add(sprecipe) | 93 | store.add(sprecipe) |
158 | 84 | return sprecipe | 94 | return sprecipe |
159 | 95 | |||
160 | 96 | def requestBuild(self, archive, requester, pocket): | ||
161 | 97 | if archive.purpose != ArchivePurpose.PPA: | ||
162 | 98 | raise NonPPABuildRequest | ||
163 | 99 | component = getUtility(IComponentSet)["multiverse"] | ||
164 | 100 | reject_reason = check_upload_to_archive(requester, | ||
165 | 101 | self.distroseries, self.sourcepackagename, | ||
166 | 102 | archive, component, pocket) | ||
167 | 103 | if reject_reason is not None: | ||
168 | 104 | raise reject_reason | ||
169 | 105 | sourcepackage = self.distroseries.getSourcePackage( | ||
170 | 106 | self.sourcepackagename) | ||
171 | 107 | build = getUtility(ISourcePackageRecipeBuildSource).new(sourcepackage, | ||
172 | 108 | self, requester, archive) | ||
173 | 109 | build.createBuildQueueEntry() | ||
174 | 110 | return build | ||
175 | 85 | 111 | ||
176 | === modified file 'lib/lp/soyuz/tests/test_sourcepackagerecipe.py' | |||
177 | --- lib/lp/soyuz/tests/test_sourcepackagerecipe.py 2010-01-15 01:34:24 +0000 | |||
178 | +++ lib/lp/soyuz/tests/test_sourcepackagerecipe.py 2010-01-15 01:34:29 +0000 | |||
179 | @@ -10,14 +10,33 @@ | |||
180 | 10 | 10 | ||
181 | 11 | from bzrlib.plugins.builder.recipe import RecipeParser | 11 | from bzrlib.plugins.builder.recipe import RecipeParser |
182 | 12 | 12 | ||
183 | 13 | from storm.locals import Store | ||
184 | 14 | |||
185 | 13 | from zope.component import getUtility | 15 | from zope.component import getUtility |
186 | 14 | from zope.security.interfaces import Unauthorized | 16 | from zope.security.interfaces import Unauthorized |
187 | 17 | from zope.security.proxy import removeSecurityProxy | ||
188 | 15 | 18 | ||
189 | 16 | from canonical.testing.layers import DatabaseFunctionalLayer | 19 | from canonical.testing.layers import DatabaseFunctionalLayer |
190 | 17 | 20 | ||
191 | 21 | from lp.archiveuploader.permission import ( | ||
192 | 22 | ArchiveDisabled, CannotUploadToArchive, InvalidPocketForPPA) | ||
193 | 23 | from lp.registry.interfaces.pocket import PackagePublishingPocket | ||
194 | 24 | from lp.services.job.interfaces.job import ( | ||
195 | 25 | IJob, JobStatus) | ||
196 | 26 | from lp.soyuz.interfaces.archive import ArchivePurpose | ||
197 | 27 | from lp.soyuz.interfaces.buildqueue import ( | ||
198 | 28 | IBuildQueue) | ||
199 | 18 | from lp.soyuz.interfaces.sourcepackagerecipe import ( | 29 | from lp.soyuz.interfaces.sourcepackagerecipe import ( |
200 | 19 | ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource, | 30 | ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource, |
201 | 20 | TooNewRecipeFormat) | 31 | TooNewRecipeFormat) |
202 | 32 | from lp.soyuz.interfaces.sourcepackagerecipebuild import ( | ||
203 | 33 | ISourcePackageRecipeBuild, ISourcePackageRecipeBuildJob) | ||
204 | 34 | from lp.soyuz.model.buildqueue import ( | ||
205 | 35 | BuildQueue) | ||
206 | 36 | from lp.soyuz.model.sourcepackagerecipebuild import ( | ||
207 | 37 | SourcePackageRecipeBuildJob) | ||
208 | 38 | from lp.soyuz.model.sourcepackagerecipe import ( | ||
209 | 39 | NonPPABuildRequest) | ||
210 | 21 | from lp.testing import login_person, TestCaseWithFactory | 40 | from lp.testing import login_person, TestCaseWithFactory |
211 | 22 | 41 | ||
212 | 23 | 42 | ||
213 | @@ -154,6 +173,56 @@ | |||
214 | 154 | TooNewRecipeFormat, | 173 | TooNewRecipeFormat, |
215 | 155 | self.makeSourcePackageRecipeFromBuilderRecipe, builder_recipe) | 174 | self.makeSourcePackageRecipeFromBuilderRecipe, builder_recipe) |
216 | 156 | 175 | ||
217 | 176 | def test_requestBuild(self): | ||
218 | 177 | recipe = self.factory.makeSourcePackageRecipe() | ||
219 | 178 | ppa = self.factory.makeArchive() | ||
220 | 179 | build = recipe.requestBuild(ppa, ppa.owner, | ||
221 | 180 | PackagePublishingPocket.RELEASE) | ||
222 | 181 | # TODO: Fails as SourcePackageRecipeBuild doesn't correctly | ||
223 | 182 | # implement the interface currently. | ||
224 | 183 | #self.assertProvides(build, ISourcePackageRecipeBuild) | ||
225 | 184 | self.assertEqual(build.archive, ppa) | ||
226 | 185 | self.assertEqual(build.distroseries, recipe.distroseries) | ||
227 | 186 | self.assertEqual(build.requester, ppa.owner) | ||
228 | 187 | store = Store.of(build) | ||
229 | 188 | store.flush() | ||
230 | 189 | build_job = store.find(SourcePackageRecipeBuildJob, | ||
231 | 190 | SourcePackageRecipeBuildJob.build_id==build.id).one() | ||
232 | 191 | self.assertProvides(build_job, ISourcePackageRecipeBuildJob) | ||
233 | 192 | self.assertTrue(build_job.virtualized) | ||
234 | 193 | job = build_job.job | ||
235 | 194 | self.assertProvides(job, IJob) | ||
236 | 195 | self.assertEquals(job.status, JobStatus.WAITING) | ||
237 | 196 | build_queue = store.find(BuildQueue, BuildQueue.job==job.id).one() | ||
238 | 197 | self.assertProvides(build_queue, IBuildQueue) | ||
239 | 198 | self.assertTrue(build_queue.virtualized) | ||
240 | 199 | |||
241 | 200 | def test_requestBuildRejectsNotPPA(self): | ||
242 | 201 | recipe = self.factory.makeSourcePackageRecipe() | ||
243 | 202 | not_ppa = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY) | ||
244 | 203 | self.assertRaises(NonPPABuildRequest, recipe.requestBuild, not_ppa, | ||
245 | 204 | not_ppa.owner, PackagePublishingPocket.RELEASE) | ||
246 | 205 | |||
247 | 206 | def test_requestBuildRejectsNoPermission(self): | ||
248 | 207 | recipe = self.factory.makeSourcePackageRecipe() | ||
249 | 208 | ppa = self.factory.makeArchive() | ||
250 | 209 | requester = self.factory.makePerson() | ||
251 | 210 | self.assertRaises(CannotUploadToArchive, recipe.requestBuild, ppa, | ||
252 | 211 | requester, PackagePublishingPocket.RELEASE) | ||
253 | 212 | |||
254 | 213 | def test_requestBuildRejectsInvalidPocket(self): | ||
255 | 214 | recipe = self.factory.makeSourcePackageRecipe() | ||
256 | 215 | ppa = self.factory.makeArchive() | ||
257 | 216 | self.assertRaises(InvalidPocketForPPA, recipe.requestBuild, ppa, | ||
258 | 217 | ppa.owner, PackagePublishingPocket.BACKPORTS) | ||
259 | 218 | |||
260 | 219 | def test_requestBuildRejectsDisabledArchive(self): | ||
261 | 220 | recipe = self.factory.makeSourcePackageRecipe() | ||
262 | 221 | ppa = self.factory.makeArchive() | ||
263 | 222 | removeSecurityProxy(ppa).disable() | ||
264 | 223 | self.assertRaises(ArchiveDisabled, recipe.requestBuild, ppa, | ||
265 | 224 | ppa.owner, PackagePublishingPocket.RELEASE) | ||
266 | 225 | |||
267 | 157 | 226 | ||
268 | 158 | class TestRecipeBranchRoundTripping(TestCaseWithFactory): | 227 | class TestRecipeBranchRoundTripping(TestCaseWithFactory): |
269 | 159 | 228 |
This adds a requestBuild method to SourcePackageRe cipe, allowing you
to request that the recipe be built in to a specific archive.
There is a TODO which can be removed and the assertion uncommented
when the prerequisite branch is fixed to implement the interface
correctly.
There are a few other changes necessary to do this elegantly:
The objects returned from check_upload_ to_archive now derive from
Exception so that we can raise them directly. This shouldn't affect
existing code.
check_ upload_ to_archive now checks for disabled archives, and this
is moved out of processupload.py
Thanks,
James