[PATCH] parsemail: Clarify exit codes

[Daniel Axtens]

Daniel Axtens <dja at axtens.net> writes:

> Daniel Axtens <dja at axtens.net> writes:
>
>> 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.
>>
>
> jk applied this to OzLabs and used it to verify the bugs fixed in
> stable/2.0, so I am considering that to be a pretty solid test, and
> am merging this to master and stable/2.0.

Hmm, nope, we need fixes to the tests first. v2 to come.

Regards,
Daniel

>
> Regards,
> Daniel
>
>> 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 ++++++++-
>>  2 files changed, 20 insertions(+), 5 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 c1f40a577fa3..019ca20bb159 100644
>> --- a/patchwork/parser.py
>> +++ b/patchwork/parser.py
>> @@ -922,7 +922,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:
>> -- 
>> 2.14.1