Launchpad itself

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

  1. Git
  2. lp:~cjwatson/launchpad
  3. code-import-worker-require-argparse
  4. Merge into master
Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: efa730b092749f18248f01c777e9729ceca8fdc1
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:code-import-worker-require-argparse
Merge into: launchpad:master
Diff against target: 313 lines (+34/-178)
2 files modified
lib/lp/codehosting/codeimport/tests/test_worker.py (+8/-104)
lib/lp/codehosting/codeimport/worker.py (+26/-74)
Reviewer Review Type Date Requested Status
Ioana Lasc (community) Approve
Review via email: mp+392998@code.launchpad.net

Commit message

Remove old-style argument parsing from code import worker

Description of the change

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

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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):
991 self.foreign_commit_count = 2991 self.foreign_commit_count = 2
992992
993 return [993 return [
994 str(self.factory.getUniqueInteger()), 'cvs:bzr',
995 cvs_server.getRoot(), 'trunk',
996 ]
997
998
999class TestCVSImportArgparse(TestCVSImport):
1000 """Like `TestCVSImport`, but with argparse-style arguments."""
1001
1002 def makeWorkerArguments(self, module_name, files, stacked_on_url=None):
1003 """Make CVS worker arguments pointing at a real CVS repo."""
1004 cvs_server = CVSServer(self.makeTemporaryDirectory())
1005 cvs_server.start_server()
1006 self.addCleanup(cvs_server.stop_server)
1007
1008 cvs_server.makeModule('trunk', [('README', 'original\n')])
1009
1010 self.foreign_commit_count = 2
1011
1012 return [
1013 str(self.factory.getUniqueInteger()), 'cvs', 'bzr',994 str(self.factory.getUniqueInteger()), 'cvs', 'bzr',
1014 cvs_server.getRoot(), '--cvs-module', 'trunk',995 cvs_server.getRoot(), '--cvs-module', 'trunk',
1015 ]996 ]
@@ -1047,11 +1028,11 @@ class SubversionImportHelpers:
1047 svn_branch_url = svn_branch_url.replace('://localhost/', ':///')1028 svn_branch_url = svn_branch_url.replace('://localhost/', ':///')
1048 self.foreign_commit_count = 21029 self.foreign_commit_count = 2
1049 arguments = [1030 arguments = [
1050 str(self.factory.getUniqueInteger()), '%s:bzr' % self.rcstype,1031 str(self.factory.getUniqueInteger()), self.rcstype, 'bzr',
1051 svn_branch_url,1032 svn_branch_url,
1052 ]1033 ]
1053 if stacked_on_url is not None:1034 if stacked_on_url is not None:
1054 arguments.append(stacked_on_url)1035 arguments.extend(['--stacked-on', stacked_on_url])
1055 return arguments1036 return arguments
10561037
10571038
@@ -1213,11 +1194,11 @@ class TestGitImport(WorkerTest, TestActualImportMixin,
1213 self.foreign_commit_count = 11194 self.foreign_commit_count = 1
12141195
1215 arguments = [1196 arguments = [
1216 str(self.factory.getUniqueInteger()), 'git:bzr',1197 str(self.factory.getUniqueInteger()), 'git', 'bzr',
1217 git_server.get_url('source'),1198 git_server.get_url('source'),
1218 ]1199 ]
1219 if stacked_on_url is not None:1200 if stacked_on_url is not None:
1220 arguments.append(stacked_on_url)1201 arguments.extend(['--stacked-on', stacked_on_url])
1221 return arguments1202 return arguments
12221203
1223 def test_non_master(self):1204 def test_non_master(self):
@@ -1244,28 +1225,6 @@ class TestGitImport(WorkerTest, TestActualImportMixin,
1244 self.assertEqual(lastrev.message, "Message for other")1225 self.assertEqual(lastrev.message, "Message for other")
12451226
12461227
1247class TestGitImportArgparse(TestGitImport):
1248 """Like `TestGitImport`, but with argparse-style arguments."""
1249
1250 def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
1251 """Make Git worker arguments pointing at a real Git repo."""
1252 repository_store = self.makeTemporaryDirectory()
1253 git_server = GitServer(repository_store)
1254 git_server.start_server()
1255 self.addCleanup(git_server.stop_server)
1256
1257 git_server.makeRepo('source', files)
1258 self.foreign_commit_count = 1
1259
1260 arguments = [
1261 str(self.factory.getUniqueInteger()), 'git', 'bzr',
1262 git_server.get_url('source'),
1263 ]
1264 if stacked_on_url is not None:
1265 arguments.extend(['--stacked-on', stacked_on_url])
1266 return arguments
1267
1268
1269class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,1228class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
1270 TestActualImportMixin, PullingImportWorkerTests):1229 TestActualImportMixin, PullingImportWorkerTests):
12711230
@@ -1327,27 +1286,6 @@ class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
1327 self.assertEqual(content, cache_file.read())1286 self.assertEqual(content, cache_file.read())
13281287
13291288
1330class TestBzrSvnImportArgparse(TestBzrSvnImport):
1331 """Like `TestBzrSvnImport`, but with argparse-style arguments."""
1332
1333 def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
1334 """Make SVN worker arguments pointing at a real SVN repo."""
1335 svn_server = SubversionServer(self.makeTemporaryDirectory())
1336 svn_server.start_server()
1337 self.addCleanup(svn_server.stop_server)
1338
1339 svn_branch_url = svn_server.makeBranch(branch_name, files)
1340 svn_branch_url = svn_branch_url.replace('://localhost/', ':///')
1341 self.foreign_commit_count = 2
1342 arguments = [
1343 str(self.factory.getUniqueInteger()), self.rcstype, 'bzr',
1344 svn_branch_url,
1345 ]
1346 if stacked_on_url is not None:
1347 arguments.extend(['--stacked-on', stacked_on_url])
1348 return arguments
1349
1350
1351class TestBzrImport(WorkerTest, TestActualImportMixin,1289class TestBzrImport(WorkerTest, TestActualImportMixin,
1352 PullingImportWorkerTests):1290 PullingImportWorkerTests):
13531291
@@ -1382,11 +1320,11 @@ class TestBzrImport(WorkerTest, TestActualImportMixin,
1382 self.foreign_commit_count = 11320 self.foreign_commit_count = 1
13831321
1384 arguments = [1322 arguments = [
1385 str(self.factory.getUniqueInteger()), 'bzr:bzr',1323 str(self.factory.getUniqueInteger()), 'bzr', 'bzr',
1386 bzr_server.get_url(),1324 bzr_server.get_url(),
1387 ]1325 ]
1388 if stacked_on_url is not None:1326 if stacked_on_url is not None:
1389 arguments.append(stacked_on_url)1327 arguments.extend(['--stacked-on', stacked_on_url])
1390 return arguments1328 return arguments
13911329
1392 def test_partial(self):1330 def test_partial(self):
@@ -1428,36 +1366,14 @@ class TestBzrImport(WorkerTest, TestActualImportMixin,
1428 branch.repository.get_revision(branch.last_revision()).committer)1366 branch.repository.get_revision(branch.last_revision()).committer)
14291367
14301368
1431class TestBzrImportArgparse(TestBzrImport):
1432 """Like `TestBzrImport`, but with argparse-style arguments."""
1433
1434 def makeWorkerArguments(self, branch_name, files, stacked_on_url=None):
1435 """Make Bzr worker arguments pointing at a real Bzr repo."""
1436 repository_path = self.makeTemporaryDirectory()
1437 bzr_server = BzrServer(repository_path)
1438 bzr_server.start_server()
1439 self.addCleanup(bzr_server.stop_server)
1440
1441 bzr_server.makeRepo(files)
1442 self.foreign_commit_count = 1
1443
1444 arguments = [
1445 str(self.factory.getUniqueInteger()), 'bzr', 'bzr',
1446 bzr_server.get_url(),
1447 ]
1448 if stacked_on_url is not None:
1449 arguments.extend(['--stacked-on', stacked_on_url])
1450 return arguments
1451
1452
1453class TestGitToGitImportWorker(TestCase):1369class TestGitToGitImportWorker(TestCase):
14541370
1455 def makeWorkerArguments(self):1371 def makeWorkerArguments(self):
1456 """Make Git worker arguments, pointing at a fake URL for now."""1372 """Make Git worker arguments, pointing at a fake URL for now."""
1457 return [1373 return [
1458 'git-unique-name', 'git:git',1374 'git-unique-name', 'git', 'git',
1459 self.factory.getUniqueURL(scheme='git'),1375 self.factory.getUniqueURL(scheme='git'),
1460 Macaroon().serialize(),1376 '--macaroon', Macaroon().serialize(),
1461 ]1377 ]
14621378
1463 def test_throttleProgress(self):1379 def test_throttleProgress(self):
@@ -1505,18 +1421,6 @@ class TestGitToGitImportWorker(TestCase):
1505 ContainsAll([u"%d ...\r" % i for i in (9, 19, 29, 39, 49)]))1421 ContainsAll([u"%d ...\r" % i for i in (9, 19, 29, 39, 49)]))
15061422
15071423
1508class TestGitToGitImportWorkerArgparse(TestCase):
1509 """Like `TestGitToGitImportWorker`, but with argparse-style arguments."""
1510
1511 def makeWorkerArguments(self):
1512 """Make Git worker arguments, pointing at a fake URL for now."""
1513 return [
1514 'git-unique-name', 'git', 'git',
1515 self.factory.getUniqueURL(scheme='git'),
1516 '--macaroon', Macaroon().serialize(),
1517 ]
1518
1519
1520class CodeImportBranchOpenPolicyTests(TestCase):1424class CodeImportBranchOpenPolicyTests(TestCase):
15211425
1522 def setUp(self):1426 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():
284 get_transport_from_url(config.codeimport.bazaar_branch_store))284 get_transport_from_url(config.codeimport.bazaar_branch_store))
285285
286286
287class ArgumentParserError(Exception):
288 """An argument parsing error."""
289
290
291class TolerantArgumentParser(ArgumentParser):
292 """An `ArgumentParser` that raises exceptions on errors."""
293
294 def error(self, message):
295 raise ArgumentParserError(message)
296
297
298class CodeImportSourceDetails:287class CodeImportSourceDetails:
299 """The information needed to process an import.288 """The information needed to process an import.
300289
@@ -336,7 +325,7 @@ class CodeImportSourceDetails:
336 def fromArguments(cls, arguments):325 def fromArguments(cls, arguments):
337 """Convert command line-style arguments to an instance."""326 """Convert command line-style arguments to an instance."""
338 # Keep this in sync with CodeImportJob.makeWorkerArguments.327 # Keep this in sync with CodeImportJob.makeWorkerArguments.
339 parser = TolerantArgumentParser(description='Code import worker.')328 parser = ArgumentParser(description='Code import worker.')
340 parser.add_argument(329 parser.add_argument(
341 'target_id', help='Target branch ID or repository unique name')330 'target_id', help='Target branch ID or repository unique name')
342 parser.add_argument(331 parser.add_argument(
@@ -357,69 +346,32 @@ class CodeImportSourceDetails:
357 help=(346 help=(
358 'Macaroon authorising push (only valid for target_rcstype '347 'Macaroon authorising push (only valid for target_rcstype '
359 'git)'))348 'git)'))
360 arguments = list(arguments)349 args = parser.parse_args(arguments)
361 try:350 target_id = args.target_id
362 args = parser.parse_args(arguments)351 rcstype = args.rcstype
363 target_id = args.target_id352 target_rcstype = args.target_rcstype
364 rcstype = args.rcstype353 url = args.url if rcstype in ('bzr', 'bzr-svn', 'git') else None
365 target_rcstype = args.target_rcstype354 if rcstype == 'cvs':
366 url = args.url if rcstype in ('bzr', 'bzr-svn', 'git') else None355 if args.cvs_module is None:
367 if rcstype == 'cvs':356 parser.error('rcstype cvs requires --cvs-module')
368 if args.cvs_module is None:357 cvs_root = args.url
369 parser.error('rcstype cvs requires --cvs-module')358 cvs_module = args.cvs_module
370 cvs_root = args.url359 else:
371 cvs_module = args.cvs_module360 cvs_root = None
372 else:361 cvs_module = None
373 cvs_root = None362 if target_rcstype == 'bzr':
374 cvs_module = None363 try:
375 if target_rcstype == 'bzr':
376 try:
377 target_id = int(target_id)
378 except ValueError:
379 parser.error(
380 'rcstype bzr requires target_id to be an integer')
381 stacked_on_url = args.stacked_on
382 macaroon = None
383 elif target_rcstype == 'git':
384 if args.macaroon is None:
385 parser.error('target_rcstype git requires --macaroon')
386 stacked_on_url = None
387 macaroon = args.macaroon
388 except ArgumentParserError:
389 # XXX cjwatson 2020-10-05: Remove this old-style argument
390 # handling once the scheduler always passes something argparse
391 # can handle.
392 target_id = arguments.pop(0)
393 rcstype = arguments.pop(0)
394 if ':' not in rcstype:
395 raise AssertionError(
396 "'%s' does not contain both source and target types." %
397 rcstype)
398 rcstype, target_rcstype = rcstype.split(':', 1)
399 if rcstype in ['bzr-svn', 'git', 'bzr']:
400 url = arguments.pop(0)
401 if target_rcstype == 'bzr':
402 try:
403 stacked_on_url = arguments.pop(0)
404 except IndexError:
405 stacked_on_url = None
406 else:
407 stacked_on_url = None
408 cvs_root = cvs_module = None
409 elif rcstype == 'cvs':
410 url = None
411 stacked_on_url = None
412 [cvs_root, cvs_module] = arguments
413 else:
414 raise AssertionError("Unknown rcstype %r." % rcstype)
415 if target_rcstype == 'bzr':
416 target_id = int(target_id)364 target_id = int(target_id)
417 macaroon = None365 except ValueError:
418 elif target_rcstype == 'git':366 parser.error(
419 macaroon = Macaroon.deserialize(arguments.pop(0))367 'rcstype bzr requires target_id to be an integer')
420 else:368 stacked_on_url = args.stacked_on
421 raise AssertionError(369 macaroon = None
422 "Unknown target_rcstype %r." % target_rcstype)370 elif target_rcstype == 'git':
371 if args.macaroon is None:
372 parser.error('target_rcstype git requires --macaroon')
373 stacked_on_url = None
374 macaroon = args.macaroon
423 return cls(375 return cls(
424 target_id, rcstype, target_rcstype, url, cvs_root, cvs_module,376 target_id, rcstype, target_rcstype, url, cvs_root, cvs_module,
425 stacked_on_url, macaroon)377 stacked_on_url, macaroon)

Subscribers

People subscribed via source and target branches

to status/vote changes: