From patchwork Wed Mar 21 14:00:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 888875 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 405sR82XTrz9s0w for ; Thu, 22 Mar 2018 01:20:52 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="MIV8swv/"; dkim-atps=neutral Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 405sR802bMzF1qM for ; Thu, 22 Mar 2018 01:20:52 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="MIV8swv/"; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=axtens.net (client-ip=2a00:1450:400c:c0c::235; helo=mail-wr0-x235.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="MIV8swv/"; dkim-atps=neutral Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 405s0K4D78zF0ws for ; Thu, 22 Mar 2018 01:00:57 +1100 (AEDT) Received: by mail-wr0-x235.google.com with SMTP id c24so5315870wrc.6 for ; Wed, 21 Mar 2018 07:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id; bh=05yVvDiPmB45NW3C7oHdXtShcUlBj3/OMxX+QomyLqc=; b=MIV8swv/zRIMqdJhaMJAWkULAG6rwWZhxZc3tXqOECO5Qm7svgekgnnvYPqvA2AwOF U+GR/QHA2R35MC3WGl+lBu14Z9MtABycKtpDBhbFKjXQ9Do2NbZy9fxAx2csDFJVyqhk hieIMHchq7x0ozdXeBXCXuQc4tI1E3y3qg9RM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=05yVvDiPmB45NW3C7oHdXtShcUlBj3/OMxX+QomyLqc=; b=O6kE7BGuKwGo2z4rQ3SoBHDXnWPrcJXl3xKBTe3nuUgoCtz/s7lwOgIqmuuYSpuhKO 1sDQC3a38hEN30ghvO+uw1FALCOD/Gy+ju2sRwb8i6Y64OljK1ReAZitwkMjaRDUwvBl Acrl9llicLSkCLOuWGwcVQcuYsR6z0YxCuBZY5hlKQ8G2HiU8ZdtiavmmoP/VotDEwcV 6cFy9jUt6RYxuhG3luP1xliZ2ZvALzFgQY6e8XtnO4r3AhDvcnGHNSwKwuyfXx/eW/93 eo80Yhh6YoM7swgbxS7sMK9KeGfa96BWATY0NopohlL7D47lR0vr9KffpiaV0Z5GLZeo omCw== X-Gm-Message-State: AElRT7G7I+cohv99wc8W3tN00ZREDJUQuVuiHJzjL5BvGdVPEaxf5v/i zm6JO0tKPQMoSPqw2f+z2DudbvZh6TE3xA== X-Google-Smtp-Source: AG47ELtiZbkhi5sRak5UOQN2xV5MjnDZfnSiHLQevIhqZF+qF37todkzF0E7fG+4LrxRk061gAKABw== X-Received: by 10.223.136.13 with SMTP id d13mr15976677wrd.271.1521640851444; Wed, 21 Mar 2018 07:00:51 -0700 (PDT) Received: from linkitivity.nonius.hsia ([88.157.144.190]) by smtp.gmail.com with ESMTPSA id j4sm6465438wrd.53.2018.03.21.07.00.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 21 Mar 2018 07:00:50 -0700 (PDT) From: Daniel Axtens To: patchwork@lists.ozlabs.org Subject: [PATCH v2] parsemail: Clarify exit codes Date: Thu, 22 Mar 2018 01:00:28 +1100 Message-Id: <20180321140028.26270-1-dja@axtens.net> X-Mailer: git-send-email 2.14.1 X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" 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 Fixes: #171 Signed-off-by: Daniel Axtens --- 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):