Merge lp:~cjwatson/launchpad/decouple-code-import-worker-arguments into lp:launchpad

[Colin Watson]

Move code import worker argument construction into CodeImportJob.

This can only be used from the main application side anyway because it talks directly to the database, so it's much less confusing to have this in the main application code. Moving this will also help us to split codeimport out to a separate codebase.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/interfaces/codeimportjob.py'
--- lib/lp/code/interfaces/codeimportjob.py	2013-01-07 02:40:55 +0000
+++ lib/lp/code/interfaces/codeimportjob.py	2018-03-15 21:30:48 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 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).
 
 """Interfaces and enumeratrions for CodeImportJobs.
@@ -97,6 +97,9 @@
         to the time of the current transaction.
         """
 
+    def makeWorkerArguments():
+        """Return a list of worker arguments for this job."""
+
 
 class ICodeImportJobSet(Interface):
     """The set of pending and active code import jobs."""

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2016-10-20 09:34:02 +0000
+++ lib/lp/code/model/codeimportjob.py	2018-03-15 21:30:48 +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).
 
 """Database classes for the CodeImportJob table."""
@@ -31,6 +31,12 @@
     CodeImportMachineState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    RevisionControlSystems,
+    )
+from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.codehosting import (
+    branch_id_alias,
+    compose_public_url,
     )
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import (
@@ -104,6 +110,61 @@
             "id = %s AND date_due <= %s" % sqlvalues(self.id, UTC_NOW))
         return import_job is not None
 
+    def makeWorkerArguments(self):
+        """See `ICodeImportJob`."""
+        # Keep this in sync with CodeImportSourceDetails.fromArguments.
+        code_import = self.code_import
+        target = code_import.target
+
+        if IBranch.providedBy(target):
+            target_id = target.id
+        else:
+            # We don't have a better way to identify the target repository
+            # than the mutable unique name, but the macaroon constrains
+            # pushes tightly enough that the worst case is an authentication
+            # failure.
+            target_id = target.unique_name
+
+        if code_import.rcs_type == RevisionControlSystems.BZR_SVN:
+            rcs_type = 'bzr-svn'
+            target_rcs_type = 'bzr'
+        elif code_import.rcs_type == RevisionControlSystems.CVS:
+            rcs_type = 'cvs'
+            target_rcs_type = 'bzr'
+        elif code_import.rcs_type == RevisionControlSystems.GIT:
+            rcs_type = 'git'
+            if IBranch.providedBy(target):
+                target_rcs_type = 'bzr'
+            else:
+                target_rcs_type = 'git'
+        elif code_import.rcs_type == RevisionControlSystems.BZR:
+            rcs_type = 'bzr'
+            target_rcs_type = 'bzr'
+        else:
+            raise AssertionError("Unknown rcs_type %r." % code_import.rcs_type)
+
+        result = [str(target_id), '%s:%s' % (rcs_type, target_rcs_type)]
+        if rcs_type in ('bzr-svn', 'git', 'bzr'):
+            result.append(str(code_import.url))
+            if (IBranch.providedBy(target) and
+                    target.stacked_on is not None and
+                    not target.stacked_on.private):
+                stacked_path = branch_id_alias(target.stacked_on)
+                stacked_on_url = compose_public_url('http', stacked_path)
+                result.append(stacked_on_url)
+        elif rcs_type == 'cvs':
+            result.append(str(code_import.cvs_root))
+            result.append(str(code_import.cvs_module))
+        else:
+            raise AssertionError("Unknown rcs_type %r." % rcs_type)
+        if target_rcs_type == 'git':
+            issuer = getUtility(IMacaroonIssuer, 'code-import-job')
+            macaroon = removeSecurityProxy(issuer).issueMacaroon(self)
+            # XXX cjwatson 2016-10-12: Consider arranging for this to be
+            # passed to worker processes in the environment instead.
+            result.append(macaroon.serialize())
+        return result
+
 
 @implementer(ICodeImportJobSet, ICodeImportJobSetPublic)
 class CodeImportJobSet(object):

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2018-01-02 16:10:26 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2018-03-15 21:30:48 +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).
 
 """Unit tests for CodeImportJob and CodeImportJobWorkflow."""
@@ -17,13 +17,17 @@
 from pymacaroons import Macaroon
 from pytz import UTC
 from testtools.matchers import (
+    Equals,
+    Matcher,
     MatchesListwise,
     MatchesStructure,
+    Mismatch,
     )
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.code.enums import (
     CodeImportEventType,
     CodeImportJobState,
@@ -31,6 +35,10 @@
     CodeImportReviewStatus,
     TargetRevisionControlSystems,
     )
+from lp.code.interfaces.codehosting import (
+    branch_id_alias,
+    compose_public_url,
+    )
 from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import (
@@ -76,6 +84,106 @@
     return login_celebrity('vcs_imports')
 
 
+class CodeImportJobMacaroonVerifies(Matcher):
+    """Matches if a code-import-job macaroon can be verified."""
+
+    def __init__(self, code_import):
+        self.code_import = code_import
+
+    def match(self, macaroon_raw):
+        issuer = getUtility(IMacaroonIssuer, 'code-import-job')
+        macaroon = Macaroon.deserialize(macaroon_raw)
+        if not issuer.verifyMacaroon(macaroon, self.code_import.import_job):
+            return Mismatch("Macaroon '%s' does not verify" % macaroon_raw)
+
+
+class TestCodeImportJob(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestCodeImportJob, self).setUp()
+        login_for_code_imports()
+
+    def assertArgumentsMatch(self, code_import, matcher, start_job=False):
+        job = self.factory.makeCodeImportJob(code_import=code_import)
+        if start_job:
+            machine = self.factory.makeCodeImportMachine(set_online=True)
+            getUtility(ICodeImportJobWorkflow).startJob(job, machine)
+        self.assertThat(job.makeWorkerArguments(), matcher)
+
+    def test_bzr_arguments(self):
+        code_import = self.factory.makeCodeImport(
+            bzr_branch_url="http://example.com/foo")
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr:bzr',
+                'http://example.com/foo']))
+
+    def test_git_arguments(self):
+        code_import = self.factory.makeCodeImport(
+            git_repo_url="git://git.example.com/project.git")
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'git:bzr',
+                'git://git.example.com/project.git']))
+
+    def test_git_to_git_arguments(self):
+        self.pushConfig('codeimport', macaroon_secret_key='some-secret')
+        self.useFixture(GitHostingFixture())
+        code_import = self.factory.makeCodeImport(
+            git_repo_url="git://git.example.com/project.git",
+            target_rcs_type=TargetRevisionControlSystems.GIT)
+        self.assertArgumentsMatch(
+            code_import, MatchesListwise([
+                Equals(code_import.git_repository.unique_name),
+                Equals('git:git'), Equals('git://git.example.com/project.git'),
+                CodeImportJobMacaroonVerifies(code_import)]),
+            # Start the job so that the macaroon can be verified.
+            start_job=True)
+
+    def test_cvs_arguments(self):
+        code_import = self.factory.makeCodeImport(
+            cvs_root=':pserver:[e-mail]/bar', cvs_module='bar')
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'cvs:bzr',
+                ':pserver:[e-mail]/bar', 'bar']))
+
+    def test_bzr_svn_arguments(self):
+        code_import = self.factory.makeCodeImport(
+            svn_branch_url='svn://svn.example.com/trunk')
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr-svn:bzr',
+                '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)
+        code_import.branch.stacked_on = devfocus
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(code_import.branch.id), 'bzr: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=removeSecurityProxy(devfocus).target.context,
+            bzr_branch_url='bzr://bzr.example.com/foo')
+        branch = removeSecurityProxy(code_import.branch)
+        branch.stacked_on = devfocus
+        self.assertArgumentsMatch(
+            code_import, Equals([
+                str(branch.id), 'bzr:bzr', 'bzr://bzr.example.com/foo']))
+
+
 class TestCodeImportJobSet(TestCaseWithFactory):
     """Unit tests for the CodeImportJobSet utility."""
 

=== modified file 'lib/lp/code/xmlrpc/codeimportscheduler.py'
--- lib/lp/code/xmlrpc/codeimportscheduler.py	2016-10-11 13:41:32 +0000
+++ lib/lp/code/xmlrpc/codeimportscheduler.py	2018-03-15 21:30:48 +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).
 
 """The code import scheduler XML-RPC API."""
@@ -18,7 +18,6 @@
     ICodeImportJobWorkflow,
     )
 from lp.code.interfaces.codeimportscheduler import ICodeImportScheduler
-from lp.codehosting.codeimport.worker import CodeImportSourceDetails
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.webapp import (
     canonical_url,
@@ -67,8 +66,7 @@
     @return_fault
     def _getImportDataForJobID(self, job_id):
         job = self._getJob(job_id)
-        arguments = CodeImportSourceDetails.fromCodeImportJob(
-            job).asArguments()
+        arguments = job.makeWorkerArguments()
         target = job.code_import.target
         target_url = canonical_url(target)
         log_file_name = '%s.log' % target.unique_name[1:].replace('/', '-')

=== modified file 'lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py'
--- lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py	2016-10-11 13:41:32 +0000
+++ lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py	2018-03-15 21:30:48 +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).
 
 """Test for the methods of `ICodeImportScheduler`."""
@@ -15,7 +15,6 @@
 from lp.code.model.codeimportjob import CodeImportJob
 from lp.code.tests.codeimporthelpers import make_running_import
 from lp.code.xmlrpc.codeimportscheduler import CodeImportSchedulerAPI
-from lp.codehosting.codeimport.worker import CodeImportSourceDetails
 from lp.services.database.constants import UTC_NOW
 from lp.services.webapp import canonical_url
 from lp.testing import (
@@ -63,8 +62,7 @@
         code_import = removeSecurityProxy(code_import_job).code_import
         code_import_arguments, target_url, log_file_name = \
             self.api.getImportDataForJobID(code_import_job.id)
-        import_as_arguments = CodeImportSourceDetails.fromCodeImportJob(
-            code_import_job).asArguments()
+        import_as_arguments = code_import_job.makeWorkerArguments()
         expected_log_file_name = '%s.log' % (
             code_import.target.unique_name[1:].replace('/', '-'))
         self.assertEqual(

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2018-02-14 01:27:28 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2018-03-15 21:30:48 +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).
 
 """Tests for the code import worker."""
@@ -45,26 +45,10 @@
     )
 from dulwich.repo import Repo as GitRepo
 from fixtures import FakeLogger
-from pymacaroons import Macaroon
 import subvertpy
 import subvertpy.client
 import subvertpy.ra
-from testtools.matchers import (
-    Equals,
-    Matcher,
-    MatchesListwise,
-    Mismatch,
-    )
-from zope.component import getUtility
 
