[PATCH 09/11] parsemail: Convert to a management command

[Andy Doan]

On 07/13/2016 04:40 AM, Stephen Finucane 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.
>
> Signed-off-by: Stephen Finucane <stephen.finucane at intel.com>

Couple of questions:

> Partial-bug: #17
> Closes-bug: #15
> ---
>  patchwork/bin/parsemail.py                 |  113 ----------------------------
>  patchwork/bin/parsemail.sh                 |    2 +-
>  patchwork/management/commands/parsemail.py |   63 +++++++++++++++
>  patchwork/parser.py                        |   12 ++--
>  patchwork/settings/base.py                 |   52 +++++++++++++
>  5 files changed, 122 insertions(+), 120 deletions(-)
>  delete mode 100755 patchwork/bin/parsemail.py
>  create mode 100644 patchwork/management/commands/parsemail.py
>
> diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py
> deleted file mode 100755
> index abcee04..0000000
> --- a/patchwork/bin/parsemail.py
> +++ /dev/null
> @@ -1,113 +0,0 @@
> -#!/usr/bin/env python
> -#
> -# Patchwork - automated patch tracking system
> -# Copyright (C) 2008 Jeremy Kerr <jk at 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..5a8ca58 100755
> --- a/patchwork/bin/parsemail.sh
> +++ b/patchwork/bin/parsemail.sh
> @@ -24,6 +24,6 @@ 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"
>
>  exit 0

I've never been a consumer of this script, but it looks like you can't 
pass options to it, so it will only read patches from stdin. Wouldn't be 
a little cleaner if it was like:

PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \
            DJANGO_SETTINGS_MODULE=patchwork.settings.production \
            exec "$PATCHWORK_BASE/patchwork/manage.py parsemail" $*

(NOTE i also added "exec" to be a little cleaner and exit with proper code)

> diff --git a/patchwork/management/commands/parsemail.py b/patchwork/management/commands/parsemail.py
> new file mode 100644
> index 0000000..ce6a63e
> --- /dev/null
> +++ b/patchwork/management/commands/parsemail.py
> @@ -0,0 +1,63 @@
> +# 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 django.utils import log

"log" looks like an unused import?

> +
> +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()})
> +            raise exc

Do you really want to raise an exception since logger.exception will 
show the stack trace? Maybe just a sys.exit(1)?