launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-code-import-context into lp:launchpad.

Commit message:
Rearrange CodeImportSet.new to take the branch target context directly rather than via IBranchTarget.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-code-import-context/+merge/307322

IBranchTarget is a slightly peculiar intermediate concept that I chose not to replicate in the Git model: it represents some aspects of the target of a branch but not its owner, and in order to get the combination of both as an IBranchNamespace you need to do IBranchTarget.getNamespace(owner).  For Git, we only have IGitNamespace which serves both functions.

At present, CodeImportSet.new takes an IBranchTarget.  However, we won't easily be able to pass an IGitNamespace to CodeImportSet.new because the owner is a separate parameter that defaults to the registrant, and I don't think it makes sense to push that logic out to all callers.

The simplest resolution for all this is to just pass the underlying branch target context (i.e. a project, source package, or similar) directly.  CodeImportSet.new can convert this to an IBranchTarget itself, and when we add Git-target support we can teach it to convert to IGitNamespace as an alternative.

Based on this, it might well be possible to fold IBranchTarget into IBranchNamespace at some later point, which would be a nice simplification.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-code-import-context into lp:launchpad.

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2015-10-29 14:17:21 +0000
+++ lib/lp/code/browser/codeimport.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for CodeImports."""
@@ -63,7 +63,6 @@
     get_branch_namespace,
     IBranchNamespacePolicy,
     )
-from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codeimport import (
     ICodeImport,
     ICodeImportSet,
@@ -363,7 +362,7 @@
         return getUtility(ICodeImportSet).new(
             registrant=self.user,
             owner=data['owner'],
-            target=IBranchTarget(product),
+            context=product,
             branch_name=data['branch_name'],
             rcs_type=data['rcs_type'],
             url=url,

=== modified file 'lib/lp/code/doc/codeimport.txt'
--- lib/lp/code/doc/codeimport.txt	2015-09-11 12:20:23 +0000
+++ lib/lp/code/doc/codeimport.txt	2016-09-30 13:56:19 +0000
@@ -21,7 +21,6 @@
 CodeImports are created and found using the ICodeImportSet interface,
 which is registered as a utility.
 
-    >>> from lp.code.interfaces.branchtarget import IBranchTarget
     >>> from lp.code.interfaces.codeimport import ICodeImport, ICodeImportSet
     >>> from zope.component import getUtility
     >>> from zope.security.proxy import removeSecurityProxy
@@ -74,9 +73,9 @@
     >>> cvs = RevisionControlSystems.CVS
     >>> cvs_root = ':pserver:anonymous@xxxxxxxxxxxxxxx:/cvsroot'
     >>> cvs_module = 'hello'
-    >>> target = IBranchTarget(factory.makeProduct(name='widget'))
+    >>> context = factory.makeProduct(name='widget')
     >>> cvs_import = code_import_set.new(
-    ...     registrant=nopriv, target=target, branch_name='trunk-cvs',
+    ...     registrant=nopriv, context=context, branch_name='trunk-cvs',
     ...     rcs_type=cvs, cvs_root=cvs_root, cvs_module=cvs_module)
     >>> verifyObject(ICodeImport, removeSecurityProxy(cvs_import))
     True
@@ -136,7 +135,7 @@
     >>> svn = RevisionControlSystems.BZR_SVN
     >>> svn_url = 'svn://svn.example.com/trunk'
     >>> svn_import = code_import_set.new(
-    ...     registrant=nopriv, target=target, branch_name='trunk-svn',
+    ...     registrant=nopriv, context=context, branch_name='trunk-svn',
     ...     rcs_type=svn, url=svn_url)
     >>> verifyObject(ICodeImport, removeSecurityProxy(svn_import))
     True
@@ -164,7 +163,7 @@
     >>> git = RevisionControlSystems.GIT
     >>> git_url = 'git://git.example.com/hello.git'
     >>> git_import = code_import_set.new(
-    ...     registrant=nopriv, target=target, branch_name='trunk-git',
+    ...     registrant=nopriv, context=context, branch_name='trunk-git',
     ...     rcs_type=git, url=git_url)
     >>> verifyObject(ICodeImport, removeSecurityProxy(git_import))
     True

=== modified file 'lib/lp/code/interfaces/codeimport.py'
--- lib/lp/code/interfaces/codeimport.py	2015-09-28 17:38:45 +0000
+++ lib/lp/code/interfaces/codeimport.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Code import interfaces."""
@@ -220,12 +220,12 @@
 class ICodeImportSet(Interface):
     """Interface representing the set of code imports."""
 