-from lp.app.enums import InformationType
-from lp.code.enums import TargetRevisionControlSystems
-from lp.code.interfaces.codehosting import (
-    branch_id_alias,
-    compose_public_url,
-    )
-from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
-from lp.code.tests.helpers import GitHostingFixture
 import lp.codehosting
 from lp.codehosting.codeimport.tarball import (
     create_tarball,
@@ -100,16 +84,8 @@
 from lp.codehosting.tests.helpers import create_branch_with_one_revision
 from lp.services.config import config
 from lp.services.log.logger import BufferLogger
-from lp.services.macaroons.interfaces import IMacaroonIssuer
-from lp.testing import (
-    celebrity_logged_in,
-    TestCase,
-    TestCaseWithFactory,
-    )
-from lp.testing.layers import (
-    BaseLayer,
-    DatabaseFunctionalLayer,
-    )
+from lp.testing import TestCase
+from lp.testing.layers import BaseLayer
 
 
 class ForeignBranchPluginLayer(BaseLayer):
@@ -820,8 +796,8 @@
         raise NotImplementedError(
             "Override this with a VCS-specific implementation.")
 
-    def makeSourceDetails(self, module_name, files, stacked_on_url=None):
-        """Make a `CodeImportSourceDetails` that points to a real repository.
+    def makeWorkerArguments(self, module_name, files, stacked_on_url=None):
+        """Make a list of worker arguments pointing to a real repository.
 
         This should set `self.foreign_commit_count` to an appropriate value.
 
@@ -830,6 +806,11 @@
         raise NotImplementedError(
             "Override this with a VCS-specific implementation.")
 
+    def makeSourceDetails(self, module_name, files, stacked_on_url=None):
+        """Make a `CodeImportSourceDetails` pointing to a real repository."""
+        return CodeImportSourceDetails.fromArguments(self.makeWorkerArguments(
+            module_name, files, stacked_on_url=stacked_on_url))
+
     def getStoredBazaarBranch(self, worker):
         """Get the Bazaar branch 'worker' stored into its BazaarBranchStore.
         """
@@ -876,8 +857,9 @@
     def test_import_script(self):
         # Like test_import, but using the code-import-worker.py script
         # to perform the import.
-        source_details = self.makeSourceDetails(
+        arguments = self.makeWorkerArguments(
             'trunk', [('README', 'Original contents')])
+        source_details = CodeImportSourceDetails.fromArguments(arguments)
 
         clean_up_default_stores_for_import(source_details)
 
@@ -885,8 +867,7 @@
             config.root, 'scripts', 'code-import-worker.py')
         output = tempfile.TemporaryFile()
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] +
-            source_details.asArguments(),
+            [script_path, '--access-policy=anything'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, 0)
 
@@ -914,8 +895,9 @@
         # with a code of CodeImportWorkerExitCode.SUCCESS.  After a successful
         # import that does not import revisions, the worker exits with a code
         # of CodeImportWorkerExitCode.SUCCESS_NOCHANGE.
-        source_details = self.makeSourceDetails(
+        arguments = self.makeWorkerArguments(
             'trunk', [('README', 'Original contents')])
+        source_details = CodeImportSourceDetails.fromArguments(arguments)
 
         clean_up_default_stores_for_import(source_details)
 
@@ -923,13 +905,11 @@
             config.root, 'scripts', 'code-import-worker.py')
         output = tempfile.TemporaryFile()
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] +
-            source_details.asArguments(),
+            [script_path, '--access-policy=anything'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, CodeImportWorkerExitCode.SUCCESS)
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] +
-            source_details.asArguments(),
+            [script_path, '--access-policy=anything'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, CodeImportWorkerExitCode.SUCCESS_NOCHANGE)
 
@@ -971,9 +951,8 @@
         self.foreign_commit_count += 1
         shutil.rmtree('working_dir')
 
-    def makeSourceDetails(self, module_name, files, stacked_on_url=None):
-        """Make a CVS `CodeImportSourceDetails` pointing at a real CVS repo.
-        """
+    def makeWorkerArguments(self, module_name, files, stacked_on_url=None):
+        """Make CVS worker arguments pointing at a real CVS repo."""
         cvs_server = CVSServer(self.makeTemporaryDirectory())
         cvs_server.start_server()
         self.addCleanup(cvs_server.stop_server)
@@ -982,9 +961,10 @@
 
         self.foreign_commit_count = 2
 
-        return self.factory.makeCodeImportSourceDetails(
-            rcstype='cvs', cvs_root=cvs_server.getRoot(), cvs_module='trunk',
-            stacked_on_url=stacked_on_url)
+        return [
+            str(self.factory.getUniqueInteger()), 'cvs:bzr',
+            cvs_server.getRoot(), 'trunk',
+            ]
 
 
 class SubversionImportHelpers:
@@ -1009,9 +989,8 @@
         self.foreign_commit_count += 1
         shutil.rmtree('working_tree')
 
-    def makeSourceDetails(self, branch_name, files, stacked_on_url=None):
-        """Make a SVN `CodeImportSourceDetails` pointing at a real SVN repo.
-        """
+    def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
+        """Make SVN worker arguments pointing at a real SVN repo."""
         svn_server = SubversionServer(self.makeTemporaryDirectory())
         svn_server.start_server()
         self.addCleanup(svn_server.stop_server)
@@ -1019,9 +998,13 @@
         svn_branch_url = svn_server.makeBranch(branch_name, files)
         svn_branch_url = svn_branch_url.replace('://localhost/', ':///')
         self.foreign_commit_count = 2
-        return self.factory.makeCodeImportSourceDetails(
-            rcstype=self.rcstype, url=svn_branch_url,
-            stacked_on_url=stacked_on_url)
+        arguments = [
+            str(self.factory.getUniqueInteger()), '%s:bzr' % self.rcstype,
+            svn_branch_url,
+            ]
+        if stacked_on_url is not None:
+            arguments.append(stacked_on_url)
+        return arguments
 
 
 class PullingImportWorkerTests:
@@ -1171,9 +1154,8 @@
             committer="Joe Random Hacker <[e-mail]>", ref=ref)
         self.foreign_commit_count += 1
 
-    def makeSourceDetails(self, branch_name, files, stacked_on_url=None):
-        """Make a Git `CodeImportSourceDetails` pointing at a real Git repo.
-        """
+    def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
+        """Make Git worker arguments pointing at a real Git repo."""
         repository_store = self.makeTemporaryDirectory()
         git_server = GitServer(repository_store)
         git_server.start_server()
@@ -1182,9 +1164,13 @@
         git_server.makeRepo('source', files)
         self.foreign_commit_count = 1
 
-        return self.factory.makeCodeImportSourceDetails(
-            rcstype='git', url=git_server.get_url('source'),
-            stacked_on_url=stacked_on_url)
+        arguments = [
+            str(self.factory.getUniqueInteger()), 'git:bzr',
+            git_server.get_url('source'),
+            ]
+        if stacked_on_url is not None:
+            arguments.append(stacked_on_url)
+        return arguments
 
     def test_non_master(self):
         # non-master branches can be specified in the import URL.
@@ -1250,9 +1236,8 @@
             committer="Joe Random Hacker <[e-mail]>")
         self.foreign_commit_count += 1
 
-    def makeSourceDetails(self, branch_name, files, stacked_on_url=None):
-        """Make Bzr `CodeImportSourceDetails` pointing at a real Bzr repo.
-        """
+    def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
+        """Make Bzr worker arguments pointing at a real Bzr repo."""
         repository_path = self.makeTemporaryDirectory()
         bzr_server = BzrServer(repository_path)
         bzr_server.start_server()
@@ -1261,9 +1246,13 @@
         bzr_server.makeRepo(files)
         self.foreign_commit_count = 1
 
