launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/recipe-ambiguous-vcs into lp:launchpad.

Commit message:
Handle the case where a Bazaar branch and a Git repository have the same identity URL when creating a recipe.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1623924 in Launchpad itself: "Source package recipes prefer Bazaar when lp:$foo alias is VCS-ambiguous"
  https://bugs.launchpad.net/launchpad/+bug/1623924

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/recipe-ambiguous-vcs/+merge/350699

There are still various bodges because we're using bzr-builder to parse git-build-recipe recipes, but passing through the type should smooth over the rough edges.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/recipe-ambiguous-vcs into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2018-01-02 16:10:26 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2018-07-23 19:40:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the source package recipe view classes and templates."""
@@ -551,12 +551,9 @@
         # branch location, they should get an error.
         browser = self.createRecipe(
             self.minimal_recipe_text.splitlines()[0] + '\nfoo\n')
-        # This page doesn't know whether the user was aiming for a Bazaar
-        # branch or a Git repository; the error message always says
-        # "branch".
         self.assertEqual(
             get_feedback_messages(browser.contents)[1],
-            'foo is not a branch on Launchpad.')
+            'foo %s' % self.no_such_object_message)
 
     def test_create_recipe_bad_instruction_branch(self):
         # If a user tries to create source package recipe with a bad

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2017-06-16 10:02:47 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2018-07-23 19:40:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface of the `SourcePackageRecipe` content type."""
@@ -15,11 +15,16 @@
     'ISourcePackageRecipeSource',
     'MINIMAL_RECIPE_TEXT_BZR',
     'MINIMAL_RECIPE_TEXT_GIT',
+    'RecipeBranchType',
     ]
 
 
 from textwrap import dedent
 
+from lazr.enum import (
+    EnumeratedType,
+    Item,
+    )
 from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     call_with,
@@ -105,13 +110,23 @@
         """An iterator of the branches referenced by this recipe."""
 
 
+class RecipeBranchType(EnumeratedType):
+    """The revision control system used for a recipe."""
+
+    BZR = Item("Bazaar")
+
+    GIT = Item("Git")
+
+
 class IRecipeBranchSource(Interface):
 
     def getParsedRecipe(recipe_text):
         """Parse recipe text into recipe data.
 
         :param recipe_text: Recipe text as a string.
-        :return: a `RecipeBranch` representing the recipe.
+        :return: a tuple of a `RecipeBranch` representing the recipe and a
+            `RecipeBranchType` indicating the revision control system to be
+            used for the recipe.
         """
 
 

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2018-05-09 09:23:36 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2018-07-23 19:40:00 +0000
@@ -184,8 +184,9 @@
             owner_ids, need_validity=True))
 
     def setRecipeText(self, recipe_text):
-        parsed = getUtility(IRecipeBranchSource).getParsedRecipe(recipe_text)
-        self._recipe_data.setRecipe(parsed)
+        parsed, recipe_branch_type = (
+            getUtility(IRecipeBranchSource).getParsedRecipe(recipe_text))
+        self._recipe_data.setRecipe(parsed, recipe_branch_type)
 
     def getRecipeText(self, validate=False):
         """See `ISourcePackageRecipe`."""
@@ -215,9 +216,9 @@
         """See `ISourcePackageRecipeSource.new`."""
         store = IMasterStore(SourcePackageRecipe)
         sprecipe = SourcePackageRecipe()
-        builder_recipe = getUtility(IRecipeBranchSource).getParsedRecipe(
-            recipe)
-        SourcePackageRecipeData(builder_recipe, sprecipe)
+        builder_recipe, recipe_branch_type = (
+            getUtility(IRecipeBranchSource).getParsedRecipe(recipe))
+        SourcePackageRecipeData(builder_recipe, recipe_branch_type, sprecipe)
         sprecipe.registrant = registrant
         sprecipe.owner = owner
         sprecipe.name = name

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2016-08-12 12:56:41 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2018-07-23 19:40:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation code for source package builds."""
@@ -164,8 +164,9 @@
             getUtility(ISourcePackageRecipeDataSource).createManifestFromText(
                 text, self)
         else:
-            self.manifest.setRecipe(
+            parsed, recipe_branch_type = (
                 getUtility(IRecipeBranchSource).getParsedRecipe(text))
+            self.manifest.setRecipe(parsed, recipe_branch_type)
 
     def getManifestText(self):
         if self.manifest is None:

=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2016-02-06 02:20:04 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2018-07-23 19:40:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation of the recipe storage.
@@ -61,6 +61,7 @@
     IRecipeBranchSource,
     ISourcePackageRecipeData,
     ISourcePackageRecipeDataSource,
+    RecipeBranchType,
     )
 from lp.code.model.branch import Branch
 from lp.code.model.gitrepository import GitRepository
@@ -239,10 +240,14 @@
         # formats are mostly compatible, the header line must say
         # "bzr-builder" even though git-build-recipe also supports its own
         # name there.
-        recipe_text = re.sub(
+        recipe_text, git_substitutions = re.subn(
             r"^(#\s*)git-build-recipe", r"\1bzr-builder", recipe_text)
+        recipe_branch_type = (
+            RecipeBranchType.GIT if git_substitutions
+            else RecipeBranchType.BZR)
         parser = RecipeParser(recipe_text)
-        return parser.parse(permitted_instructions=SAFE_INSTRUCTIONS)
+        recipe_branch = parser.parse(permitted_instructions=SAFE_INSTRUCTIONS)
+        return recipe_branch, recipe_branch_type
 
     @staticmethod
     def findRecipes(branch_or_repository, revspecs=None):
@@ -306,9 +311,10 @@
     @classmethod
     def createManifestFromText(cls, text, sourcepackage_recipe_build):
         """See `ISourcePackageRecipeDataSource`."""
-        parsed = cls.getParsedRecipe(text)
+        parsed, recipe_branch_type = cls.getParsedRecipe(text)
         return cls(
-            parsed, sourcepackage_recipe_build=sourcepackage_recipe_build)
+            parsed, recipe_branch_type,
+            sourcepackage_recipe_build=sourcepackage_recipe_build)
 
     def getRecipe(self):
         """The BaseRecipeBranch version of the recipe."""
