Launchpad itself

Merge lp:~rockstar/launchpad/recipe-oopses into lp:launchpad

  1. recipe-oopses
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: 10997
Proposed branch: lp:~rockstar/launchpad/recipe-oopses
Merge into: lp:launchpad
Diff against target: 312 lines (+111/-46)
7 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+24/-5)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+49/-5)
lib/lp/code/errors.py (+32/-0)
lib/lp/code/interfaces/sourcepackagerecipe.py (+0/-31)
lib/lp/code/model/sourcepackagerecipe.py (+2/-1)
lib/lp/code/model/sourcepackagerecipedata.py (+1/-2)
lib/lp/code/model/tests/test_sourcepackagerecipe.py (+3/-2)
To merge this branch: bzr merge lp:~rockstar/launchpad/recipe-oopses
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+27313@code.launchpad.net

Description of the change

This bug fixes bug #586966 - The run command is not allowed in our use of bzr-builder, and so we should catch that case and notify the user that what they're doing just ain't cool.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

ForbiddenInstruction is being imported from
lp.code.interfaces.sourcepakagerecipe. Can we put that in lp.code.errors
instead?

> except ForbiddenInstruction:
> self.setFieldError(
> 'recipe_text',
> 'The bzr-builder instruction "run" is not permitted here.')

Can we get the instruction out of the forbidden instruction exception? That
makes more sense than hard coding it. This may cause unnecessary confusion if
they have a different forbidden instruction.

Also, you should do the check in the validate method, not in the action.

review: Needs Fixing
Revision history for this message
Tim Penhey (thumper) wrote :

Approved conditional on fixing the formatting on the XXX comments :)

