launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/codeimport-source-details-refactor into lp:launchpad with lp:~cjwatson/launchpad/codeimport-git-auth as a prerequisite.

Commit message:
Refactor CodeImportSourceDetails.fromCodeImport to CodeImportSourceDetails.fromCodeImportJob, to make it easier to issue macaroons in future.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1469459 in Launchpad itself: "import external code into a LP git repo (natively)"
  https://bugs.launchpad.net/launchpad/+bug/1469459

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/codeimport-source-details-refactor/+merge/308117
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/codeimport-source-details-refactor into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/codeimportscheduler.py'
--- lib/lp/code/xmlrpc/codeimportscheduler.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/xmlrpc/codeimportscheduler.py	2016-10-11 13:11:47 +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).
 
 """The code import scheduler XML-RPC API."""
@@ -67,8 +67,8 @@
     @return_fault
     def _getImportDataForJobID(self, job_id):
         job = self._getJob(job_id)
-        arguments = CodeImportSourceDetails.fromCodeImport(
-            job.code_import).asArguments()
+        arguments = CodeImportSourceDetails.fromCodeImportJob(
+            job).asArguments()
         branch = job.code_import.branch
         branch_url = canonical_url(branch)
         log_file_name = '%s.log' % branch.unique_name[1:].replace('/', '-')

