launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25582
[Merge] ~cjwatson/launchpad:code-import-worker-require-argparse into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:code-import-worker-require-argparse into launchpad:master.
Commit message:
Remove old-style argument parsing from code import worker
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/392998
The scheduler now always dispatches code imports using argparse-style arguments.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:code-import-worker-require-argparse into launchpad:master.
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)
