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

[Colin Watson]

Remove old-style argument parsing from code import worker

The scheduler now always dispatches code imports using argparse-style arguments.

[Ioana Lasc (ilasc)]

LGTM

review: Approve

diff --git a/lib/lp/codehosting/codeimport/tests/test_worker.py b/lib/lp/codehosting/codeimport/tests/test_worker.py
index 931c065..892ec3c 100644
--- a/lib/lp/codehosting/codeimport/tests/test_worker.py
+++ b/lib/lp/codehosting/codeimport/tests/test_worker.py
@@ -991,25 +991,6 @@ class TestCVSImport(WorkerTest, CSCVSActualImportMixin):
         self.foreign_commit_count = 2
 
         return [
-            str(self.factory.getUniqueInteger()), 'cvs:bzr',
-            cvs_server.getRoot(), 'trunk',
-            ]
-
-
-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',
             ]
@@ -1047,11 +1028,11 @@ class SubversionImportHelpers:
         svn_branch_url = svn_branch_url.replace('://localhost/', ':///')
         self.foreign_commit_count = 2
         arguments = [
-            str(self.factory.getUniqueInteger()), '%s:bzr' % self.rcstype,
+            str(self.factory.getUniqueInteger()), self.rcstype, 'bzr',
             svn_branch_url,
             ]
         if stacked_on_url is not None:
-            arguments.append(stacked_on_url)
+            arguments.extend(['--stacked-on', stacked_on_url])
         return arguments
 
 
@@ -1213,11 +1194,11 @@ class TestGitImport(WorkerTest, TestActualImportMixin,
         self.foreign_commit_count = 1
 
         arguments = [
-            str(self.factory.getUniqueInteger()), 'git:bzr',
+            str(self.factory.getUniqueInteger()), 'git', 'bzr',
             git_server.get_url('source'),
             ]
         if stacked_on_url is not None:
-            arguments.append(stacked_on_url)
+            arguments.extend(['--stacked-on', stacked_on_url])
         return arguments
 
     def test_non_master(self):
@@ -1244,28 +1225,6 @@ 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):
 
@@ -1327,27 +1286,6 @@ 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):
 
@@ -1382,11 +1320,11 @@ class TestBzrImport(WorkerTest, TestActualImportMixin,
         self.foreign_commit_count = 1
 
         arguments = [
-            str(self.factory.getUniqueInteger()), 'bzr:bzr',
+            str(self.factory.getUniqueInteger()), 'bzr', 'bzr',
             bzr_server.get_url(),
             ]
         if stacked_on_url is not None:
-            arguments.append(stacked_on_url)
+            arguments.extend(['--stacked-on', stacked_on_url])
         return arguments
 
     def test_partial(self):
@@ -1428,36 +1366,14 @@ 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',
+            'git-unique-name', 'git', 'git',
             self.factory.getUniqueURL(scheme='git'),
-            Macaroon().serialize(),
+            '--macaroon', Macaroon().serialize(),
             ]
 
     def test_throttleProgress(self):
@@ -1505,18 +1421,6 @@ 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 9cd5258..6bf5d8d 100644
--- a/lib/lp/codehosting/codeimport/worker.py
+++ b/lib/lp/codehosting/codeimport/worker.py
@@ -284,17 +284,6 @@ 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.
 
@@ -336,7 +325,7 @@ 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 = ArgumentParser(description='Code import worker.')
         parser.add_argument(
             'target_id', help='Target branch ID or repository unique name')
         parser.add_argument(
@@ -357,69 +346,32 @@ class CodeImportSourceDetails:
             help=(
                 'Macaroon authorising push (only valid for target_rcstype '
                 'git)'))
-        arguments = list(arguments)
-        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:
-                    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
-                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':
+        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:
                 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)
+            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
         return cls(
             target_id, rcstype, target_rcstype, url, cvs_root, cvs_module,
             stacked_on_url, macaroon)