=== modified file 'lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py'
--- lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py	2016-10-11 13:11:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test for the methods of `ICodeImportScheduler`."""
@@ -63,8 +63,8 @@
         code_import = removeSecurityProxy(code_import_job).code_import
         code_import_arguments, branch_url, log_file_name = \
             self.api.getImportDataForJobID(code_import_job.id)
-        import_as_arguments = CodeImportSourceDetails.fromCodeImport(
-            code_import).asArguments()
+        import_as_arguments = CodeImportSourceDetails.fromCodeImportJob(
+            code_import_job).asArguments()
         expected_log_file_name = '%s.log' % (
             code_import.branch.unique_name[1:].replace('/', '-'))
         self.assertEqual(

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2016-10-05 14:08:09 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2016-10-11 13:11:47 +0000
@@ -48,6 +48,7 @@
 import subvertpy
 import subvertpy.client
 import subvertpy.ra
+from testtools.matchers import Equals
 
 from lp.app.enums import InformationType
 from lp.code.interfaces.codehosting import (
@@ -1394,70 +1395,64 @@
         # Use an admin user as we aren't checking edit permissions here.
         TestCaseWithFactory.setUp(self, 'admin@xxxxxxxxxxxxx')
 
+    def assertArgumentsMatch(self, code_import, matcher):
+        job = self.factory.makeCodeImportJob(code_import=code_import)
+        details = CodeImportSourceDetails.fromCodeImportJob(job)
+        self.assertThat(details.asArguments(), matcher)
+
     def test_bzr_arguments(self):
         code_import = self.factory.makeCodeImport(
             bzr_branch_url="http://example.com/foo";)
-        arguments = CodeImportSourceDetails.fromCodeImport(
-            code_import).asArguments()
-        self.assertEquals([
-            str(code_import.branch.id), 'bzr', 'http://example.com/foo'],
-            arguments)
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr',
+                'http://example.com/foo']))
 
     def test_git_arguments(self):
         code_import = self.factory.makeCodeImport(
-                git_repo_url="git://git.example.com/project.git")
-        arguments = CodeImportSourceDetails.fromCodeImport(
-            code_import).asArguments()
-        self.assertEquals([
-            str(code_import.branch.id), 'git',
-            'git://git.example.com/project.git'],
-            arguments)
+            git_repo_url="git://git.example.com/project.git")
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'git',
+                'git://git.example.com/project.git']))
 
     def test_cvs_arguments(self):
         code_import = self.factory.makeCodeImport(
             cvs_root=':pserver:foo@xxxxxxxxxxx/bar', cvs_module='bar')
-        arguments = CodeImportSourceDetails.fromCodeImport(
-            code_import).asArguments()
-        self.assertEquals([
-            str(code_import.branch.id), 'cvs',
-            ':pserver:foo@xxxxxxxxxxx/bar', 'bar'],
-            arguments)
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'cvs',
+                ':pserver:foo@xxxxxxxxxxx/bar', 'bar']))
 
     def test_bzr_svn_arguments(self):
         code_import = self.factory.makeCodeImport(
-                svn_branch_url='svn://svn.example.com/trunk')
-        arguments = CodeImportSourceDetails.fromCodeImport(
-            code_import).asArguments()
-        self.assertEquals([
-            str(code_import.branch.id), 'bzr-svn',
-            'svn://svn.example.com/trunk'],
-            arguments)
+            svn_branch_url='svn://svn.example.com/trunk')
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr-svn',
+                'svn://svn.example.com/trunk']))
 
     def test_bzr_stacked(self):
         devfocus = self.factory.makeAnyBranch()
         code_import = self.factory.makeCodeImport(
-                bzr_branch_url='bzr://bzr.example.com/foo',
-                context=devfocus.target.context)
+            bzr_branch_url='bzr://bzr.example.com/foo',
+            context=devfocus.target.context)
         code_import.branch.stacked_on = devfocus
-        details = CodeImportSourceDetails.fromCodeImport(
-            code_import)
-        self.assertEquals([
-            str(code_import.branch.id), 'bzr',
-            'bzr://bzr.example.com/foo',
-            compose_public_url('http', branch_id_alias(devfocus))],
-            details.asArguments())
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr',
+                'bzr://bzr.example.com/foo',
+                compose_public_url('http', branch_id_alias(devfocus))]))
 
     def test_bzr_stacked_private(self):
         # Code imports can't be stacked on private branches.
         devfocus = self.factory.makeAnyBranch(
             information_type=InformationType.USERDATA)
         code_import = self.factory.makeCodeImport(
-                context=devfocus.target.context,
-                bzr_branch_url='bzr://bzr.example.com/foo')
+            context=devfocus.target.context,
+            bzr_branch_url='bzr://bzr.example.com/foo')
         code_import.branch.stacked_on = devfocus
-        details = CodeImportSourceDetails.fromCodeImport(
-            code_import)
-        self.assertEquals([
-            str(code_import.branch.id), 'bzr',
-            'bzr://bzr.example.com/foo'],
-            details.asArguments())
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr',
+                'bzr://bzr.example.com/foo']))

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2015-09-05 14:39:17 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2016-10-11 13:11:47 +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 CodeImportWorkerMonitor and related classes."""
@@ -754,15 +754,15 @@
         returns the job.  It also makes sure there are no branches or foreign
         trees in the default stores to interfere with processing this job.
         """
-        source_details = CodeImportSourceDetails.fromCodeImport(code_import)
-        clean_up_default_stores_for_import(source_details)
-        self.addCleanup(clean_up_default_stores_for_import, source_details)
         if code_import.review_status != CodeImportReviewStatus.REVIEWED:
             code_import.updateFromData(
                 {'review_status': CodeImportReviewStatus.REVIEWED},
                 self.factory.makePerson())
         job = getUtility(ICodeImportJobSet).getJobForMachine('machine', 10)
         self.assertEqual(code_import, job.code_import)
+        source_details = CodeImportSourceDetails.fromCodeImportJob(job)
+        clean_up_default_stores_for_import(source_details)
+        self.addCleanup(clean_up_default_stores_for_import, source_details)
         return job
 
     def assertCodeImportResultCreated(self, code_import):

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2015-06-12 14:20:12 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2016-10-11 13:11:47 +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).
 
 """The code import worker. This imports code from foreign repositories."""
@@ -308,8 +308,9 @@
             branch_id, rcstype, url, cvs_root, cvs_module, stacked_on_url)
 
     @classmethod
-    def fromCodeImport(cls, code_import):
-        """Convert a `CodeImport` to an instance."""
+    def fromCodeImportJob(cls, job):
+        """Convert a `CodeImportJob` to an instance."""
+        code_import = job.code_import
         branch = code_import.branch
         if branch.stacked_on is not None and not branch.stacked_on.private:
             stacked_path = branch_id_alias(branch.stacked_on)