We should have name, date and bug.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2010-06-09 15:32:32 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
@@ -39,6 +39,7 @@
39from canonical.launchpad.webapp.breadcrumb import Breadcrumb39from canonical.launchpad.webapp.breadcrumb import Breadcrumb
40from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget40from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
41from lp.buildmaster.interfaces.buildbase import BuildStatus41from lp.buildmaster.interfaces.buildbase import BuildStatus
42from lp.code.errors import ForbiddenInstruction
42from lp.code.interfaces.sourcepackagerecipe import (43from lp.code.interfaces.sourcepackagerecipe import (
43 ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)44 ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
44from lp.code.interfaces.sourcepackagerecipebuild import (45from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -352,9 +353,18 @@
352 def request_action(self, action, data):353 def request_action(self, action, data):
353 parser = RecipeParser(data['recipe_text'])354 parser = RecipeParser(data['recipe_text'])
354 recipe = parser.parse()355 recipe = parser.parse()
355 source_package_recipe = getUtility(ISourcePackageRecipeSource).new(356 try:
356 self.user, self.user, data['distros'],357 source_package_recipe = getUtility(
357 data['name'], recipe, data['description'])358 ISourcePackageRecipeSource).new(
359 self.user, self.user, data['distros'],
360 data['name'], recipe, data['description'])
361 except ForbiddenInstruction:
362 # XXX: bug=592513 We shouldn't be hardcoding "run" here.
363 self.setFieldError(
364 'recipe_text',
365 'The bzr-builder instruction "run" is not permitted here.')
366 return
367
358 self.next_url = canonical_url(source_package_recipe)368 self.next_url = canonical_url(source_package_recipe)
359369
360 def validate(self, data):370 def validate(self, data):
@@ -402,8 +412,17 @@
402 parser = RecipeParser(recipe_text)412 parser = RecipeParser(recipe_text)
403 recipe = parser.parse()413 recipe = parser.parse()
404 if self.context.builder_recipe != recipe:414 if self.context.builder_recipe != recipe:
405 self.context.builder_recipe = recipe415 try:
406 changed = True416 self.context.builder_recipe = recipe
417 changed = True
418 except ForbiddenInstruction:
419 # XXX: bug=592513 We shouldn't be hardcoding "run" here.
420 self.setFieldError(
421 'recipe_text',
422 'The bzr-builder instruction "run" is not permitted here.'
423 )
424 return
425
407426
408 distros = data.pop('distros')427 distros = data.pop('distros')
409 if distros != self.context.distroseries:428 if distros != self.context.distroseries:
410429
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-09 15:32:32 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
@@ -88,7 +88,6 @@
88 name='ratatouille', displayname='Ratatouille')88 name='ratatouille', displayname='Ratatouille')
89 branch = self.factory.makeBranch(89 branch = self.factory.makeBranch(
90 owner=self.chef, product=product, name='veggies')90 owner=self.chef, product=product, name='veggies')
91 self.factory.makeSourcePackage()
9291
93 # A new recipe can be created from the branch page.92 # A new recipe can be created from the branch page.
94 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)93 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
@@ -120,6 +119,31 @@
120 self.assertTextMatchesExpressionIgnoreWhitespace(119 self.assertTextMatchesExpressionIgnoreWhitespace(
121 pattern, main_text)120 pattern, main_text)
122121
122 def test_create_recipe_forbidden_instruction(self):
123 # We don't allow the "run" instruction in our recipes. Make sure this
124 # is communicated to the user properly.
125 product = self.factory.makeProduct(
126 name='ratatouille', displayname='Ratatouille')
127 branch = self.factory.makeBranch(
128 owner=self.chef, product=product, name='veggies')
129
130 # A new recipe can be created from the branch page.
131 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
132 browser.getLink('Create packaging recipe').click()
133
134 browser.getControl(name='field.name').value = 'daily'
135 browser.getControl('Description').value = 'Make some food!'
136 browser.getControl('Secret Squirrel').click()
137
138 browser.getControl('Recipe text').value = (
139 browser.getControl('Recipe text').value + 'run cat /etc/passwd')
140
141 browser.getControl('Create Recipe').click()
142
143 self.assertEqual(
144 extract_text(find_tags_by_class(browser.contents, 'message')[1]),
145 'The bzr-builder instruction "run" is not permitted here.')
146
123 def test_create_new_recipe_empty_name(self):147 def test_create_new_recipe_empty_name(self):
124 # Leave off the name and make sure that the widgets validate before148 # Leave off the name and make sure that the widgets validate before
125 # the content validates.149 # the content validates.
@@ -127,7 +151,6 @@
127 name='ratatouille', displayname='Ratatouille')151 name='ratatouille', displayname='Ratatouille')
128 branch = self.factory.makeBranch(152 branch = self.factory.makeBranch(
129 owner=self.chef, product=product, name='veggies')153 owner=self.chef, product=product, name='veggies')
130 self.factory.makeSourcePackage()
131154
132 # A new recipe can be created from the branch page.155 # A new recipe can be created from the branch page.
133 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)156 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
@@ -148,7 +171,6 @@
148 name='ratatouille', displayname='Ratatouille')171 name='ratatouille', displayname='Ratatouille')
149 branch = self.factory.makeBranch(172 branch = self.factory.makeBranch(
150 owner=self.chef, product=product, name='veggies')173 owner=self.chef, product=product, name='veggies')
151 self.factory.makeSourcePackage()
152174
153 # A new recipe can be created from the branch page.175 # A new recipe can be created from the branch page.
154 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)176 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
@@ -172,7 +194,6 @@
172 name='ratatouille', displayname='Ratatouille')194 name='ratatouille', displayname='Ratatouille')
173 branch = self.factory.makeBranch(195 branch = self.factory.makeBranch(
174 owner=self.chef, product=product, name='veggies')196 owner=self.chef, product=product, name='veggies')
175 self.factory.makeSourcePackage(sourcepackagename='ratatouille')
176197
177 # A new recipe can be created from the branch page.198 # A new recipe can be created from the branch page.
178 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)199 browser = self.getUserBrowser(canonical_url(branch), user=self.chef)
@@ -241,6 +262,29 @@
241 self.assertTextMatchesExpressionIgnoreWhitespace(262 self.assertTextMatchesExpressionIgnoreWhitespace(
242 pattern, main_text)263 pattern, main_text)
243264
265 def test_edit_recipe_forbidden_instruction(self):
266 self.factory.makeDistroSeries(
267 displayname='Mumbly Midget', name='mumbly',
268 distribution=self.ppa.distribution)
269 product = self.factory.makeProduct(
270 name='ratatouille', displayname='Ratatouille')
271 veggie_branch = self.factory.makeBranch(
272 owner=self.chef, product=product, name='veggies')
273 recipe = self.factory.makeSourcePackageRecipe(
274 owner=self.chef, registrant=self.chef,
275 name=u'things', description=u'This is a recipe',
276 distroseries=self.squirrel, branches=[veggie_branch])
277
278 browser = self.getUserBrowser(canonical_url(recipe), user=self.chef)
279 browser.getLink('Edit recipe').click()
280 browser.getControl('Recipe text').value = (
281 browser.getControl('Recipe text').value + 'run cat /etc/passwd')
282 browser.getControl('Update Recipe').click()
283
284 self.assertEqual(
285 extract_text(find_tags_by_class(browser.contents, 'message')[1]),
286 'The bzr-builder instruction "run" is not permitted here.')
287
244 def test_edit_recipe_already_exists(self):288 def test_edit_recipe_already_exists(self):
245 self.factory.makeDistroSeries(289 self.factory.makeDistroSeries(
246 displayname='Mumbly Midget', name='mumbly',290 displayname='Mumbly Midget', name='mumbly',
@@ -255,7 +299,7 @@
255 owner=self.chef, registrant=self.chef,299 owner=self.chef, registrant=self.chef,
256 name=u'things', description=u'This is a recipe',300 name=u'things', description=u'This is a recipe',
257 distroseries=self.squirrel, branches=[veggie_branch])301 distroseries=self.squirrel, branches=[veggie_branch])
258 recipe_fings = self.factory.makeSourcePackageRecipe(302 self.factory.makeSourcePackageRecipe(
259 owner=self.chef, registrant=self.chef,303 owner=self.chef, registrant=self.chef,
260 name=u'fings', description=u'This is a recipe',304 name=u'fings', description=u'This is a recipe',
261 distroseries=self.squirrel, branches=[veggie_branch])305 distroseries=self.squirrel, branches=[veggie_branch])
262306
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2010-05-26 17:16:01 +0000
+++ lib/lp/code/errors.py 2010-06-11 19:07:30 +0000
@@ -12,8 +12,11 @@
12 'CodeImportAlreadyRunning',12 'CodeImportAlreadyRunning',
13 'CodeImportNotInReviewedState',13 'CodeImportNotInReviewedState',
14 'ClaimReviewFailed',14 'ClaimReviewFailed',
15 'ForbiddenInstruction',
15 'InvalidBranchMergeProposal',16 'InvalidBranchMergeProposal',
16 'ReviewNotPending',17 'ReviewNotPending',
18 'TooManyBuilds',
19 'TooNewRecipeFormat',
17 'UnknownBranchTypeError',20 'UnknownBranchTypeError',
18 'UserHasExistingReview',21 'UserHasExistingReview',
19 'UserNotBranchReviewer',22 'UserNotBranchReviewer',
@@ -91,3 +94,32 @@
91 """Raised when the user requests an import that is already running."""94 """Raised when the user requests an import that is already running."""
9295
93 webservice_error(400)96 webservice_error(400)
97
98
99class ForbiddenInstruction(Exception):
100 """A forbidden instruction was found in the recipe."""
101
102 def __init__(self, instruction_name):
103 super(ForbiddenInstruction, self).__init__()
104 self.instruction_name = instruction_name
105
106
107class TooNewRecipeFormat(Exception):
108 """The format of the recipe supplied was too new."""
109
110 def __init__(self, supplied_format, newest_supported):
111 super(TooNewRecipeFormat, self).__init__()
112 self.supplied_format = supplied_format
113 self.newest_supported = newest_supported
114
115
116class TooManyBuilds(Exception):
117 """A build was requested that exceeded the quota."""
118
119 def __init__(self, recipe, distroseries):
120 self.recipe = recipe
121 self.distroseries = distroseries
122 msg = (
123 'You have exceeded your quota for recipe %s for distroseries %s'
124 % (self.recipe, distroseries))
125 Exception.__init__(self, msg)
94126
=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py 2010-06-04 16:14:54 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
@@ -10,13 +10,10 @@
1010
1111
12__all__ = [12__all__ = [
13 'ForbiddenInstruction',
14 'ISourcePackageRecipe',13 'ISourcePackageRecipe',
15 'ISourcePackageRecipeData',14 'ISourcePackageRecipeData',
16 'ISourcePackageRecipeSource',15 'ISourcePackageRecipeSource',
17 'MINIMAL_RECIPE_TEXT',16 'MINIMAL_RECIPE_TEXT',
18 'TooManyBuilds',
19 'TooNewRecipeFormat',
20 ]17 ]
2118
2219
@@ -47,34 +44,6 @@
47 %s44 %s
48 ''')45 ''')
4946
50class ForbiddenInstruction(Exception):
51 """A forbidden instruction was found in the recipe."""
52
53 def __init__(self, instruction_name):
54 super(ForbiddenInstruction, self).__init__()
55 self.instruction_name = instruction_name
56
57
58class TooManyBuilds(Exception):
59 """A build was requested that exceeded the quota."""
60
61 def __init__(self, recipe, distroseries):
62 self.recipe = recipe
63 self.distroseries = distroseries
64 msg = (
65 'You have exceeded your quota for recipe %s for distroseries %s'
66 % (self.recipe, distroseries))
67 Exception.__init__(self, msg)
68
69
70class TooNewRecipeFormat(Exception):
71 """The format of the recipe supplied was too new."""
72
73 def __init__(self, supplied_format, newest_supported):
74 super(TooNewRecipeFormat, self).__init__()
75 self.supplied_format = supplied_format
76 self.newest_supported = newest_supported
77
7847
79class ISourcePackageRecipeData(Interface):48class ISourcePackageRecipeData(Interface):
80 """A recipe as database data, not text."""49 """A recipe as database data, not text."""
8150
=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py 2010-06-10 09:55:32 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
@@ -23,9 +23,10 @@
23from canonical.database.datetimecol import UtcDateTimeCol23from canonical.database.datetimecol import UtcDateTimeCol
24from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore24from canonical.launchpad.interfaces.lpstorm import IMasterStore, IStore
2525
26from lp.code.errors import TooManyBuilds
26from lp.code.interfaces.sourcepackagerecipe import (27from lp.code.interfaces.sourcepackagerecipe import (
27 ISourcePackageRecipe, ISourcePackageRecipeSource,28 ISourcePackageRecipe, ISourcePackageRecipeSource,
28 ISourcePackageRecipeData, TooManyBuilds)29 ISourcePackageRecipeData)
29from lp.code.interfaces.sourcepackagerecipebuild import (30from lp.code.interfaces.sourcepackagerecipebuild import (
30 ISourcePackageRecipeBuildSource)31 ISourcePackageRecipeBuildSource)
31from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild32from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
3233
=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py 2010-04-19 05:04:00 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py 2010-06-11 19:07:30 +0000
@@ -27,11 +27,10 @@
27from canonical.database.enumcol import EnumCol27from canonical.database.enumcol import EnumCol
28from canonical.launchpad.interfaces.lpstorm import IStore28from canonical.launchpad.interfaces.lpstorm import IStore
2929
30from lp.code.errors import ForbiddenInstruction, TooNewRecipeFormat
30from lp.code.model.branch import Branch31from lp.code.model.branch import Branch
31from lp.code.interfaces.branch import NoSuchBranch32from lp.code.interfaces.branch import NoSuchBranch
32from lp.code.interfaces.branchlookup import IBranchLookup33from lp.code.interfaces.branchlookup import IBranchLookup
33from lp.code.interfaces.sourcepackagerecipe import (
34 ForbiddenInstruction, TooNewRecipeFormat)
3534
3635
37class InstructionType(DBEnumeratedType):36class InstructionType(DBEnumeratedType):
3837
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-06-10 09:55:32 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-06-11 19:07:30 +0000
@@ -29,9 +29,10 @@
29 InvalidPocketForPPA)29 InvalidPocketForPPA)
30from lp.buildmaster.interfaces.buildqueue import IBuildQueue30from lp.buildmaster.interfaces.buildqueue import IBuildQueue
31from lp.buildmaster.model.buildqueue import BuildQueue31from lp.buildmaster.model.buildqueue import BuildQueue
32from lp.code.errors import (
33 ForbiddenInstruction, TooManyBuilds, TooNewRecipeFormat)
32from lp.code.interfaces.sourcepackagerecipe import (34from lp.code.interfaces.sourcepackagerecipe import (
33 ForbiddenInstruction, ISourcePackageRecipe, ISourcePackageRecipeSource,35 ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
34 TooManyBuilds, TooNewRecipeFormat, MINIMAL_RECIPE_TEXT)
35from lp.code.interfaces.sourcepackagerecipebuild import (36from lp.code.interfaces.sourcepackagerecipebuild import (
36 ISourcePackageRecipeBuild, ISourcePackageRecipeBuildJob)37 ISourcePackageRecipeBuild, ISourcePackageRecipeBuildJob)
37from lp.code.model.sourcepackagerecipebuild import (38from lp.code.model.sourcepackagerecipebuild import (