launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04006
[Merge] lp:~jelmer/launchpad/classify-code-import-errors into lp:launchpad
Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/classify-code-import-errors into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #616812 in Launchpad itself: "Should mark imports as invalid on some exceptions"
https://bugs.launchpad.net/launchpad/+bug/616812
For more details, see:
https://code.launchpad.net/~jelmer/launchpad/classify-code-import-errors/+merge/65207
Recognize a few specific cases of code imports failing - code imports that fail because the remote branch is somehow invalid (it has disappeared, the server is down, it requires authentication, ...) or that can not be imported because of (current) limitations in our toolchain (files with backslashes in their name, git repositories that contain submodules, etc).
This makes the code import worker look at the exception class of exceptions that are raised and based on that, return a different error code. The worker monitor can then set a more specific status for the code import event.
This is the first step towards fixing bug 616812 - reducing the amount of manual gardening of code imports.
--
https://code.launchpad.net/~jelmer/launchpad/classify-code-import-errors/+merge/65207
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/classify-code-import-errors into lp:launchpad.
=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py 2010-10-18 02:37:53 +0000
+++ lib/lp/code/enums.py 2011-06-20 14:31:27 +0000
@@ -791,35 +791,18 @@
An internal error occurred. This is a problem with Launchpad.
""")
- CHECKOUT_FAILURE = DBItem(220, """
- Source Checkout Failed
-
- Unable to checkout from the foreign version control
- system. The import details are probably incorrect or the
- remote server is down.
- """)
-
- IMPORT_FAILURE = DBItem(230, """
- Bazaar Import Failed
-
- The initial import failed to complete. It may be a bug in
- Launchpad's conversion software or a problem with the remote
- repository.
- """)
-
- UPDATE_FAILURE = DBItem(240, """
- Source Update Failed
-
- Unable to update the foreign version control system tree. This
- is probably a problem with the remote repository.
- """)
-
- SYNC_FAILURE = DBItem(250, """
- Bazaar Update Failed
-
- An update to the existing Bazaar import failed to complete. It
- may be a bug in Launchpad's conversion software or a problem
- with the remote repository.
+ FAILURE_INVALID = DBItem(220, """
+ Foreign branch invalid
+
+ The import failed because the foreign branch did not exist or
+ was not accessible.
+ """)
+
+ FAILURE_UNSUPPORTED_FEATURE = DBItem(230, """
+ Unsupported feature
+
+ The import failed because of missing feature support in
+ Bazaar or the Bazaar foreign branch support.
""")
RECLAIMED = DBItem(310, """
=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py 2011-06-17 09:56:57 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py 2011-06-20 14:31:27 +0000
@@ -998,7 +998,22 @@
raise AssertionError("unexpected rcs_type %r" % self.rcs_type)
source_details = self.factory.makeCodeImportSourceDetails(**args)
worker = self.makeImportWorker(source_details)
- self.assertRaises(NotBranchError, worker.run)
+ self.assertEqual(
+ CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
+
+ def test_invalid(self):
+ # If there is no branch in the target URL, exit with FAILURE_INVALID
+ worker = self.makeImportWorker(self.factory.makeCodeImportSourceDetails(
+ rcstype=self.rcstype, url="file:///path/non/existant"))
+ self.assertEqual(
+ CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
+
+ def test_unsupported_feature(self):
+ # If there is no branch in the target URL, exit with FAILURE_INVALID
+ worker = self.makeImportWorker(self.makeSourceDetails(
+ 'trunk', [('bzr\\doesnt\\support\\this', 'Original contents')]))
+ self.assertEqual(
+ CodeImportWorkerExitCode.FAILURE_UNSUPPORTED_FEATURE, worker.run())
def test_partial(self):
# Only config.codeimport.revisions_import_limit will be imported in a
=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2011-03-29 13:57:20 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2011-06-20 14:31:27 +0000
@@ -426,6 +426,38 @@
# callFinishJob did not swallow the error, this will fail the test.
return ret
+ def test_callFinishJobCallsFinishJobInvalid(self):
+ # If the argument to callFinishJob indicates that the subprocess
+ # exited with a code of CodeImportWorkerExitCode.FAILURE_INVALID, it
+ # calls finishJob with a status of FAILURE_INVALID.
+ worker_monitor = self.makeWorkerMonitorWithJob()
+ calls = self.patchOutFinishJob(worker_monitor)
+ ret = worker_monitor.callFinishJob(
+ makeFailure(
+ error.ProcessTerminated,
+ exitCode=CodeImportWorkerExitCode.FAILURE_INVALID))
+ self.assertEqual(calls, [CodeImportResultStatus.FAILURE_INVALID])
+ self.assertOopsesLogged([])
+ # We return the deferred that callFinishJob returns -- if
+ # callFinishJob did not swallow the error, this will fail the test.
+ return ret
+
+ def test_callFinishJobCallsFinishJobUnsupportedFeature(self):
+ # If the argument to callFinishJob indicates that the subprocess
+ # exited with a code of FAILURE_UNSUPPORTED_FEATURE, it
+ # calls finishJob with a status of FAILURE_UNSUPPORTED_FEATURE.
+ worker_monitor = self.makeWorkerMonitorWithJob()
+ calls = self.patchOutFinishJob(worker_monitor)
+ ret = worker_monitor.callFinishJob(
+ makeFailure(
+ error.ProcessTerminated,
+ exitCode=CodeImportWorkerExitCode.FAILURE_UNSUPPORTED_FEATURE))
+ self.assertEqual(calls, [CodeImportResultStatus.FAILURE_UNSUPPORTED_FEATURE])
+ self.assertOopsesLogged([])
+ # We return the deferred that callFinishJob returns -- if
+ # callFinishJob did not swallow the error, this will fail the test.
+ return ret
+
@suppress_stderr
def test_callFinishJobLogsTracebackOnFailure(self):
# When callFinishJob is called with a failure, it dumps the traceback
=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py 2011-06-17 09:56:57 +0000
+++ lib/lp/codehosting/codeimport/worker.py 2011-06-20 14:31:27 +0000
@@ -30,7 +30,10 @@
BzrDirFormat,
)
from bzrlib.errors import (
+ ConnectionError,
+ InvalidEntryName,
NoSuchFile,
+ NoRepositoryPresent,
NotBranchError,
)
from bzrlib.transport import get_transport
@@ -66,6 +69,8 @@
FAILURE = 1
SUCCESS_NOCHANGE = 2
SUCCESS_PARTIAL = 3
+ FAILURE_INVALID = 4
+ FAILURE_UNSUPPORTED_FEATURE = 5
class BazaarBranchStore:
@@ -457,7 +462,7 @@
def _doImport(self):
"""Perform the import.
- :return: True if the import actually imported some new revisions.
+ :return: A CodeImportWorkerExitCode
"""
raise NotImplementedError()
@@ -551,6 +556,16 @@
needs_bzr_tree = False
@property
+ def invalid_branch_exceptions(self):
+ """Exceptions that indicate no (valid) remote branch is present."""
+ raise NotImplementedError
+
+ @property
+ def unsupported_feature_exceptions(self):
+ """The exceptions to consider for unsupported features."""
+ raise NotImplementedError
+
+ @property
def probers(self):
"""The probers that should be tried for this import."""
raise NotImplementedError
@@ -578,20 +593,33 @@
except NotBranchError:
pass
else:
- raise NotBranchError(self.source_details.url)
+ self._logger.info("No branch found at remote location.")
+ return CodeImportWorkerExitCode.FAILURE_INVALID
remote_branch = format.open(transport).open_branch()
remote_branch_tip = remote_branch.last_revision()
inter_branch = InterBranch.get(remote_branch, bazaar_branch)
self._logger.info("Importing branch.")
- inter_branch.fetch(limit=self.getRevisionLimit())
- if bazaar_branch.repository.has_revision(remote_branch_tip):
- pull_result = inter_branch.pull(overwrite=True)
- if pull_result.old_revid != pull_result.new_revid:
- result = CodeImportWorkerExitCode.SUCCESS
- else:
- result = CodeImportWorkerExitCode.SUCCESS_NOCHANGE
- else:
- result = CodeImportWorkerExitCode.SUCCESS_PARTIAL
+ try:
+ inter_branch.fetch(limit=self.getRevisionLimit())
+ if bazaar_branch.repository.has_revision(remote_branch_tip):
+ pull_result = inter_branch.pull(overwrite=True)
+ if pull_result.old_revid != pull_result.new_revid:
+ result = CodeImportWorkerExitCode.SUCCESS
+ else:
+ result = CodeImportWorkerExitCode.SUCCESS_NOCHANGE
+ else:
+ result = CodeImportWorkerExitCode.SUCCESS_PARTIAL
+ except Exception, e:
+ if e.__class__ in self.unsupported_feature_exceptions:
+ self._logger.info(
+ "Unable to import branch because of limitations in Bazaar.")
+ self._logger.info(str(e))
+ return CodeImportWorkerExitCode.FAILURE_UNSUPPORTED_FEATURE
+ elif e.__class__ in self.invalid_branch_exceptions:
+ self._logger.info("Branch invalid: %s", e(str))
+ return CodeImportWorkerExitCode.FAILURE_INVALID
+ else:
+ raise
self._logger.info("Pushing local import branch to central store.")
self.pushBazaarBranch(bazaar_branch)
self._logger.info("Job complete.")
@@ -607,6 +635,22 @@
"""
@property
+ def invalid_branch_exceptions(self):
+ return [
+ NoRepositoryPresent,
+ NotBranchError,
+ ConnectionError,
+ ]
+
+ @property
+ def unsupported_feature_exceptions(self):
+ from bzrlib.plugins.git.fetch import SubmodulesRequireSubtrees
+ return [
+ InvalidEntryName,
+ SubmodulesRequireSubtrees,
+ ]
+
+ @property
def probers(self):
"""See `PullingImportWorker.probers`."""
from bzrlib.plugins.git import (
@@ -662,6 +706,20 @@
"""
@property
+ def invalid_branch_exceptions(self):
+ return [
+ NoRepositoryPresent,
+ NotBranchError,
+ ConnectionError,
+ ]
+
+ @property
+ def unsupported_feature_exceptions(self):
+ return [
+ InvalidEntryName,
+ ]
+
+ @property
def probers(self):
"""See `PullingImportWorker.probers`."""
from bzrlib.plugins.hg import HgProber
@@ -713,6 +771,22 @@
class BzrSvnImportWorker(PullingImportWorker):
"""An import worker for importing Subversion via bzr-svn."""
+ @property
+ def invalid_branch_exceptions(self):
+ return [
+ NoRepositoryPresent,
+ NotBranchError,
+ ConnectionError,
+ ]
+
+ @property
+ def unsupported_feature_exceptions(self):
+ from bzrlib.plugins.svn.errors import InvalidFileName
+ return [
+ InvalidEntryName,
+ InvalidFileName,
+ ]
+
def getRevisionLimit(self):
"""See `PullingImportWorker.getRevisionLimit`."""
return config.codeimport.svn_revisions_import_limit
=== modified file 'lib/lp/codehosting/codeimport/workermonitor.py'
--- lib/lp/codehosting/codeimport/workermonitor.py 2010-10-03 15:30:06 +0000
+++ lib/lp/codehosting/codeimport/workermonitor.py 2011-06-20 14:31:27 +0000
@@ -248,14 +248,20 @@
Different exit codes are presumed by Twisted to be errors, but are
different kinds of success for us.
"""
+ exit_code_map = {
+ CodeImportWorkerExitCode.SUCCESS_NOCHANGE:
+ CodeImportResultStatus.SUCCESS_NOCHANGE,
+ CodeImportWorkerExitCode.SUCCESS_PARTIAL:
+ CodeImportResultStatus.SUCCESS_PARTIAL,
+ CodeImportWorkerExitCode.FAILURE_UNSUPPORTED_FEATURE:
+ CodeImportResultStatus.FAILURE_UNSUPPORTED_FEATURE,
+ CodeImportWorkerExitCode.FAILURE_INVALID:
+ CodeImportResultStatus.FAILURE_INVALID,
+ }
if isinstance(reason, failure.Failure):
if reason.check(error.ProcessTerminated):
- if reason.value.exitCode == \
- CodeImportWorkerExitCode.SUCCESS_NOCHANGE:
- return CodeImportResultStatus.SUCCESS_NOCHANGE
- elif reason.value.exitCode == \
- CodeImportWorkerExitCode.SUCCESS_PARTIAL:
- return CodeImportResultStatus.SUCCESS_PARTIAL
+ return exit_code_map.get(reason.value.exitCode,
+ CodeImportResultStatus.FAILURE)
return CodeImportResultStatus.FAILURE
else:
return CodeImportResultStatus.SUCCESS
=== modified file 'scripts/code-import-worker.py'
--- scripts/code-import-worker.py 2010-04-27 19:48:39 +0000
+++ scripts/code-import-worker.py 2011-06-20 14:31:27 +0000
@@ -65,11 +65,11 @@
elif source_details.rcstype == 'hg':
load_optional_plugin('hg')
import_worker_cls = HgImportWorker
+ elif source_details.rcstype in ['cvs', 'svn']:
+ import_worker_cls = CSCVSImportWorker
else:
- if source_details.rcstype not in ['cvs', 'svn']:
- raise AssertionError(
- 'unknown rcstype %r' % source_details.rcstype)
- import_worker_cls = CSCVSImportWorker
+ raise AssertionError(
+ 'unknown rcstype %r' % source_details.rcstype)
import_worker = import_worker_cls(
source_details,
get_transport(config.codeimport.foreign_tree_store),
Follow ups
