| Message ID | 1448712886-3221-11-git-send-email-mchehab@osg.samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Stephen Finucane |
| Headers | show |
On 28 Nov 10:14, Mauro Carvalho Chehab wrote: > When things are broken at parsemail, we need to be able to > debug it. So, add some messages to it, in order to allow > checking what it actually did, and to let it return 1 if > an email got skipped by parsemail. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> Some minor changes below - mostly dropping duplicated stuff and using logging instead of 'print'. If you're happy for me to fix, I can do so. Stephen > --- > patchwork/bin/parsemail.py | 19 +++++++++++++++---- > patchwork/bin/parsemail.sh | 2 +- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py > index 5d6ddf45d264..71d284b273b0 100755 > --- a/patchwork/bin/parsemail.py > +++ b/patchwork/bin/parsemail.py > @@ -79,6 +79,9 @@ def find_project(mail): > except Project.DoesNotExist: > pass > > + if project is None: > + project = Project.objects.get(listid = 'linux-media.vger.kernel.org') > + This is too specific to include - did you actually mean to include it? BTW I'm open to ideas to provide a default project: the '--list-id' parameter for this script (`./parsemail.py --help`) currently overrides the search by header functionality - maybe this should change? > return project > > def find_author(mail): > @@ -143,6 +146,9 @@ def find_pull_request(content): > match = git_re.search(content) > if match: > return match.group(1) > + > + print "not a git pull request" > + This should be changed to LOG.debug (see below). > return None > > def try_decode(payload, charset): > @@ -392,13 +398,16 @@ def parse_mail(mail): > > # some basic sanity checks > if 'From' not in mail: > - return 0 > + print "From: is missing" > + return 1 I think this is a good idea. It's so good, in fact, that I did it myself a few months back :) [1] There's some differences though so the two efforts can be merged. > > if 'Subject' not in mail: > - return 0 > + print "Subject: is missing" > + return 1 > > if 'Message-Id' not in mail: > - return 0 > + print "Message-Id: is missing" > + return 1 > > hint = mail.get('X-Patchwork-Hint', '').lower() > if hint == 'ignore': > @@ -407,7 +416,7 @@ def parse_mail(mail): > project = find_project(mail) > if project is None: > print "no project found" > - return 0 > + return 1 > > msgid = mail.get('Message-Id').strip() > > @@ -431,6 +440,7 @@ def parse_mail(mail): > patch.delegate = delegate > try: > patch.save() > + print "patch saved" > except Exception, ex: > print str(ex) > > @@ -444,6 +454,7 @@ def parse_mail(mail): > comment.msgid = msgid > try: > comment.save() > + print "comment saved" > except Exception, ex: > print str(ex) > > diff --git a/patchwork/bin/parsemail.sh b/patchwork/bin/parsemail.sh > index 9973392de9d4..c8220a799bba 100755 > --- a/patchwork/bin/parsemail.sh > +++ b/patchwork/bin/parsemail.sh > @@ -26,4 +26,4 @@ PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > > -exit 0 > +exit $@ Good idea - always wondered why we didn't make these exit codes mean something. > -- > 2.5.0 [1] https://github.com/getpatchwork/patchwork/commit/9a1802aa49299384e68d0fd2a3dd46c0885b29eb
On 04 Jan 09:45, Finucane, Stephen wrote: > On 28 Nov 10:14, Mauro Carvalho Chehab wrote: > > When things are broken at parsemail, we need to be able to > > debug it. So, add some messages to it, in order to allow > > checking what it actually did, and to let it return 1 if > > an email got skipped by parsemail. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > Some minor changes below - mostly dropping duplicated stuff and using > logging instead of 'print'. If you're happy for me to fix, I can do > so. > > Stephen > > > --- > > patchwork/bin/parsemail.py | 19 +++++++++++++++---- > > patchwork/bin/parsemail.sh | 2 +- > > 2 files changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py > > index 5d6ddf45d264..71d284b273b0 100755 > > --- a/patchwork/bin/parsemail.py > > +++ b/patchwork/bin/parsemail.py > > @@ -79,6 +79,9 @@ def find_project(mail): > > except Project.DoesNotExist: > > pass > > > > + if project is None: > > + project = Project.objects.get(listid = 'linux-media.vger.kernel.org') > > + > > This is too specific to include - did you actually mean to include it? > BTW I'm open to ideas to provide a default project: the '--list-id' > parameter for this script (`./parsemail.py --help`) currently > overrides the search by header functionality - maybe this should change? Dropped this. > > return project > > > > def find_author(mail): > > @@ -143,6 +146,9 @@ def find_pull_request(content): > > match = git_re.search(content) > > if match: > > return match.group(1) > > + > > + print "not a git pull request" > > + > > This should be changed to LOG.debug (see below). Changed to LOG.debug. > > return None > > > > def try_decode(payload, charset): > > @@ -392,13 +398,16 @@ def parse_mail(mail): > > > > # some basic sanity checks > > if 'From' not in mail: > > - return 0 > > + print "From: is missing" > > + return 1 > > I think this is a good idea. It's so good, in fact, that I did it > myself a few months back :) [1] There's some differences though > so the two efforts can be merged. > > > > > if 'Subject' not in mail: > > - return 0 > > + print "Subject: is missing" > > + return 1 > > > > if 'Message-Id' not in mail: > > - return 0 > > + print "Message-Id: is missing" > > + return 1 > > > > hint = mail.get('X-Patchwork-Hint', '').lower() > > if hint == 'ignore': > > @@ -407,7 +416,7 @@ def parse_mail(mail): > > project = find_project(mail) > > if project is None: > > print "no project found" > > - return 0 > > + return 1 > > > > msgid = mail.get('Message-Id').strip() > > > > @@ -431,6 +440,7 @@ def parse_mail(mail): > > patch.delegate = delegate > > try: > > patch.save() > > + print "patch saved" > > except Exception, ex: > > print str(ex) > > > > @@ -444,6 +454,7 @@ def parse_mail(mail): > > comment.msgid = msgid > > try: > > comment.save() > > + print "comment saved" > > except Exception, ex: > > print str(ex) > > > > diff --git a/patchwork/bin/parsemail.sh b/patchwork/bin/parsemail.sh > > index 9973392de9d4..c8220a799bba 100755 > > --- a/patchwork/bin/parsemail.sh > > +++ b/patchwork/bin/parsemail.sh > > @@ -26,4 +26,4 @@ PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ > > DJANGO_SETTINGS_MODULE=patchwork.settings.production \ > > "$PATCHWORK_BASE/patchwork/bin/parsemail.py" > > > > -exit 0 > > +exit $@ > > Good idea - always wondered why we didn't make these exit codes mean > something. > > > -- > > 2.5.0 > > [1] https://github.com/getpatchwork/patchwork/commit/9a1802aa49299384e68d0fd2a3dd46c0885b29eb Merged with above changes.
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py index 5d6ddf45d264..71d284b273b0 100755 --- a/patchwork/bin/parsemail.py +++ b/patchwork/bin/parsemail.py @@ -79,6 +79,9 @@ def find_project(mail): except Project.DoesNotExist: pass + if project is None: + project = Project.objects.get(listid = 'linux-media.vger.kernel.org') + return project def find_author(mail): @@ -143,6 +146,9 @@ def find_pull_request(content): match = git_re.search(content) if match: return match.group(1) + + print "not a git pull request" + return None def try_decode(payload, charset): @@ -392,13 +398,16 @@ def parse_mail(mail): # some basic sanity checks if 'From' not in mail: - return 0 + print "From: is missing" + return 1 if 'Subject' not in mail: - return 0 + print "Subject: is missing" + return 1 if 'Message-Id' not in mail: - return 0 + print "Message-Id: is missing" + return 1 hint = mail.get('X-Patchwork-Hint', '').lower() if hint == 'ignore': @@ -407,7 +416,7 @@ def parse_mail(mail): project = find_project(mail) if project is None: print "no project found" - return 0 + return 1 msgid = mail.get('Message-Id').strip() @@ -431,6 +440,7 @@ def parse_mail(mail): patch.delegate = delegate try: patch.save() + print "patch saved" except Exception, ex: print str(ex) @@ -444,6 +454,7 @@ def parse_mail(mail): comment.msgid = msgid try: comment.save() + print "comment saved" except Exception, ex: print str(ex) diff --git a/patchwork/bin/parsemail.sh b/patchwork/bin/parsemail.sh index 9973392de9d4..c8220a799bba 100755 --- a/patchwork/bin/parsemail.sh +++ b/patchwork/bin/parsemail.sh @@ -26,4 +26,4 @@ PYTHONPATH="$PATCHWORK_BASE":"$PATCHWORK_BASE/lib/python:$PYTHONPATH" \ DJANGO_SETTINGS_MODULE=patchwork.settings.production \ "$PATCHWORK_BASE/patchwork/bin/parsemail.py" -exit 0 +exit $@
When things are broken at parsemail, we need to be able to debug it. So, add some messages to it, in order to allow checking what it actually did, and to let it return 1 if an email got skipped by parsemail. Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> --- patchwork/bin/parsemail.py | 19 +++++++++++++++---- patchwork/bin/parsemail.sh | 2 +- 2 files changed, 16 insertions(+), 5 deletions(-)