launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00322
[Merge] lp:~abentley/launchpad/reject-private-recipe into lp:launchpad/devel
Aaron Bentley has proposed merging lp:~abentley/launchpad/reject-private-recipe into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#607908 recipes with private branches should fail earlier
https://bugs.launchpad.net/bugs/607908
= Summary =
Fix bug #607908: recipes with private branches should fail earlier
== Proposed fix ==
Raise an exception when creating or updating SourcePackageRecipeData that uses
private branches. Forward the exception to the user as a user error.
== Pre-implementation notes ==
None
== Implementation details ==
I fixed some lint, e.g. whitespace between 'And' and '('.
== Tests ==
bin/test -t test_creation -t setRecipeText -t test_edit_recipe_private_branch test_sourcepackagerecipe
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/model/sourcepackagerecipedata.py
lib/lp/code/errors.py
lib/lp/code/model/tests/test_sourcepackagerecipe.py
lib/lp/testing/factory.py
lib/lp/code/browser/sourcepackagerecipe.py
lib/lp/code/browser/tests/test_sourcepackagerecipe.py
./lib/lp/code/model/sourcepackagerecipedata.py
154: E202 whitespace before ')'
./lib/lp/code/model/tests/test_sourcepackagerecipe.py
255: E231 missing whitespace after ','
279: E231 missing whitespace after ','
287: E231 missing whitespace after ','
294: E231 missing whitespace after ','
302: E231 missing whitespace after ','
328: E231 missing whitespace after ','
388: E231 missing whitespace after ','
685: Line exceeds 78 characters.
./lib/lp/testing/factory.py
160: 'logout' imported but unused
846: E231 missing whitespace after ','
2752: E231 missing whitespace after ','
2754: E302 expected 2 blank lines, found 1
2781: E301 expected 1 blank line, found 0
1052: Line exceeds 78 characters.
2755: Line exceeds 78 characters.
./lib/lp/code/browser/sourcepackagerecipe.py
133: E231 missing whitespace after ','
242: E202 whitespace before ']'
354: E231 missing whitespace after ','
386: E303 too many blank lines (3)
./lib/lp/code/browser/tests/test_sourcepackagerecipe.py
244: E231 missing whitespace after ','
--
https://code.launchpad.net/~abentley/launchpad/reject-private-recipe/+merge/31171
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/reject-private-recipe into lp:launchpad/devel.
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2010-07-23 03:53:46 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2010-07-28 15:52:48 +0000
@@ -34,8 +34,8 @@
LaunchpadView, Link, Navigation, NavigationMenu, stepthrough, structured)
from canonical.launchpad.webapp.breadcrumb import Breadcrumb
from canonical.widgets.itemswidgets import LabeledMultiCheckBoxWidget
-from lp.code.errors import ForbiddenInstruction
-from lp.code.errors import BuildAlreadyPending
+from lp.code.errors import (
+ BuildAlreadyPending, ForbiddenInstruction, PrivateBranchRecipe)
from lp.code.interfaces.branch import NoSuchBranch
from lp.code.interfaces.sourcepackagerecipe import (
ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
@@ -316,6 +316,9 @@
self.setFieldError(
'recipe_text', '%s is not a branch on Launchpad.' % e.name)
return
+ except PrivateBranchRecipe, e:
+ self.setFieldError('recipe_text', str(e))
+ return
self.next_url = canonical_url(source_package_recipe)
@@ -371,9 +374,13 @@
# XXX: bug=592513 We shouldn't be hardcoding "run" here.
self.setFieldError(
'recipe_text',
- 'The bzr-builder instruction "run" is not permitted here.'
- )
- return
+ 'The bzr-builder instruction "run" is not permitted'
+ ' here.')
+ return
+ except PrivateBranchRecipe, e:
+ self.setFieldError('recipe_text', str(e))
+ return
+
distros = data.pop('distros')
=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-26 06:21:45 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-07-28 15:52:48 +0000
@@ -28,7 +28,8 @@
from lp.code.interfaces.sourcepackagerecipe import MINIMAL_RECIPE_TEXT
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.soyuz.model.processor import ProcessorFamily
-from lp.testing import ANONYMOUS, BrowserTestCase, login, logout
+from lp.testing import (
+ ANONYMOUS, BrowserTestCase, login, logout, person_logged_in)
from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
@@ -303,6 +304,18 @@
get_message_text(browser, 2),
'There is already a recipe owned by Master Chef with this name.')
+ def test_create_recipe_private_branch(self):
+ # If a user tries to create source package recipe with a private
+ # base branch, they should get an error.
+ branch = self.factory.makeAnyBranch(private=True, owner=self.user)
+ with person_logged_in(self.user):
+ bzr_identity = branch.bzr_identity
+ recipe_text = MINIMAL_RECIPE_TEXT % bzr_identity
+ browser = self.createRecipe(recipe_text)
+ self.assertEqual(
+ get_message_text(browser, 2),
+ 'Recipe may not refer to private branch: %s' % bzr_identity)
+
class TestSourcePackageRecipeEditView(TestCaseForRecipe):
"""Test the editing behaviour of a source package recipe."""
@@ -475,6 +488,21 @@
self.assertTextMatchesExpressionIgnoreWhitespace(
pattern, main_text)
+ def test_edit_recipe_private_branch(self):
+ # If a user tries to set source package recipe to use a private
+ # branch, they should get an error.
+ recipe = self.factory.makeSourcePackageRecipe(owner=self.user)
+ branch = self.factory.makeAnyBranch(private=True, owner=self.user)
+ with person_logged_in(self.user):
+ bzr_identity = branch.bzr_identity
+ recipe_text = MINIMAL_RECIPE_TEXT % bzr_identity
+ browser = self.getViewBrowser(recipe, '+edit')
+ browser.getControl('Recipe text').value = recipe_text
+ browser.getControl('Update Recipe').click()
+ self.assertEqual(
+ get_message_text(browser, 1),
+ 'Recipe may not refer to private branch: %s' % bzr_identity)
+
class TestSourcePackageRecipeView(TestCaseForRecipe):
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2010-07-20 12:35:09 +0000
+++ lib/lp/code/errors.py 2010-07-28 15:52:48 +0000
@@ -16,6 +16,7 @@
'ClaimReviewFailed',
'ForbiddenInstruction',
'InvalidBranchMergeProposal',
+ 'PrivateBranchRecipe',
'ReviewNotPending',
'TooManyBuilds',
'TooNewRecipeFormat',
@@ -53,6 +54,16 @@
webservice_error(400) #Bad request.
+class PrivateBranchRecipe(Exception):
+
+ def __init__(self, branch):
+ message = (
+ 'Recipe may not refer to private branch: %s' %
+ branch.bzr_identity)
+ self.branch = branch
+ Exception.__init__(self, message)
+
+
class ReviewNotPending(Exception):
"""The requested review is not in a pending state."""
=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py 2010-06-17 20:55:11 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py 2010-07-28 15:52:48 +0000
@@ -27,7 +27,8 @@
from canonical.database.enumcol import EnumCol
from canonical.launchpad.interfaces.lpstorm import IStore
-from lp.code.errors import ForbiddenInstruction, TooNewRecipeFormat
+from lp.code.errors import (
+ ForbiddenInstruction, PrivateBranchRecipe, TooNewRecipeFormat)
from lp.code.model.branch import Branch
from lp.code.interfaces.branch import NoSuchBranch
from lp.code.interfaces.branchlookup import IBranchLookup
@@ -147,7 +148,7 @@
SourcePackageRecipeData.base_branch == branch),
Select(
SourcePackageRecipeData.sourcepackage_recipe_id,
- And (
+ And(
_SourcePackageRecipeDataInstruction.recipe_data_id ==
SourcePackageRecipeData.id,
_SourcePackageRecipeDataInstruction.branch == branch)
@@ -203,6 +204,8 @@
raise ForbiddenInstruction(str(instruction))
db_branch = getUtility(IBranchLookup).getByUrl(
instruction.recipe_branch.url)
+ if db_branch.private:
+ raise PrivateBranchRecipe(db_branch)
r[instruction.recipe_branch.url] = db_branch
r.update(self._scanInstructions(instruction.recipe_branch))
return r
@@ -245,6 +248,8 @@
self.instructions.find().remove()
branch_lookup = getUtility(IBranchLookup)
base_branch = branch_lookup.getByUrl(builder_recipe.url)
+ if base_branch.private:
+ raise PrivateBranchRecipe(base_branch)
if base_branch is None:
raise NoSuchBranch(builder_recipe.url)
if builder_recipe.revspec is not None:
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-07-22 08:39:05 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2010-07-28 15:52:48 +0000
@@ -32,8 +32,8 @@
from lp.buildmaster.interfaces.buildqueue import IBuildQueue
from lp.buildmaster.model.buildqueue import BuildQueue
from lp.code.errors import (
- BuildAlreadyPending, ForbiddenInstruction, TooManyBuilds,
- TooNewRecipeFormat)
+ BuildAlreadyPending, ForbiddenInstruction, PrivateBranchRecipe,
+ TooManyBuilds, TooNewRecipeFormat)
from lp.code.interfaces.sourcepackagerecipe import (
ISourcePackageRecipe, ISourcePackageRecipeSource, MINIMAL_RECIPE_TEXT)
from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -72,25 +72,64 @@
registrant=registrant, owner=owner, distroseries=[distroseries],
name=name, description=description, builder_recipe=builder_recipe)
+ def makeRecipeComponents(self, branches=()):
+ """Return a dict of values that can be used to make a recipe.
+
+ Suggested use: provide as kwargs to ISourcePackageRecipeSource.new
+ :param branches: The list of branches to use in the recipe. (If
+ unspecified, a branch will be autogenerated.
+ """
+ registrant = self.factory.makePerson()
+ return dict(
+ registrant=registrant,
+ owner = self.factory.makeTeam(owner=registrant),
+ distroseries = [self.factory.makeDistroSeries()],
+ name = self.factory.getUniqueString(u'recipe-name'),
+ description = self.factory.getUniqueString(u'recipe-description'),
+ builder_recipe = self.factory.makeRecipe(*branches))
+
def test_creation(self):
# The metadata supplied when a SourcePackageRecipe is created is
# present on the new object.
- registrant = self.factory.makePerson()
- owner = self.factory.makeTeam(owner=registrant)
- distroseries = self.factory.makeDistroSeries()
- name = self.factory.getUniqueString(u'recipe-name')
- description = self.factory.getUniqueString(u'recipe-description')
- builder_recipe = self.factory.makeRecipe()
- recipe = getUtility(ISourcePackageRecipeSource).new(
- registrant=registrant, owner=owner, distroseries=[distroseries],
- name=name, description=description, builder_recipe=builder_recipe)
+ components = self.makeRecipeComponents()
+ recipe = getUtility(ISourcePackageRecipeSource).new(**components)
transaction.commit()
self.assertEquals(
- (registrant, owner, set([distroseries]), name),
+ (components['registrant'], components['owner'],
+ set(components['distroseries']), components['name']),
(recipe.registrant, recipe.owner, set(recipe.distroseries),
recipe.name))
self.assertEqual(True, recipe.is_stale)
+ def test_creation_private_base_branch(self):
+ """An exception should be raised if the base branch is private."""
+ owner = self.factory.makePerson()
+ with person_logged_in(owner):
+ branch = self.factory.makeAnyBranch(private=True, owner=owner)
+ components = self.makeRecipeComponents(branches=[branch])
+ recipe_source = getUtility(ISourcePackageRecipeSource)
+ e = self.assertRaises(
+ PrivateBranchRecipe, recipe_source.new, **components)
+ self.assertEqual(
+ 'Recipe may not refer to private branch: %s' %
+ branch.bzr_identity, str(e))
+
+ def test_creation_private_referenced_branch(self):
+ """An exception should be raised if a referenced branch is private."""
+ owner = self.factory.makePerson()
+ with person_logged_in(owner):
+ base_branch = self.factory.makeAnyBranch(owner=owner)
+ referenced_branch = self.factory.makeAnyBranch(
+ private=True, owner=owner)
+ branches = [base_branch, referenced_branch]
+ components = self.makeRecipeComponents(branches=branches)
+ recipe_source = getUtility(ISourcePackageRecipeSource)
+ e = self.assertRaises(
+ PrivateBranchRecipe, recipe_source.new, **components)
+ self.assertEqual(
+ 'Recipe may not refer to private branch: %s' %
+ referenced_branch.bzr_identity, str(e))
+
def test_exists(self):
# Test ISourcePackageRecipeSource.exists
recipe = self.factory.makeSourcePackageRecipe()
@@ -691,6 +730,35 @@
recipe.setRecipeText(recipe_text=recipe_text2)
self.assertEqual(recipe_text2, recipe.recipe_text)
+ def test_setRecipeText_private_base_branch(self):
+ source_package_recipe = self.factory.makeSourcePackageRecipe()
+ with person_logged_in(source_package_recipe.owner):
+ branch = self.factory.makeAnyBranch(
+ private=True, owner=source_package_recipe.owner)
+ recipe_text = self.factory.makeRecipeText(branch)
+ e = self.assertRaises(
+ PrivateBranchRecipe, source_package_recipe.setRecipeText,
+ recipe_text)
+ self.assertEqual(
+ 'Recipe may not refer to private branch: %s' %
+ branch.bzr_identity, str(e))
+
+ def test_setRecipeText_private_referenced_branch(self):
+ source_package_recipe = self.factory.makeSourcePackageRecipe()
+ with person_logged_in(source_package_recipe.owner):
+ base_branch = self.factory.makeAnyBranch(
+ owner=source_package_recipe.owner)
+ referenced_branch = self.factory.makeAnyBranch(
+ private=True, owner=source_package_recipe.owner)
+ recipe_text = self.factory.makeRecipeText(
+ base_branch, referenced_branch)
+ e = self.assertRaises(
+ PrivateBranchRecipe, source_package_recipe.setRecipeText,
+ recipe_text)
+ self.assertEqual(
+ 'Recipe may not refer to private branch: %s' %
+ referenced_branch.bzr_identity, str(e))
+
def test_getRecipe(self):
"""Person.getRecipe returns the named recipe."""
recipe, user = self.makeRecipe()[:-1]
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-07-27 05:05:19 +0000
+++ lib/lp/testing/factory.py 2010-07-28 15:52:48 +0000
@@ -36,6 +36,7 @@
from threading import local
from types import InstanceType
+from bzrlib.plugins.builder.recipe import BaseRecipeBranch
import pytz
from twisted.python.util import mergeFunctionMetadata
@@ -2747,7 +2748,8 @@
# security wrappers for them, as well as for objects created by
# other Python libraries.
unwrapped_types = (
- DSCFile, InstanceType, MergeDirective2, Message, datetime, int, str, unicode)
+ BaseRecipeBranch, DSCFile, InstanceType, MergeDirective2, Message,
+ datetime, int, str, unicode,)
def is_security_proxied_or_harmless(obj):
"""Check that the given object is security wrapped or a harmless object."""
