[PATCH v2] parsemail: Clarify exit codes
Daniel Axtens
dja at axtens.net
Thu Mar 22 01:00:28 AEDT 2018
jk reports that the patchwork error codes are really unhelpful for
correct integration with an MDA. In particular they make sorting out
failures into a separate queue very difficult. Make this better and
clearer.
Update the comment for parse_mail regarding return values and exceptions
to line up with how the function actually works and how we use the
results.
Update the tests. None of the existing tests should exit 1; they're
all 'expected' failures: unknown project etc. Also we removed the
exit(0) from the success path, so stop expecting that exception to
be raised.
Add a test for db errors - deliberately insert a mail twice - make
sure that exit(1)s.
Reported-by: Jeremy Kerr <jk at ozlabs.org>
Fixes: #171
Signed-off-by: Daniel Axtens <dja at axtens.net>
---
patchwork/management/commands/parsemail.py | 16 +++++++---
patchwork/parser.py | 9 +++++-
patchwork/tests/test_management.py | 51 +++++++++++++++++-------------
3 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/patchwork/management/commands/parsemail.py b/patchwork/management/commands/parsemail.py
index 52ec8bc56899..1ed12ae886cf 100644
--- a/patchwork/management/commands/parsemail.py
+++ b/patchwork/management/commands/parsemail.py
@@ -77,12 +77,20 @@ class Command(base.BaseCommand):
logger.warning("Broken email ignored")
return
+ # it's important to get exit codes correct here. The key is to allow
+ # proper separation of real errors vs expected 'failures'.
+ #
+ # patch/comment parsed: 0
+ # no parseable content found: 0
+ # db integrity/other db error: 1 (this will mean dups are logged, which
+ # might get annoying - we'll see)
+ # broken email (ValueError): 1 (this could also be noisy, if an issue
+ # we could use a different return code)
try:
result = parse_mail(mail, options['list_id'])
- if result:
- sys.exit(0)
- logger.warning('Failed to parse mail')
- sys.exit(1)
+ if result is None:
+ logger.warning('Nothing added to database')
except Exception:
logger.exception('Error when parsing incoming email',
extra={'mail': mail.as_string()})
+ sys.exit(1)
diff --git a/patchwork/parser.py b/patchwork/parser.py
index 805037c72d73..42159a69cb34 100644
--- a/patchwork/parser.py
+++ b/patchwork/parser.py
@@ -939,7 +939,14 @@ def parse_mail(mail, list_id=None):
list_id (str): Mailing list ID
Returns:
- None
+ patch/cover letter/comment
+ Or None if nothing is found in the mail
+ or X-P-H: ignore
+ or project not found
+
+ Raises:
+ ValueError if there is an error in parsing
+ Some db errors are passed through (e.g. IntegrityError)
"""
# some basic sanity checks
if 'From' not in mail:
diff --git a/patchwork/tests/test_management.py b/patchwork/tests/test_management.py
index f548fce3b8e5..9cdb1ea5ada9 100644
--- a/patchwork/tests/test_management.py
+++ b/patchwork/tests/test_management.py
@@ -40,30 +40,27 @@ class ParsemailTest(TestCase):
def test_missing_project_path(self):
path = os.path.join(TEST_MAIL_DIR, '0001-git-pull-request.mbox')
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=path)
+ call_command('parsemail', infile=path)
- self.assertEqual(exc.exception.code, 1)
+ count = models.Patch.objects.all().count()
+ self.assertEqual(count, 0)
def test_missing_project_stdin(self):
path = os.path.join(TEST_MAIL_DIR, '0001-git-pull-request.mbox')
sys.stdin.close()
sys.stdin = open(path)
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=None)
+ call_command('parsemail', infile=None)
sys.stdin.close()
- self.assertEqual(exc.exception.code, 1)
+ count = models.Patch.objects.all().count()
+ self.assertEqual(count, 0)
def test_valid_path(self):
project = utils.create_project()
utils.create_state()
path = os.path.join(TEST_MAIL_DIR, '0001-git-pull-request.mbox')
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=path, list_id=project.listid)
-
- self.assertEqual(exc.exception.code, 0)
+ call_command('parsemail', infile=path, list_id=project.listid)
count = models.Patch.objects.filter(project=project.id).count()
self.assertEqual(count, 1)
@@ -75,12 +72,9 @@ class ParsemailTest(TestCase):
path = os.path.join(TEST_MAIL_DIR, '0001-git-pull-request.mbox')
sys.stdin.close()
sys.stdin = open(path)
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=None,
- list_id=project.listid)
+ call_command('parsemail', infile=None, list_id=project.listid)
sys.stdin.close()
- self.assertEqual(exc.exception.code, 0)
count = models.Patch.objects.filter(project=project.id).count()
self.assertEqual(count, 1)
@@ -90,10 +84,7 @@ class ParsemailTest(TestCase):
utils.create_state()
path = os.path.join(TEST_MAIL_DIR, '0013-with-utf8-body.mbox')
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=path, list_id=project.listid)
-
- self.assertEqual(exc.exception.code, 0)
+ call_command('parsemail', infile=path, list_id=project.listid)
count = models.Patch.objects.filter(project=project.id).count()
self.assertEqual(count, 1)
@@ -105,15 +96,31 @@ class ParsemailTest(TestCase):
path = os.path.join(TEST_MAIL_DIR, '0013-with-utf8-body.mbox')
sys.stdin.close()
sys.stdin = open(path)
- with self.assertRaises(SystemExit) as exc:
- call_command('parsemail', infile=None,
- list_id=project.listid)
+ call_command('parsemail', infile=None, list_id=project.listid)
- self.assertEqual(exc.exception.code, 0)
+ count = models.Patch.objects.filter(project=project.id).count()
+ self.assertEqual(count, 1)
+
+ def test_dup_mail(self):
+ project = utils.create_project()
+ utils.create_state()
+
+ path = os.path.join(TEST_MAIL_DIR, '0001-git-pull-request.mbox')
+ call_command('parsemail', infile=path, list_id=project.listid)
count = models.Patch.objects.filter(project=project.id).count()
self.assertEqual(count, 1)
+ with self.assertRaises(SystemExit) as exc:
+ call_command('parsemail', infile=path, list_id=project.listid)
+
+ self.assertEqual(exc.exception.code, 1)
+
+ # this would be lovely but doesn't work because we caused an error in
+ # the transaction and we have no way to reset it
+ # count = models.Patch.objects.filter(project=project.id).count()
+ # self.assertEqual(count, 1)
+
class ParsearchiveTest(TestCase):
def test_invalid_path(self):
--
2.14.1
More information about the Patchwork
mailing list