On 23 Sep 10:06, Daniel Axtens wrote: > Management comands allow applications to register their own actions with > 'manage.py'. This provides some advantages, like automatically > configuring Django (removing the need for 'django.setup' calls) and > removing the need to set the PYTHON_PATH. The 'parsemail' script is a > natural fit for this type of application. Migrate 'parsemail' to a > management command. > > This includes some extensive work on logging configuration, as logging > is moved from code into settings. In addition, it removes a lot of the > customizable logging previously introduced in the parsemail command, in > favour of modifications to the settings files. > > Originally-by: Stephen Finucane <stephenfinuc...@hotmail.com> > Partial-bug: #17 > Closes-bug: #15 > [dja: significant changes: > - fix merge conflicts with my changes to parsemail.sh > - py3 binary file email parsing > - make stdin tests close and re-open stdin] > Signed-off-by: Daniel Axtens <d...@axtens.net>
Reviewed-by: Stephen Finucane <stephenfinuc...@hotmail.com> > --- > > Stephen: the changes here are significant enough that I thought it > might be inappropriate to keep your signed-off-by until you've had > a chance to review them. If you're happy with them I'm more than > happy for you to add your sign-off when you merge it. > --- > patchwork/bin/parsemail.py | 114 > ----------------------------- > patchwork/bin/parsemail.sh | 8 +- > patchwork/management/commands/parsemail.py | 84 +++++++++++++++++++++ > patchwork/parser.py | 12 +-- > patchwork/settings/base.py | 60 +++++++++++++-- > patchwork/tests/__init__.py | 23 ++++++ > patchwork/tests/test_management.py | 83 +++++++++++++++++++++ > patchwork/tests/test_parser.py | 4 +- > patchwork/tests/utils.py | 2 +- > 9 files changed, 260 insertions(+), 130 deletions(-) > delete mode 100755 patchwork/bin/parsemail.py > create mode 100644 patchwork/management/commands/parsemail.py > create mode 100644 patchwork/tests/test_management.py > > diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py > deleted file mode 100755 > index b35b1f62ef9a..000000000000 > --- a/patchwork/bin/parsemail.py > +++ /dev/null > @@ -1,114 +0,0 @@ > -#!/usr/bin/env python > -# > -# Patchwork - automated patch tracking system > -# Copyright (C) 2008 Jeremy Kerr <j...@ozlabs.org> > -# > -# This file is part of the Patchwork package. > -# > -# Patchwork is free software; you can redistribute it and/or modify > -# it under the terms of the GNU General Public License as published by > -# the Free Software Foundation; either version 2 of the License, or > -# (at your option) any later version. > -# > -# Patchwork is distributed in the hope that it will be useful, > -# but WITHOUT ANY WARRANTY; without even the implied warranty of > -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > -# GNU General Public License for more details. > -# > -# You should have received a copy of the GNU General Public License > -# along with Patchwork; if not, write to the Free Software > -# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > - > -from __future__ import absolute_import > - > -import argparse > -from email import message_from_file > -import logging > -import sys > - > -import django > -from django.conf import settings > -from django.utils.log import AdminEmailHandler > - > -from patchwork.parser import parse_mail > - > -LOGGER = logging.getLogger(__name__) > - > -VERBOSITY_LEVELS = { > - 'debug': logging.DEBUG, > - 'info': logging.INFO, > - 'warning': logging.WARNING, > - 'error': logging.ERROR, > - 'critical': logging.CRITICAL > -} > - > - > -extra_error_message = ''' > -== Mail > - > -%(mail)s > - > - > -== Traceback > - > -''' > - > - > -def setup_error_handler(): > - """Configure error handler. > - > - Ensure emails are send to settings.ADMINS when errors are > - encountered. > - """ > - if settings.DEBUG: > - return > - > - mail_handler = AdminEmailHandler() > - mail_handler.setLevel(logging.ERROR) > - mail_handler.setFormatter(logging.Formatter(extra_error_message)) > - > - logger = logging.getLogger('patchwork') > - logger.addHandler(mail_handler) > - > - return logger > - > - > -def main(args): > - django.setup() > - logger = setup_error_handler() > - parser = argparse.ArgumentParser() > - > - def list_logging_levels(): > - """Give a summary of all available logging levels.""" > - return sorted(list(VERBOSITY_LEVELS.keys()), > - key=lambda x: VERBOSITY_LEVELS[x]) > - > - parser.add_argument('infile', nargs='?', type=argparse.FileType('r'), > - default=sys.stdin, help='input mbox file (a filename > ' > - 'or stdin)') > - > - group = parser.add_argument_group('Mail parsing configuration') > - group.add_argument('--list-id', help='mailing list ID. If not supplied ' > - 'this will be extracted from the mail headers.') > - group.add_argument('--verbosity', choices=list_logging_levels(), > - help='debug level', default='info') > - > - args = vars(parser.parse_args()) > - > - logging.basicConfig(level=VERBOSITY_LEVELS[args['verbosity']]) > - > - mail = message_from_file(args['infile']) > - try: > - result = parse_mail(mail, args['list_id']) > - if result: > - return 0 > - return 1 > - except: > - if logger: > - logger.exception('Error when parsing incoming email', extra={ > - 'mail': mail.as_string(), > - }) > - raise > - > -if __name__ == '__main__': > - sys.exit(main(sys.argv)) > diff --git a/patchwork/bin/parsemail.sh b/patchwork/bin/parsemail.sh > index 241f18c6bbc9..b6e11457bbc0 100755 > --- a/patchwork/bin/parsemail.sh > +++ b/patchwork/bin/parsemail.sh > @@ -31,6 +31,12 @@ if [ -z $DJANGO_SETTINGS_MODULE ]; then > fi > > PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > - $PW_PYTHON "$PATCHWORK_BASE/patchwork/bin/parsemail.py" $@ > + $PW_PYTHON "$PATCHWORK_BASE/manage.py" parsemail $@ > > +# NOTE(stephenfin): We must return 0 here. When parsemail is used as a > +# delivery command from a mail server like postfix (as it is intended > +# to be), a non-zero exit code will cause a bounce message to be > +# returned to the user. We don't want to do that for a parse error, so > +# always return 0. For more information, refer to > +# https://patchwork.ozlabs.org/patch/602248/ > exit 0 > diff --git a/patchwork/management/commands/parsemail.py > b/patchwork/management/commands/parsemail.py > new file mode 100644 > index 000000000000..d454ce544644 > --- /dev/null > +++ b/patchwork/management/commands/parsemail.py > @@ -0,0 +1,84 @@ > +# Patchwork - automated patch tracking system > +# Copyright (C) 2016 Stephen Finucane <stephenfinuc...@hotmail.com> > +# > +# This file is part of the Patchwork package. > +# > +# Patchwork is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# Patchwork is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with Patchwork; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + > +import argparse > +import email > +import logging > +from optparse import make_option > +import sys > + > +import django > +from django.core.management import base > +from django.utils import six > + > +from patchwork.parser import parse_mail > + > +logger = logging.getLogger(__name__) > + > + > +class Command(base.BaseCommand): > + help = 'Parse an mbox file and store any patch/comment found.' > + > + if django.VERSION < (1, 8): > + args = '<infile>' > + option_list = base.BaseCommand.option_list + ( > + make_option( > + '--list-id', > + help='mailing list ID. If not supplied, this will be ' > + 'extracted from the mail headers.'), > + ) > + else: > + def add_arguments(self, parser): > + parser.add_argument( > + 'infile', > + nargs='?', > + type=str, > + default=None, > + help='input mbox file (a filename or stdin)') > + parser.add_argument( > + '--list-id', > + help='mailing list ID. If not supplied, this will be ' > + 'extracted from the mail headers.') > + > + def handle(self, *args, **options): > + infile = args[0] if args else options['infile'] > + > + if infile: > + logger.info('Parsing mail loaded by filename') > + if six.PY3: > + with open(infile, 'rb') as file_: > + mail = email.message_from_binary_file(file_) > + else: > + with open(infile) as file_: > + mail = email.message_from_file(file_) > + else: > + logger.info('Parsing mail loaded from stdin') > + if six.PY3: > + mail = email.message_from_binary_file(sys.stdin.buffer) Ah...good catch. > + else: > + mail = email.message_from_file(sys.stdin) > + try: > + result = parse_mail(mail, options['list_id']) > + if result: > + sys.exit(0) > + logger.warning('Failed to parse mail') > + sys.exit(1) > + except Exception: > + logger.exception('Error when parsing incoming email', > + extra={'mail': mail.as_string()}) > diff --git a/patchwork/parser.py b/patchwork/parser.py > index 1b4cab1eb1a8..eefdb6f94156 100644 > --- a/patchwork/parser.py > +++ b/patchwork/parser.py > @@ -42,7 +42,7 @@ _hunk_re = re.compile(r'^\@\@ -\d+(?:,(\d+))? > \+\d+(?:,(\d+))? \@\@') > _filename_re = re.compile(r'^(---|\+\+\+) (\S+)') > list_id_headers = ['List-ID', 'X-Mailing-List', 'X-list'] > > -LOGGER = logging.getLogger(__name__) > +logger = logging.getLogger(__name__) > > > def normalise_space(str): > @@ -654,7 +654,7 @@ def parse_mail(mail, list_id=None): > > hint = mail.get('X-Patchwork-Hint', '').lower() > if hint == 'ignore': > - LOGGER.debug("Ignoring email due to 'ignore' hint") > + logger.debug("Ignoring email due to 'ignore' hint") > return > > if list_id: > @@ -663,7 +663,7 @@ def parse_mail(mail, list_id=None): > project = find_project_by_header(mail) > > if project is None: > - LOGGER.error('Failed to find a project for email') > + logger.error('Failed to find a project for email') > return > > # parse content > @@ -706,7 +706,7 @@ def parse_mail(mail, list_id=None): > delegate=delegate, > state=find_state(mail)) > patch.save() > - LOGGER.debug('Patch saved') > + logger.debug('Patch saved') > > return patch > elif x == 0: # (potential) cover letters > @@ -738,7 +738,7 @@ def parse_mail(mail, list_id=None): > submitter=author, > content=message) > cover_letter.save() > - LOGGER.debug('Cover letter saved') > + logger.debug('Cover letter saved') > > return cover_letter > > @@ -759,7 +759,7 @@ def parse_mail(mail, list_id=None): > submitter=author, > content=message) > comment.save() > - LOGGER.debug('Comment saved') > + logger.debug('Comment saved') > > return comment > > diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py > index b78ed4b0fd64..a32710f1d02b 100644 > --- a/patchwork/settings/base.py > +++ b/patchwork/settings/base.py > @@ -125,6 +125,8 @@ STATICFILES_DIRS = [ > # Third-party application settings > # > > +# rest_framework > + > try: > # django rest framework isn't a standard package in most distros, so > # don't make it compulsory > @@ -136,17 +138,65 @@ try: > except ImportError: > pass > > -# > -# Third-party application settings > -# > - > -# rest_framework > > REST_FRAMEWORK = { > 'DEFAULT_VERSIONING_CLASS': > 'rest_framework.versioning.NamespaceVersioning' > } > > # > +# Logging settings > +# > + > +LOGGING = { > + 'version': 1, > + 'disable_existing_loggers': False, > + 'formatters': { > + 'email': { > + 'format': '== Mail\n\n%(mail)s\n\n== Traceback\n', > + }, > + }, > + 'filters': { > + 'require_debug_false': { > + '()': 'django.utils.log.RequireDebugFalse', > + }, > + 'require_debug_true': { > + '()': 'django.utils.log.RequireDebugTrue', > + }, > + }, > + 'handlers': { > + 'console': { > + 'level': 'DEBUG', > + 'filters': ['require_debug_true'], > + 'class': 'logging.StreamHandler', > + }, > + 'mail_admins': { > + 'level': 'ERROR', > + 'filters': ['require_debug_false'], > + 'class': 'django.utils.log.AdminEmailHandler', > + 'formatter': 'email', > + 'include_html': True, > + }, > + }, > + 'loggers': { > + 'django': { > + 'handlers': ['console'], > + 'level': 'INFO', > + 'propagate': True, > + }, > + 'patchwork.parser': { > + 'handlers': ['console'], > + 'level': 'DEBUG', > + 'propagate': False, > + }, > + 'patchwork.management.commands': { > + 'handlers': ['console', 'mail_admins'], > + 'level': 'INFO', > + 'propagate': True, > + }, > + }, > +} > + > +# > # Patchwork settings > # > > diff --git a/patchwork/tests/__init__.py b/patchwork/tests/__init__.py > index e69de29bb2d1..f6b9104de392 100644 > --- a/patchwork/tests/__init__.py > +++ b/patchwork/tests/__init__.py > @@ -0,0 +1,23 @@ > +# Patchwork - automated patch tracking system > +# Copyright (C) 2016 Stephen Finucane <stephenfinuc...@hotmail.com> > +# > +# This file is part of the Patchwork package. > +# > +# Patchwork is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# Patchwork is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with Patchwork; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + > +import os > + > +TEST_MAIL_DIR = os.path.join(os.path.dirname(__file__), 'mail') > +TEST_PATCH_DIR = os.path.join(os.path.dirname(__file__), 'patches') > diff --git a/patchwork/tests/test_management.py > b/patchwork/tests/test_management.py > new file mode 100644 > index 000000000000..5f97aa76866f > --- /dev/null > +++ b/patchwork/tests/test_management.py > @@ -0,0 +1,83 @@ > +# Patchwork - automated patch tracking system > +# Copyright (C) 2016 Stephen Finucane <stephenfinuc...@hotmail.com> > +# > +# This file is part of the Patchwork package. > +# > +# Patchwork is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# Patchwork is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with Patchwork; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + > +import os > +import sys > + > +from django.core.management import call_command > +from django.test import TestCase > + > +from patchwork import models > +from patchwork.tests import TEST_MAIL_DIR > +from patchwork.tests import utils > + > + > +class ParsemailTest(TestCase): > + > + def test_invalid_path(self): > + # this can raise IOError, CommandError, or FileNotFoundError, > + # depending of the versions of Python and Django used. Just > + # catch a generic exception > + with self.assertRaises(Exception): > + call_command('parsemail', infile='xyz123random') > + > + 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) > + > + self.assertEqual(exc.exception.code, 1) > + > + 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) > + > + self.assertEqual(exc.exception.code, 1) > + > + 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) > + > + count = models.Patch.objects.filter(project=project.id).count() > + self.assertEqual(count, 1) > + > + def test_valid_stdin(self): > + project = utils.create_project() > + utils.create_state() > + > + 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) > + > + self.assertEqual(exc.exception.code, 0) > + > + count = models.Patch.objects.filter(project=project.id).count() > + self.assertEqual(count, 1) > diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py > index 17e0e7ae3468..c0e3bb61725a 100644 > --- a/patchwork/tests/test_parser.py > +++ b/patchwork/tests/test_parser.py > @@ -39,6 +39,7 @@ from patchwork.parser import parse_mail as _parse_mail > from patchwork.parser import parse_pull_request > from patchwork.parser import parse_series_marker > from patchwork.parser import split_prefixes > +from patchwork.tests import TEST_MAIL_DIR > from patchwork.tests.utils import create_project > from patchwork.tests.utils import create_state > from patchwork.tests.utils import create_user > @@ -46,9 +47,6 @@ from patchwork.tests.utils import read_patch > from patchwork.tests.utils import SAMPLE_DIFF > > > -TEST_MAIL_DIR = os.path.join(os.path.dirname(__file__), 'mail') > - > - > def read_mail(filename, project=None): > """Read a mail from a file.""" > file_path = os.path.join(TEST_MAIL_DIR, filename) > diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py > index 29455d2bdc2e..23b0c87cda56 100644 > --- a/patchwork/tests/utils.py > +++ b/patchwork/tests/utils.py > @@ -32,6 +32,7 @@ from patchwork.models import Patch > from patchwork.models import Person > from patchwork.models import Project > from patchwork.models import State > +from patchwork.tests import TEST_PATCH_DIR > > SAMPLE_DIFF = """--- /dev/null 2011-01-01 00:00:00.000000000 +0800 > +++ a 2011-01-01 00:00:00.000000000 +0800 > @@ -39,7 +40,6 @@ SAMPLE_DIFF = """--- /dev/null 2011-01-01 > 00:00:00.000000000 +0800 > +a > """ > SAMPLE_CONTENT = 'Hello, world.' > -TEST_PATCH_DIR = os.path.join(os.path.dirname(__file__), 'patches') > > > def read_patch(filename, encoding=None): > -- > 2.7.4 > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork