launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:code-import-scheduler-argparse into launchpad:master.

Commit message:
Dispatch code imports using argparse-style arguments

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/392366

This will allow us to remove old-style argument parsing from the worker.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:code-import-scheduler-argparse into launchpad:master.

diff --git a/lib/lp/code/model/codeimportjob.py b/lib/lp/code/model/codeimportjob.py
index d33216f..fa81587 100644
--- a/lib/lp/code/model/codeimportjob.py
+++ b/lib/lp/code/model/codeimportjob.py
@@ -145,7 +145,7 @@ class CodeImportJob(StormBase):
         else:
             raise AssertionError("Unknown rcs_type %r." % code_import.rcs_type)
 
-        result = [str(target_id), '%s:%s' % (rcs_type, target_rcs_type)]
+        result = [str(target_id), rcs_type, target_rcs_type]
         if rcs_type in ('bzr-svn', 'git', 'bzr'):
             result.append(str(code_import.url))
             if (IBranch.providedBy(target) and
@@ -153,10 +153,10 @@ class CodeImportJob(StormBase):
                     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)
+                result.extend(['--stacked-on', stacked_on_url])
         elif rcs_type == 'cvs':
             result.append(str(code_import.cvs_root))
-            result.append(str(code_import.cvs_module))
+            result.extend(['--cvs-module', str(code_import.cvs_module)])
         else:
             raise AssertionError("Unknown rcs_type %r." % rcs_type)
         if target_rcs_type == 'git':
@@ -164,7 +164,7 @@ class CodeImportJob(StormBase):
             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())
+            result.extend(['--macaroon', macaroon.serialize()])
         return result
 
     def destroySelf(self):
diff --git a/lib/lp/code/model/tests/test_codeimportjob.py b/lib/lp/code/model/tests/test_codeimportjob.py
index 2f343e8..14ea8f2 100644
--- a/lib/lp/code/model/tests/test_codeimportjob.py
+++ b/lib/lp/code/model/tests/test_codeimportjob.py
@@ -128,7 +128,7 @@ class TestCodeImportJob(TestCaseWithFactory):
             bzr_branch_url="http://example.com/foo";)
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(code_import.branch.id), 'bzr:bzr',
+                str(code_import.branch.id), 'bzr', 'bzr',
                 'http://example.com/foo']))
 
     def test_git_arguments(self):
@@ -136,7 +136,7 @@ class TestCodeImportJob(TestCaseWithFactory):
             git_repo_url="git://git.example.com/project.git")
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(code_import.branch.id), 'git:bzr',
+                str(code_import.branch.id), 'git', 'bzr',
                 'git://git.example.com/project.git']))
 
     def test_git_to_git_arguments(self):
@@ -149,7 +149,9 @@ class TestCodeImportJob(TestCaseWithFactory):
         self.assertArgumentsMatch(
             code_import, MatchesListwise([
                 Equals(code_import.git_repository.unique_name),
-                Equals('git:git'), Equals('git://git.example.com/project.git'),
+                Equals('git'), Equals('git'),
+                Equals('git://git.example.com/project.git'),
+                Equals('--macaroon'),
                 CodeImportJobMacaroonVerifies(code_import)]),
             # Start the job so that the macaroon can be verified.
             start_job=True)
@@ -159,15 +161,15 @@ class TestCodeImportJob(TestCaseWithFactory):
             cvs_root=':pserver:foo@xxxxxxxxxxx/bar', cvs_module='bar')
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(code_import.branch.id), 'cvs:bzr',
-                ':pserver:foo@xxxxxxxxxxx/bar', 'bar']))
+                str(code_import.branch.id), 'cvs', 'bzr',
+                ':pserver:foo@xxxxxxxxxxx/bar', '--cvs-module', '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',
+                str(code_import.branch.id), 'bzr-svn', 'bzr',
                 'svn://svn.example.com/trunk']))
 
     def test_bzr_stacked(self):
@@ -178,8 +180,9 @@ class TestCodeImportJob(TestCaseWithFactory):
         code_import.branch.stacked_on = devfocus
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(code_import.branch.id), 'bzr:bzr',
+                str(code_import.branch.id), 'bzr', 'bzr',
                 'bzr://bzr.example.com/foo',
+                '--stacked-on',
                 compose_public_url('http', branch_id_alias(devfocus))]))
 
     def test_bzr_stacked_private(self):
@@ -193,7 +196,7 @@ class TestCodeImportJob(TestCaseWithFactory):
         branch.stacked_on = devfocus
         self.assertArgumentsMatch(
             code_import, Equals([
-                str(branch.id), 'bzr:bzr', 'bzr://bzr.example.com/foo']))
+                str(branch.id), 'bzr', 'bzr', 'bzr://bzr.example.com/foo']))
 
 
 class TestCodeImportJobSet(TestCaseWithFactory):