-    def new(registrant, target, branch_name, rcs_type, url=None,
+    def new(registrant, context, branch_name, rcs_type, url=None,
             cvs_root=None, cvs_module=None, review_status=None,
             owner=None):
         """Create a new CodeImport.
 
-        :param target: An `IBranchTarget` that the code is associated with.
+        :param context: An `IHasCodeImports` that the code is associated with.
         :param owner: The `IPerson` to set as the owner of the branch, or
             None to use registrant. registrant must be a member of owner to
             do this.

=== modified file 'lib/lp/code/model/branchtarget.py'
--- lib/lp/code/model/branchtarget.py	2016-01-12 13:43:34 +0000
+++ lib/lp/code/model/branchtarget.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Branch targets."""
@@ -43,7 +43,7 @@
                       cvs_root=None, cvs_module=None, owner=None):
         """See `IBranchTarget`."""
         return getUtility(ICodeImportSet).new(
-            registrant, self, branch_name, rcs_type, url=url,
+            registrant, self.context, branch_name, rcs_type, url=url,
             cvs_root=cvs_root, cvs_module=cvs_module, owner=owner)
 
     def getRelatedSeriesBranchInfo(self, parent_branch, limit_results=None):

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/codeimport.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database classes including and related to CodeImport."""
@@ -46,6 +46,7 @@
     CodeImportAlreadyRunning,
     CodeImportNotInReviewedState,
     )
+from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codeimport import (
     ICodeImport,
     ICodeImportSet,
@@ -232,7 +233,7 @@
 class CodeImportSet:
     """See `ICodeImportSet`."""
 
-    def new(self, registrant, target, branch_name, rcs_type,
+    def new(self, registrant, context, branch_name, rcs_type,
             url=None, cvs_root=None, cvs_module=None, review_status=None,
             owner=None):
         """See `ICodeImportSet`."""
@@ -246,6 +247,7 @@
             raise AssertionError(
                 "Don't know how to sanity check source details for unknown "
                 "rcs_type %s" % rcs_type)
+        target = IBranchTarget(context)
         if review_status is None:
             # Auto approve imports.
             review_status = CodeImportReviewStatus.REVIEWED

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2015-10-09 15:49:14 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IBranchNamespace` implementations."""
@@ -358,7 +358,7 @@
         product = self.factory.makeProduct()
         name = self.factory.getUniqueString()
         code_import = self.factory.makeCodeImport(
-            registrant=owner, target=IBranchTarget(product), branch_name=name)
+            registrant=owner, context=product, branch_name=name)
         branch = code_import.branch
         new_name = self.factory.getUniqueString()
         namespace = ProjectBranchNamespace(owner, product)
@@ -371,7 +371,7 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct()
         code_import = self.factory.makeCodeImport(
-            registrant=owner, target=IBranchTarget(product))
+            registrant=owner, context=product)
         branch = code_import.branch
         new_owner = self.factory.makePerson()
         new_namespace = ProjectBranchNamespace(new_owner, product)

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2014-02-24 07:19:52 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for methods of CodeImport and CodeImportSet."""
@@ -60,7 +60,7 @@
         """A subversion import can use the svn:// scheme."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='imported',
             rcs_type=RevisionControlSystems.BZR_SVN,
             url=self.factory.getUniqueURL(scheme="svn"))