-        return self.factory.makeCodeImportSourceDetails(
-            rcstype='bzr', url=bzr_server.get_url(),
-            stacked_on_url=stacked_on_url)
+        arguments = [
+            str(self.factory.getUniqueInteger()), 'bzr:bzr',
+            bzr_server.get_url(),
+            ]
+        if stacked_on_url is not None:
+            arguments.append(stacked_on_url)
+        return arguments
 
     def test_partial(self):
         self.skipTest(
@@ -1416,105 +1405,3 @@
             self.old_server.stop_server()
         self.assertEqual(
             CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
-
-
-class CodeImportJobMacaroonVerifies(Matcher):
-    """Matches if a code-import-job macaroon can be verified."""
-
-    def __init__(self, code_import):
-        self.code_import = code_import
-
-    def match(self, macaroon_raw):
-        issuer = getUtility(IMacaroonIssuer, 'code-import-job')
-        macaroon = Macaroon.deserialize(macaroon_raw)
-        if not issuer.verifyMacaroon(macaroon, self.code_import.import_job):
-            return Mismatch("Macaroon '%s' does not verify" % macaroon_raw)
-
-
-class CodeImportSourceDetailsTests(TestCaseWithFactory):
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        # Use an admin user as we aren't checking edit permissions here.
-        TestCaseWithFactory.setUp(self, '[e-mail]')
-
-    def assertArgumentsMatch(self, code_import, matcher, start_job=False):
-        job = self.factory.makeCodeImportJob(code_import=code_import)
-        if start_job:
-            machine = self.factory.makeCodeImportMachine(set_online=True)
-            with celebrity_logged_in("vcs_imports"):
-                getUtility(ICodeImportJobWorkflow).startJob(job, machine)
-        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")
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'bzr:bzr',
-                'http://example.com/foo']))
-
-    def test_git_arguments(self):
-        code_import = self.factory.makeCodeImport(
-            git_repo_url="git://git.example.com/project.git")
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'git:bzr',
-                'git://git.example.com/project.git']))
-
-    def test_git_to_git_arguments(self):
-        self.pushConfig('codeimport', macaroon_secret_key='some-secret')
-        self.useFixture(GitHostingFixture())
-        code_import = self.factory.makeCodeImport(
-            git_repo_url="git://git.example.com/project.git",
-            target_rcs_type=TargetRevisionControlSystems.GIT)
-        self.assertArgumentsMatch(
-            code_import, MatchesListwise([
-                Equals(code_import.git_repository.unique_name),
-                Equals('git:git'), Equals('git://git.example.com/project.git'),
-                CodeImportJobMacaroonVerifies(code_import)]),
-            # Start the job so that the macaroon can be verified.
-            start_job=True)
-
-    def test_cvs_arguments(self):
-        code_import = self.factory.makeCodeImport(
-            cvs_root=':pserver:[e-mail]/bar', cvs_module='bar')
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'cvs:bzr',
-                ':pserver:[e-mail]/bar', 'bar']))
-
-    def test_bzr_svn_arguments(self):
-        code_import = self.factory.makeCodeImport(
-            svn_branch_url='svn://svn.example.com/trunk')
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'bzr-svn:bzr',
-                '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)
-        code_import.branch.stacked_on = devfocus
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'bzr: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')
-        code_import.branch.stacked_on = devfocus
-        self.assertArgumentsMatch(
-            code_import, Equals([
-                str(code_import.branch.id), 'bzr:bzr',
-                'bzr://bzr.example.com/foo']))

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2017-12-19 17:22:44 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2018-03-15 21:30:48 +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).
 
 """Tests for the CodeImportWorkerMonitor and related classes."""
@@ -34,6 +34,7 @@
 from twisted.python import log
 from twisted.web import xmlrpc
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from lp.code.enums import (
     CodeImportResultStatus,
@@ -762,7 +763,8 @@
                 self.factory.makePerson())
         job = getUtility(ICodeImportJobSet).getJobForMachine('machine', 10)
         self.assertEqual(code_import, job.code_import)
-        source_details = CodeImportSourceDetails.fromCodeImportJob(job)
+        source_details = CodeImportSourceDetails.fromArguments(
+            removeSecurityProxy(job.makeWorkerArguments()))
         if IBranch.providedBy(code_import.target):
             clean_up_default_stores_for_import(source_details)
             self.addCleanup(clean_up_default_stores_for_import, source_details)

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2017-05-16 11:53:28 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2018-03-15 21:30:48 +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).
 
 """The code import worker. This imports code from foreign repositories."""
@@ -73,18 +73,8 @@
     )
 from pymacaroons import Macaroon
 import SCM
-from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
 
-from lp.code.enums import RevisionControlSystems
-from lp.code.interfaces.branch import (
-    get_blacklisted_hostnames,
-    IBranch,
-    )
-from lp.code.interfaces.codehosting import (
-    branch_id_alias,
-    compose_public_url,
-    )
+from lp.code.interfaces.branch import get_blacklisted_hostnames
 from lp.codehosting.codeimport.foreigntree import CVSWorkingTree
 from lp.codehosting.codeimport.tarball import (
     create_tarball,
@@ -97,7 +87,6 @@
     SafeBranchOpener,
     )
 from lp.services.config import config
-from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.propertycache import cachedproperty
 from lp.services.timeout import urlfetch
 from lp.services.utils import sanitise_urls
@@ -285,7 +274,7 @@
     As the worker doesn't talk to the database, we don't use
     `CodeImport` objects for this.
 
-    The 'fromArguments' and 'asArguments' methods convert to and from a form
+    The 'fromArguments' method builds an instance of this class from a form
     of the information suitable for passing around on executables' command
     lines.
 
@@ -319,6 +308,8 @@
     @classmethod
     def fromArguments(cls, arguments):
         """Convert command line-style arguments to an instance."""
+        # Keep this in sync with CodeImportJob.makeWorkerArguments.
+        arguments = list(arguments)
         target_id = arguments.pop(0)
         rcstype = arguments.pop(0)
         # XXX cjwatson 2016-10-12: Remove compatibility code once the
@@ -354,71 +345,6 @@
             target_id, rcstype, target_rcstype, url, cvs_root, cvs_module,
             stacked_on_url, macaroon)
 
-    @classmethod
-    def fromCodeImportJob(cls, job):
-        """Convert a `CodeImportJob` to an instance."""
-        code_import = job.code_import
-        target = code_import.target
-        if IBranch.providedBy(target):
-            if target.stacked_on is not None and not target.stacked_on.private:
-                stacked_path = branch_id_alias(target.stacked_on)
-                stacked_on_url = compose_public_url('http', stacked_path)
-            else:
-                stacked_on_url = None
-            target_id = target.id
-        else:
-            # We don't have a better way to identify the target repository
-            # than the mutable unique name, but the macaroon constrains
-            # pushes tightly enough that the worst case is an authentication
-            # failure.
-            target_id = target.unique_name
-        if code_import.rcs_type == RevisionControlSystems.BZR_SVN:
-            return cls(
-                target_id, 'bzr-svn', 'bzr', str(code_import.url),
-                stacked_on_url=stacked_on_url)
-        elif code_import.rcs_type == RevisionControlSystems.CVS:
-            return cls(
-                target_id, 'cvs', 'bzr',
-                cvs_root=str(code_import.cvs_root),
-                cvs_module=str(code_import.cvs_module))
-        elif code_import.rcs_type == RevisionControlSystems.GIT:
-            if IBranch.providedBy(target):
-                return cls(
-                    target_id, 'git', 'bzr', str(code_import.url),
-                    stacked_on_url=stacked_on_url)
-            else:
-                issuer = getUtility(IMacaroonIssuer, 'code-import-job')
-                macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-                return cls(
-                    target_id, 'git', 'git', str(code_import.url),
-                    macaroon=macaroon)
-        elif code_import.rcs_type == RevisionControlSystems.BZR:
-            return cls(
-                target_id, 'bzr', 'bzr', str(code_import.url),
-                stacked_on_url=stacked_on_url)
-        else:
-            raise AssertionError("Unknown rcstype %r." % code_import.rcs_type)
-
-    def asArguments(self):
-        """Return a list of arguments suitable for passing to a child process.
-        """
-        result = [
-            str(self.target_id), '%s:%s' % (self.rcstype, self.target_rcstype)]
-        if self.rcstype in ['bzr-svn', 'git', 'bzr']:
-            result.append(self.url)
-            if self.stacked_on_url is not None:
-                result.append(self.stacked_on_url)
-        elif self.rcstype == 'cvs':
-            result.append(self.cvs_root)
-            result.append(self.cvs_module)
-        else:
-            raise AssertionError("Unknown rcstype %r." % self.rcstype)
-        if self.target_rcstype == 'git':
-            # XXX cjwatson 2016-10-12: Consider arranging for this to be
-            # passed to worker processes in the environment instead.
-            result.append(self.macaroon.serialize())
-        return result
-
 
 class ImportDataStore:
     """A store for data associated with an import.