Merge ~cjwatson/launchpad:code-import-worker-argparse into launchpad:master

[Colin Watson]

Convert CodeImportSourceDetails.fromArguments to argparse

This requires some temporary compatibility code, since the scheduler still uses the old serialisation of arguments, but in the long run this will be much better than the previous fragile and hard-to-extend scheme.

[Ioana Lasc (ilasc)]

review: Approve

diff --git a/lib/lp/codehosting/codeimport/tests/test_worker.py b/lib/lp/codehosting/codeimport/tests/test_worker.py
index 9daf357..1c046ed 100644
--- a/lib/lp/codehosting/codeimport/tests/test_worker.py
+++ b/lib/lp/codehosting/codeimport/tests/test_worker.py
@@ -53,6 +53,7 @@ from CVS import (
     )
 from dulwich.repo import Repo as GitRepo
 from fixtures import FakeLogger
+from pymacaroons import Macaroon
 import scandir
 import six
 import subvertpy
@@ -892,7 +893,7 @@ class TestActualImportMixin:
             config.root, 'scripts', 'code-import-worker.py')
         output = tempfile.TemporaryFile()
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] + arguments,
+            [script_path, '--access-policy=anything', '--'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, 0)
 
@@ -930,11 +931,11 @@ class TestActualImportMixin:
             config.root, 'scripts', 'code-import-worker.py')
         output = tempfile.TemporaryFile()
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] + arguments,
+            [script_path, '--access-policy=anything', '--'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, CodeImportWorkerExitCode.SUCCESS)
         retcode = subprocess.call(
-            [script_path, '--access-policy=anything'] + arguments,
+            [script_path, '--access-policy=anything', '--'] + arguments,
             stderr=output, stdout=output)
         self.assertEqual(retcode, CodeImportWorkerExitCode.SUCCESS_NOCHANGE)
 
@@ -993,6 +994,25 @@ class TestCVSImport(WorkerTest, CSCVSActualImportMixin):
             ]
 
 
+class TestCVSImportArgparse(TestCVSImport):
+    """Like `TestCVSImport`, but with argparse-style arguments."""
+
+    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)
+
+        cvs_server.makeModule('trunk', [('README', 'original\n')])
+
+        self.foreign_commit_count = 2
+
+        return [
+            str(self.factory.getUniqueInteger()), 'cvs', 'bzr',
+            cvs_server.getRoot(), '--cvs-module', 'trunk',
+            ]
+
+
 class SubversionImportHelpers:
     """Implementations of `makeForeignCommit` and `makeSourceDetails` for svn.
     """
@@ -1222,6 +1242,28 @@ class TestGitImport(WorkerTest, TestActualImportMixin,
         self.assertEqual(lastrev.message, "Message for other")
 
 
+class TestGitImportArgparse(TestGitImport):
+    """Like `TestGitImport`, but with argparse-style arguments."""
+
+    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()
+        self.addCleanup(git_server.stop_server)
+
+        git_server.makeRepo('source', files)
+        self.foreign_commit_count = 1
+
+        arguments = [
+            str(self.factory.getUniqueInteger()), 'git', 'bzr',
+            git_server.get_url('source'),
+            ]
+        if stacked_on_url is not None:
+            arguments.extend(['--stacked-on', stacked_on_url])
+        return arguments
+
+
 class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
                        TestActualImportMixin, PullingImportWorkerTests):
 
@@ -1283,6 +1325,27 @@ class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
             self.assertEqual(content, cache_file.read())
 
 
+class TestBzrSvnImportArgparse(TestBzrSvnImport):
+    """Like `TestBzrSvnImport`, but with argparse-style arguments."""
+
+    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)
+
+        svn_branch_url = svn_server.makeBranch(branch_name, files)
+        svn_branch_url = svn_branch_url.replace('://localhost/', ':///')
+        self.foreign_commit_count = 2
+        arguments = [
+            str(self.factory.getUniqueInteger()), self.rcstype, 'bzr',
+            svn_branch_url,
+            ]
+        if stacked_on_url is not None:
+            arguments.extend(['--stacked-on', stacked_on_url])
+        return arguments
+
+
 class TestBzrImport(WorkerTest, TestActualImportMixin,
                     PullingImportWorkerTests):
 
@@ -1363,11 +1426,41 @@ class TestBzrImport(WorkerTest, TestActualImportMixin,
             branch.repository.get_revision(branch.last_revision()).committer)
 
 
+class TestBzrImportArgparse(TestBzrImport):
+    """Like `TestBzrImport`, but with argparse-style arguments."""
+
+    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()
+        self.addCleanup(bzr_server.stop_server)
+
+        bzr_server.makeRepo(files)
+        self.foreign_commit_count = 1
+
+        arguments = [
+            str(self.factory.getUniqueInteger()), 'bzr', 'bzr',
+            bzr_server.get_url(),
+            ]
+        if stacked_on_url is not None:
+            arguments.extend(['--stacked-on', stacked_on_url])
+        return arguments
+
+
 class TestGitToGitImportWorker(TestCase):
 
+    def makeWorkerArguments(self):
+        """Make Git worker arguments, pointing at a fake URL for now."""
+        return [
+            'git-unique-name', 'git:git',
+            self.factory.getUniqueURL(scheme='git'),
+            Macaroon().serialize(),
+            ]
+
     def test_throttleProgress(self):
-        source_details = self.factory.makeCodeImportSourceDetails(
-            rcstype="git", target_rcstype="git")
+        source_details = CodeImportSourceDetails.fromArguments(
+            self.makeWorkerArguments())
         logger = BufferLogger()
         worker = GitToGitImportWorker(
             source_details, logger, AcceptAnythingPolicy())
@@ -1410,6 +1503,18 @@ class TestGitToGitImportWorker(TestCase):
                 ContainsAll([u"%d ...\r" % i for i in (9, 19, 29, 39, 49)]))
 
 
+class TestGitToGitImportWorkerArgparse(TestCase):
+    """Like `TestGitToGitImportWorker`, but with argparse-style arguments."""
+
+    def makeWorkerArguments(self):
+        """Make Git worker arguments, pointing at a fake URL for now."""
+        return [
+            'git-unique-name', 'git', 'git',
+            self.factory.getUniqueURL(scheme='git'),
+            '--macaroon', Macaroon().serialize(),
+            ]
+
+
 class CodeImportBranchOpenPolicyTests(TestCase):
 
     def setUp(self):
diff --git a/lib/lp/codehosting/codeimport/worker.py b/lib/lp/codehosting/codeimport/worker.py
index 5aacf29..bb3b559 100644
--- a/lib/lp/codehosting/codeimport/worker.py
+++ b/lib/lp/codehosting/codeimport/worker.py
@@ -19,6 +19,7 @@ __all__ = [
     'get_default_bazaar_branch_store',
     ]
 
+from argparse import ArgumentParser
 import io
 import os
 import shutil
@@ -281,6 +282,17 @@ def get_default_bazaar_branch_store():
         get_transport_from_url(config.codeimport.bazaar_branch_store))
 
 
+class ArgumentParserError(Exception):
+    """An argument parsing error."""
+
+
+class TolerantArgumentParser(ArgumentParser):
+    """An `ArgumentParser` that raises exceptions on errors."""
+
+    def error(self, message):
+        raise ArgumentParserError(message)
+
+
 class CodeImportSourceDetails:
     """The information needed to process an import.
 
@@ -322,37 +334,90 @@ class CodeImportSourceDetails:
     def fromArguments(cls, arguments):
         """Convert command line-style arguments to an instance."""
         # Keep this in sync with CodeImportJob.makeWorkerArguments.
+        parser = TolerantArgumentParser(description='Code import worker.')
+        parser.add_argument(
+            'target_id', help='Target branch ID or repository unique name')
+        parser.add_argument(
+            'rcstype', choices=('bzr', 'bzr-svn', 'cvs', 'git'),
+            help='Source revision control system type')
+        parser.add_argument(
+            'target_rcstype', choices=('bzr', 'git'),
+            help='Target revision control system type')
+        parser.add_argument(
+            'url', help='Source URL (or $CVSROOT for rcstype cvs)')
+        parser.add_argument(
+            '--cvs-module', help='CVS module (only valid for rcstype cvs)')
+        parser.add_argument(
+            '--stacked-on',
+            help='Stacked-on URL (only valid for target_rcstype bzr)')
+        parser.add_argument(
+            '--macaroon', type=Macaroon.deserialize,
+            help=(
+                'Macaroon authorising push (only valid for target_rcstype '
+                'git)'))
         arguments = list(arguments)
-        target_id = arguments.pop(0)
-        rcstype = arguments.pop(0)
-        if ':' not in rcstype:
-            raise AssertionError(
-                "'%s' does not contain both source and target types." %
-                rcstype)
-        rcstype, target_rcstype = rcstype.split(':', 1)
-        if rcstype in ['bzr-svn', 'git', 'bzr']:
-            url = arguments.pop(0)
+        try:
+            args = parser.parse_args(arguments)
+            target_id = args.target_id
+            rcstype = args.rcstype
+            target_rcstype = args.target_rcstype
+            url = args.url if rcstype in ('bzr', 'bzr-svn', 'git') else None
+            if rcstype == 'cvs':
+                if args.cvs_module is None:
+                    parser.error('rcstype cvs requires --cvs-module')
+                cvs_root = args.url
+                cvs_module = args.cvs_module
+            else:
+                cvs_root = None
+                cvs_module = None
             if target_rcstype == 'bzr':
                 try:
-                    stacked_on_url = arguments.pop(0)
-                except IndexError:
+                    target_id = int(target_id)
+                except ValueError:
+                    parser.error(
+                        'rcstype bzr requires target_id to be an integer')
+                stacked_on_url = args.stacked_on
+                macaroon = None
+            elif target_rcstype == 'git':
+                if args.macaroon is None:
+                    parser.error('target_rcstype git requires --macaroon')
+                stacked_on_url = None
+                macaroon = args.macaroon
+        except ArgumentParserError:
+            # XXX cjwatson 2020-10-05: Remove this old-style argument
+            # handling once the scheduler always passes something argparse
+            # can handle.
+            target_id = arguments.pop(0)
+            rcstype = arguments.pop(0)
+            if ':' not in rcstype:
+                raise AssertionError(
+                    "'%s' does not contain both source and target types." %
+                    rcstype)
+            rcstype, target_rcstype = rcstype.split(':', 1)
+            if rcstype in ['bzr-svn', 'git', 'bzr']:
+                url = arguments.pop(0)
+                if target_rcstype == 'bzr':
+                    try:
+                        stacked_on_url = arguments.pop(0)
+                    except IndexError:
+                        stacked_on_url = None
+                else:
                     stacked_on_url = None
-            else:
+                cvs_root = cvs_module = None
+            elif rcstype == 'cvs':
+                url = None
                 stacked_on_url = None
-            cvs_root = cvs_module = None
-        elif rcstype == 'cvs':
-            url = None
-            stacked_on_url = None
-            [cvs_root, cvs_module] = arguments
-        else:
-            raise AssertionError("Unknown rcstype %r." % rcstype)
-        if target_rcstype == 'bzr':
-            target_id = int(target_id)
-            macaroon = None
-        elif target_rcstype == 'git':
-            macaroon = Macaroon.deserialize(arguments.pop(0))
-        else:
-            raise AssertionError("Unknown target_rcstype %r." % target_rcstype)
+                [cvs_root, cvs_module] = arguments
+            else:
+                raise AssertionError("Unknown rcstype %r." % rcstype)
+            if target_rcstype == 'bzr':
+                target_id = int(target_id)
+                macaroon = None
+            elif target_rcstype == 'git':
+                macaroon = Macaroon.deserialize(arguments.pop(0))
+            else:
+                raise AssertionError(
+                    "Unknown target_rcstype %r." % target_rcstype)
         return cls(
             target_id, rcstype, target_rcstype, url, cvs_root, cvs_module,
             stacked_on_url, macaroon)
diff --git a/lib/lp/codehosting/codeimport/workermonitor.py b/lib/lp/codehosting/codeimport/workermonitor.py
index 80cbe4b..8441978 100644
--- a/lib/lp/codehosting/codeimport/workermonitor.py
+++ b/lib/lp/codehosting/codeimport/workermonitor.py
@@ -252,6 +252,7 @@ class CodeImportWorkerMonitor:
         args = [interpreter, self.path_to_script]
         if self._access_policy is not None:
             args.append("--access-policy=%s" % self._access_policy)
+        args.append('--')
         command = args + worker_arguments
         self._logger.info(
             "Launching worker child process %s.", command)