@@ -74,7 +74,7 @@
         """A specific review status can be set for a new import."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='imported',
             rcs_type=RevisionControlSystems.BZR_SVN,
             url=self.factory.getUniqueURL(),
@@ -89,7 +89,7 @@
         """A new CVS code import should have REVIEWED status."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='imported',
             rcs_type=RevisionControlSystems.CVS,
             cvs_root=self.factory.getUniqueURL(),
@@ -105,7 +105,7 @@
         """A git import can have a git:// style URL."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(scheme="git"),
@@ -120,7 +120,7 @@
         """A new git import is always reviewed by default."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(),
@@ -135,7 +135,7 @@
         """A new bzr import is always reviewed by default."""
         code_import = CodeImportSet().new(
             registrant=self.factory.makePerson(),
-            target=IBranchTarget(self.factory.makeProduct()),
+            context=self.factory.makeProduct(),
             branch_name='mirrored',
             rcs_type=RevisionControlSystems.BZR,
             url=self.factory.getUniqueURL(),
@@ -151,7 +151,7 @@
         registrant = self.factory.makePerson()
         self.assertRaises(AssertionError, CodeImportSet().new,
             registrant=registrant,
-            target=IBranchTarget(registrant),
+            context=registrant,
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(),
@@ -161,10 +161,9 @@
         """Test that we can create an import targetting a source package."""
         registrant = self.factory.makePerson()
         source_package = self.factory.makeSourcePackage()
-        target = IBranchTarget(source_package)
         code_import = CodeImportSet().new(
             registrant=registrant,
-            target=target,
+            context=source_package,
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(),
@@ -172,7 +171,8 @@
         code_import = removeSecurityProxy(code_import)
         self.assertEqual(registrant, code_import.registrant)
         self.assertEqual(registrant, code_import.branch.owner)
-        self.assertEqual(target, code_import.branch.target)
+        self.assertEqual(
+            IBranchTarget(source_package), code_import.branch.target)
         self.assertEqual(source_package, code_import.branch.sourcepackage)
         # And a job is still created
         self.assertIsNot(None, code_import.import_job)
@@ -183,10 +183,9 @@
         owner = self.factory.makeTeam()
         removeSecurityProxy(registrant).join(owner)
         source_package = self.factory.makeSourcePackage()
-        target = IBranchTarget(source_package)
         code_import = CodeImportSet().new(
             registrant=registrant,
-            target=target,
+            context=source_package,
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(),
@@ -195,7 +194,8 @@
         self.assertEqual(registrant, code_import.registrant)
         self.assertEqual(owner, code_import.branch.owner)
         self.assertEqual(registrant, code_import.branch.registrant)
-        self.assertEqual(target, code_import.branch.target)
+        self.assertEqual(
+            IBranchTarget(source_package), code_import.branch.target)
         self.assertEqual(source_package, code_import.branch.sourcepackage)
         # And a job is still created
         self.assertIsNot(None, code_import.import_job)
@@ -205,12 +205,11 @@
         registrant = self.factory.makePerson()
         owner = self.factory.makeTeam()
         source_package = self.factory.makeSourcePackage()
-        target = IBranchTarget(source_package)
         self.assertRaises(
             BranchCreatorNotMemberOfOwnerTeam,
             CodeImportSet().new,
             registrant=registrant,
-            target=target,
+            context=source_package,
             branch_name='imported',
             rcs_type=RevisionControlSystems.GIT,
             url=self.factory.getUniqueURL(),

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2015-06-12 14:20:12 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the code import worker."""
@@ -1435,7 +1435,7 @@
         devfocus = self.factory.makeAnyBranch()
         code_import = self.factory.makeCodeImport(
                 bzr_branch_url='bzr://bzr.example.com/foo',
-                target=devfocus.target)
+                context=devfocus.target.context)
         code_import.branch.stacked_on = devfocus
         details = CodeImportSourceDetails.fromCodeImport(
             code_import)
@@ -1450,7 +1450,7 @@
         devfocus = self.factory.makeAnyBranch(
             information_type=InformationType.USERDATA)
         code_import = self.factory.makeCodeImport(
-                target=devfocus.target,
+                context=devfocus.target.context,
                 bzr_branch_url='bzr://bzr.example.com/foo')
         code_import.branch.stacked_on = devfocus
         details = CodeImportSourceDetails.fromCodeImport(

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2016-09-19 11:47:33 +0000
+++ lib/lp/registry/browser/product.py	2016-09-30 13:56:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for products."""
@@ -149,7 +149,6 @@
 from lp.code.errors import BranchExists
 from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
-from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codeimport import (
     ICodeImport,
     ICodeImportSet,
@@ -1932,11 +1931,6 @@
         else:
             raise AssertionError("Unknown branch type %s" % branch_type)
 
-    @property
-    def target(self):
-        """The branch target for the context."""
-        return IBranchTarget(self.context)
-
     def abort_update(self):
         """Abort transaction.
 
@@ -1991,7 +1985,7 @@
                     code_import = getUtility(ICodeImportSet).new(
                         owner=branch_owner,
                         registrant=self.user,
-                        target=IBranchTarget(self.target),
+                        context=self.context,
                         branch_name=branch_name,
                         rcs_type=rcs_item,
                         url=url,

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2016-07-15 17:11:09 +0000
+++ lib/lp/testing/factory.py	2016-09-30 13:56:19 +0000
@@ -117,7 +117,6 @@
 from lp.code.errors import UnknownBranchTypeError
 from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchnamespace import get_branch_namespace
-from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
@@ -2359,18 +2358,16 @@
         """Make a code import targetting a sourcepackage."""
         if sourcepackage is None:
             sourcepackage = self.makeSourcePackage()
-        target = IBranchTarget(sourcepackage)
-        return self.makeCodeImport(target=target, **kwargs)
+        return self.makeCodeImport(context=sourcepackage, **kwargs)
 
     def makeProductCodeImport(self, product=None, **kwargs):
         """Make a code import targetting a product."""
         if product is None:
             product = self.makeProduct()
-        target = IBranchTarget(product)
-        return self.makeCodeImport(target=target, **kwargs)
+        return self.makeCodeImport(context=product, **kwargs)
 
     def makeCodeImport(self, svn_branch_url=None, cvs_root=None,
-                       cvs_module=None, target=None, branch_name=None,
+                       cvs_module=None, context=None, branch_name=None,
                        git_repo_url=None,
                        bzr_branch_url=None, registrant=None,
                        rcs_type=None, review_status=None):
@@ -2384,8 +2381,8 @@
             bzr_branch_url is None):
             svn_branch_url = self.getUniqueURL()
 
-        if target is None:
-            target = IBranchTarget(self.makeProduct())
+        if context is None:
+            context = self.makeProduct()
         if branch_name is None:
             branch_name = self.getUniqueString('name')
         if registrant is None:
@@ -2395,24 +2392,24 @@
         if svn_branch_url is not None:
             assert rcs_type in (None, RevisionControlSystems.BZR_SVN)
             return code_import_set.new(
-                registrant, target, branch_name,
+                registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.BZR_SVN,
                 url=svn_branch_url, review_status=review_status)
         elif git_repo_url is not None:
             assert rcs_type in (None, RevisionControlSystems.GIT)
             return code_import_set.new(
-                registrant, target, branch_name,
+                registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.GIT,
                 url=git_repo_url, review_status=review_status)
         elif bzr_branch_url is not None:
             return code_import_set.new(
-                registrant, target, branch_name,
+                registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.BZR,
                 url=bzr_branch_url, review_status=review_status)
         else:
             assert rcs_type in (None, RevisionControlSystems.CVS)
             return code_import_set.new(
-                registrant, target, branch_name,
+                registrant, context, branch_name,
                 rcs_type=RevisionControlSystems.CVS,
                 cvs_root=cvs_root, cvs_module=cvs_module,
                 review_status=review_status)