@@ -388,26 +394,26 @@
                 instruction.recipe_branch, insn, branch_map, line_number)
         return line_number
 
-    def setRecipe(self, builder_recipe):
+    def setRecipe(self, builder_recipe, recipe_branch_type):
         """Convert the BaseRecipeBranch `builder_recipe` to the db form."""
         clear_property_cache(self)
         if builder_recipe.format > MAX_RECIPE_FORMAT:
             raise TooNewRecipeFormat(builder_recipe.format, MAX_RECIPE_FORMAT)
-        base = getUtility(IBranchLookup).getByUrl(builder_recipe.url)
-        if base is None:
+        if recipe_branch_type == RecipeBranchType.BZR:
+            base = getUtility(IBranchLookup).getByUrl(builder_recipe.url)
+            if base is None:
+                raise NoSuchBranch(builder_recipe.url)
+            elif base.private:
+                raise PrivateBranchRecipe(base)
+        elif recipe_branch_type == RecipeBranchType.GIT:
             base = getUtility(IGitLookup).getByUrl(builder_recipe.url)
             if base is None:
-                # If possible, try to raise an exception consistent with
-                # whether the current recipe is Bazaar-based or Git-based,
-                # so that error messages make more sense.
-                if self.base_git_repository is not None:
-                    raise NoSuchGitRepository(builder_recipe.url)
-                else:
-                    raise NoSuchBranch(builder_recipe.url)
+                raise NoSuchGitRepository(builder_recipe.url)
             elif base.private:
                 raise PrivateGitRepositoryRecipe(base)
-        elif base.private:
-            raise PrivateBranchRecipe(base)
+        else:
+            raise AssertionError(
+                'Unknown recipe_branch_type: %r' % recipe_branch_type)
         branch_map = self._scanInstructions(base, builder_recipe)
         # If this object hasn't been added to a store yet, there can't be any
         # instructions linking to us yet.
@@ -426,12 +432,12 @@
             self.deb_version_template = unicode(builder_recipe.deb_version)
         self.recipe_format = unicode(builder_recipe.format)
 
-    def __init__(self, recipe, sourcepackage_recipe=None,
+    def __init__(self, recipe, recipe_branch_type, sourcepackage_recipe=None,
                  sourcepackage_recipe_build=None):
         """Initialize from the bzr-builder recipe and link it to a db recipe.
         """
         super(SourcePackageRecipeData, self).__init__()
-        self.setRecipe(recipe)
+        self.setRecipe(recipe, recipe_branch_type)
         self.sourcepackage_recipe = sourcepackage_recipe
         self.sourcepackage_recipe_build = sourcepackage_recipe_build
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2018-03-02 01:11:48 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2018-07-23 19:40:00 +0000
@@ -37,6 +37,8 @@
     PrivateGitRepositoryRecipe,
     TooNewRecipeFormat,
     )
+from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe,
     ISourcePackageRecipeSource,
@@ -1098,12 +1100,47 @@
 
 class TestRecipeBranchRoundTrippingBzr(
     TestRecipeBranchRoundTrippingMixin, BzrMixin, TestCaseWithFactory):
-    pass
+
+    def test_builds_recipe_with_ambiguous_git_repository(self):
+        # Arrange for Bazaar and Git prefixes to match.
+        self.pushConfig('codehosting', bzr_lp_prefix='lp:', lp_url_hosts='')
+        project = self.base_branch.product
+        repository = self.factory.makeGitRepository(target=project)
+        with person_logged_in(project.owner):
+            ICanHasLinkedBranch(project).setBranch(self.base_branch)
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                project, repository)
+        clear_property_cache(self.base_branch)
+        recipe_text = '''\
+        # %(recipe_id)s format 0.3 deb-version 0.1-{revno}
+        %(base)s
+        ''' % {'recipe_id': self.recipe_id, 'base': self.base_branch.identity}
+        recipe = self.get_recipe(recipe_text)
+        self.assertEqual(self.base_branch, recipe.base_branch)
 
 
 class TestRecipeBranchRoundTrippingGit(
     TestRecipeBranchRoundTrippingMixin, GitMixin, TestCaseWithFactory):
-    pass
+
+    def test_builds_recipe_with_ambiguous_bzr_branch(self):
+        # Arrange for Bazaar and Git prefixes to match.
+        self.pushConfig('codehosting', bzr_lp_prefix='lp:', lp_url_hosts='')
+        project = self.base_branch.target
+        branch = self.factory.makeBranch(product=project)
+        with person_logged_in(project.owner):
+            ICanHasLinkedBranch(project).setBranch(branch)
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                project, self.base_branch.repository)
+        recipe_text = '''\
+        # %(recipe_id)s format 0.3 deb-version 0.1-{revno}
+        %(base)s
+        ''' % {
+            'recipe_id': self.recipe_id,
+            'base': self.base_branch.repository.identity,
+            }
+        recipe = self.get_recipe(recipe_text)
+        self.assertEqual(
+            self.base_branch.repository, recipe.base_git_repository)
 
 
 class RecipeDateLastModified(TestCaseWithFactory):