Message ID | BLU437-SMTP12F17F9D9BB1C5FB6E9D1A3E10@phx.gbl |
---|---|
State | Superseded |
Headers | show |
Hi Stephen, > PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > - "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > + "$PATCHWORK_BASE/patchwork/manage.py parsemail" $@ > Looks good :) > +# NOTE(stephenfin): We must return 0 here. For more information, refer > +# to https://patchwork.ozlabs.org/patch/602248/ > exit 0 Could we just put the reason inline? Perhaps: "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." > +from email import message_from_file > +import logging > +from optparse import make_option As I mentioned for v2, this should be moved to argparse for Django 1.10 compatibility. Regards, Daniel
> Hi Stephen, > > > PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > > - "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > > + "$PATCHWORK_BASE/patchwork/manage.py parsemail" $@ > > > Looks good :) > > > +# NOTE(stephenfin): We must return 0 here. For more information, refer > > +# to https://patchwork.ozlabs.org/patch/602248/ > > exit 0 > > Could we just put the reason inline? Perhaps: > > "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." OK, I'll do that when I merge. > > +from email import message_from_file > > +import logging > > +from optparse import make_option > As I mentioned for v2, this should be moved to argparse for Django 1.10 > compatibility. This breaks compatibility with Django 1.6 though, which we stillneed to support. I'll probably implement a compat library, but I'lldo this in a follow-up. Is that OK by you? Stephen
Stephen Finucane <stephenfinucane@hotmail.com> writes: >> > +from email import message_from_file >> > +import logging >> > +from optparse import make_option >> As I mentioned for v2, this should be moved to argparse for Django 1.10 >> compatibility. > > This breaks compatibility with Django 1.6 though, which we stillneed to support. I'll probably implement a compat library, but I'lldo this in a follow-up. Is that OK by you? Oh, I didn't realise that. A follow-up is totally OK in this situation then. Reviewed-by: Daniel Axtens <dja@axtens.net> Daniel > Stephen
> +++ b/patchwork/bin/parsemail.sh > @@ -24,6 +24,8 @@ PATCHWORK_BASE=`readlink -e $BIN_DIR/../..` > > PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > - "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > + "$PATCHWORK_BASE/patchwork/manage.py parsemail" $@ So, I went to test this and found a couple of other issues: - If you quote the entire thing, you try to execute the program manage.py\ parsemail - not what you want. The quote needs to be moved to after manage.py. - manage.py lives in PATCHWORK_BASE, not PATCHWORK_BASE/patchwork/ - This will fail in the Python 3 case. (It'd also be nice to be able to override DJANGO_SETTINGS_MODULE.) Both of these are only really issues when you're testing, rather than production issues. Regards, Daniel > > +# NOTE(stephenfin): We must return 0 here. 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 0000000..b1599e3 > --- /dev/null > +++ b/patchwork/management/commands/parsemail.py > @@ -0,0 +1,61 @@ > +# Patchwork - automated patch tracking system > +# Copyright (C) 2016 Intel Corporation > +# > +# 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 email import message_from_file > +import logging > +from optparse import make_option > +import sys > + > +from django.core.management import base > + > +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' > + args = '<infile>' # Django < 1.8 compatibility > + 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.' > + ), > + ) > + > + def handle(self, *args, **options): > + # Attempt to parse the path if provided, and fallback to stdin if not > + if args: > + logger.info('Parsing mail loaded by filename') > + with open(args[0]) as file_: > + mail = message_from_file(file_) > + else: > + logger.info('Parsing mail loaded from stdin') > + mail = 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 as exc: > + logger.exception('Error when parsing incoming email', > + extra={'mail': mail.as_string()}) > diff --git a/patchwork/parser.py b/patchwork/parser.py > index 1805df8..51de997 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): > @@ -599,7 +599,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: > @@ -608,7 +608,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 > @@ -651,7 +651,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 > @@ -683,7 +683,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 > > @@ -704,7 +704,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 b78ed4b..a32710f 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 > # > > -- > 2.7.4 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On 02 Sep 11:21, Daniel Axtens wrote: > > +++ b/patchwork/bin/parsemail.sh > > @@ -24,6 +24,8 @@ PATCHWORK_BASE=`readlink -e $BIN_DIR/../..` > > > > PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > > - "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > > + "$PATCHWORK_BASE/patchwork/manage.py parsemail" $@ > > So, I went to test this and found a couple of other issues: > - If you quote the entire thing, you try to execute the program > manage.py\ parsemail - not what you want. The quote needs to be > moved to after manage.py. > > - manage.py lives in PATCHWORK_BASE, not PATCHWORK_BASE/patchwork/ > > - This will fail in the Python 3 case. (It'd also be nice to be able to > override DJANGO_SETTINGS_MODULE.) Both of these are only really > issues when you're testing, rather than production issues. All valid issues - whoops. I'll post a new version of these two patches and submit the rest of them, which Andy has already reviewed. Stephen
Stephen Finucane <stephenfinucane@hotmail.com> writes: > All valid issues - whoops. I'll post a new version of these two patches > and submit the rest of them, which Andy has already reviewed. While you're at it flake8 reports: patchwork/management/commands/parsemail.py:59:29: F841 local variable 'exc' is assigned to but never used Regards, Daniel
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py deleted file mode 100755 index b35b1f6..0000000 --- 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 <jk@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 9973392..ec72488 100755 --- a/patchwork/bin/parsemail.sh +++ b/patchwork/bin/parsemail.sh @@ -24,6 +24,8 @@ PATCHWORK_BASE=`readlink -e $BIN_DIR/../..` PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ DJANGO_SETTINGS_MODULE=patchwork.settings.production \ - "$PATCHWORK_BASE/patchwork/bin/parsemail.py" + "$PATCHWORK_BASE/patchwork/manage.py parsemail" $@ +# NOTE(stephenfin): We must return 0 here. 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 0000000..b1599e3 --- /dev/null +++ b/patchwork/management/commands/parsemail.py @@ -0,0 +1,61 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2016 Intel Corporation +# +# 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 email import message_from_file +import logging +from optparse import make_option +import sys + +from django.core.management import base + +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' + args = '<infile>' # Django < 1.8 compatibility + 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.' + ), + ) + + def handle(self, *args, **options): + # Attempt to parse the path if provided, and fallback to stdin if not + if args: + logger.info('Parsing mail loaded by filename') + with open(args[0]) as file_: + mail = message_from_file(file_) + else: + logger.info('Parsing mail loaded from stdin') + mail = 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 as exc: + logger.exception('Error when parsing incoming email', + extra={'mail': mail.as_string()}) diff --git a/patchwork/parser.py b/patchwork/parser.py index 1805df8..51de997 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): @@ -599,7 +599,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: @@ -608,7 +608,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 @@ -651,7 +651,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 @@ -683,7 +683,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 @@ -704,7 +704,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 b78ed4b..a32710f 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 #