[PATCH] Only mark series as completed after cover letter has arrived
Veronika Kabatova
vkabatov at redhat.com
Tue Sep 3 00:32:46 AEST 2019
----- Original Message -----
> From: "Daniel Axtens" <dja at axtens.net>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Wednesday, August 21, 2019 9:07:11 AM
> Subject: Re: [PATCH] Only mark series as completed after cover letter has arrived
>
> Hi,
>
> I'm going through old patches/mails, apologies for the huuuuge delay.
>
No worries... I totally forgot all the details in the meanwhile though so
this is going to be exciting. I was also on vacation so obligatory sorry for
the late reply as well :)
> > People often put metadata into the cover letter. These are needed by
> > various CI systems built upon Patchwork, and they are unable to process
> > the series if a cover letter is expected to arrive and it didn't arrive
> > at the time of processing. Email delaying is normal and the CI systems
> > are currently unable to exclude series based on the `received_all`
> > attribute.
> >
> > Add a new DB field (not exposed in the API) that says "a cover letter
> > should arrive but it didn't yet" and use this field when assessing
> > whether the series are completed or not. In the end, a cover letter is
> > a vital part of the series.
>
> Technically it is exposed in that it is added to the test for
> received_all, but I guess you mean it isn't exposed by itself.
>
Yes.
> So initially, I wondered about series that deliberately don't have cover
> letters. e.g. https://patchwork.ozlabs.org/patch/1144273/
> But it looks like you've already thought of this!
>
> What about series that are posted in reply to an earlier version? There
> was one of these just the other day on linuxppc:
> https://lore.kernel.org/linuxppc-dev/20190820175801.GA9420@archlinux-threadripper/T/#m85f5c9d80692f6248cdc4f3e0b0e0defb487bf27
> https://patchwork.ozlabs.org/patch/1150491/ (v2)
> https://patchwork.ozlabs.org/patch/1148897/ (v1)
>
> Indeed, if I import that series, it would appear that cover_expected
> does get confused:
>
> >>> Patch.objects.get(id=1).series.cover_expected
> False
> >>> Patch.objects.get(id=2).series.cover_expected
> True
>
Hm, good catch! Digging into it, it looks like this is only a problem for
series without cover that are sent as replies.
> git send-email (and email generally) is really unfortunately designed
> here. I don't know if there's a good solution to this, but I suspect we
> have a hack for it somewhere already, probably parsing the
> git-send-email message-id to detect when there's a new series. If the
> message is in reply to old series only, we're not expecting a cover
> letter.
>
I found
https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L1061
which looks like what you're referring to. I'll use this check and add a test
too (or do something else if it turns out I'm brain dead after the vacation)
and send an updated version. I'm currently busy with Plumbers and CKI hackfest
preparations so it will take me a while till I get to it though.
Thanks,
Veronika
> Regards,
> Daniel
>
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> > ---
> > Most of the people needing this change currently have a workaround in
> > their pipelines.
> >
> > Besides protecting automation from race conditions in email delivery,
> > the patch also complements the proposed tagging feature changes -- one
> > of the changes (issue #113) deals with adding cover letter tags to each
> > patch in the series. Together with the planned flattening of DB
> > structure around Submission/Patch/Cover letter, unifying the behavior
> > around cover letters and patches just makes sense.
> > ---
> > .../0034_add_cover_expected_to_series.py | 22 +++++
> > patchwork/models.py | 23 ++++-
> > patchwork/signals.py | 23 ++++-
> > .../series/base-cover-letter-delayed.mbox | 94 +++++++++++++++++++
> > patchwork/tests/test_series.py | 21 ++++-
> > 5 files changed, 178 insertions(+), 5 deletions(-)
> > create mode 100644
> > patchwork/migrations/0034_add_cover_expected_to_series.py
> > create mode 100644 patchwork/tests/series/base-cover-letter-delayed.mbox
> >
> > diff --git a/patchwork/migrations/0034_add_cover_expected_to_series.py
> > b/patchwork/migrations/0034_add_cover_expected_to_series.py
> > new file mode 100644
> > index 00000000..85cf7281
> > --- /dev/null
> > +++ b/patchwork/migrations/0034_add_cover_expected_to_series.py
> > @@ -0,0 +1,22 @@
> > +# -*- coding: utf-8 -*-
> > +from __future__ import unicode_literals
> > +
> > +from django.db import migrations, models
> > +
> > +
> > +# Only add the model field. Since the cover letter should arrive within
> > approx.
> > +# 10 mins form the patches (delayed emails happen), we actually don't
> > expect to
> > +# wait for any old emails. They should be already there.
> > +class Migration(migrations.Migration):
> > +
> > + dependencies = [
> > + ('patchwork', '0033_remove_patch_series_model'),
> > + ]
> > +
> > + operations = [
> > + migrations.AddField(
> > + model_name='series',
> > + name='cover_expected',
> > + field=models.BooleanField(default=False),
> > + ),
> > + ]
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index a7eee4db..d23b0d03 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -7,6 +7,7 @@
> > from collections import Counter
> > from collections import OrderedDict
> > import datetime
> > +import email.parser
> > import random
> > import re
> >
> > @@ -380,6 +381,10 @@ class Submission(FilenameMixin, EmailMixin,
> > models.Model):
> >
> > class CoverLetter(Submission):
> >
> > + def delete(self, *args, **kwargs):
> > + self.series.cover_expected = True
> > + super(CoverLetter, self).delete(**kwargs)
> > +
> > def get_absolute_url(self):
> > return reverse('cover-detail', kwargs={'cover_id': self.id})
> >
> > @@ -627,6 +632,7 @@ class Series(FilenameMixin, models.Model):
> > cover_letter = models.OneToOneField(CoverLetter,
> > related_name='series',
> > null=True,
> > on_delete=models.CASCADE)
> >e + cover_expected = models.BooleanField(default=False)
> >
> > # metadata
> > name = models.CharField(max_length=255, blank=True, null=True,
> > @@ -658,7 +664,7 @@ class Series(FilenameMixin, models.Model):
> >
> > @property
> > def received_all(self):
> > - return self.total <= self.received_total
> > + return (self.total <= self.received_total) and not
> > self.cover_expected
> >
> > def add_cover_letter(self, cover):
> > """Add a cover letter to the series.
> > @@ -699,13 +705,24 @@ class Series(FilenameMixin, models.Model):
> > if self.name == name:
> > self.name = self._format_name(cover)
> >
> > + self.cover_expected = False
> > +
> > self.save()
> >
> > def add_patch(self, patch, number):
> > """Add a patch to the series."""
> > # both user defined names and cover letter-based names take
> > precedence
> > - if not self.name and number == 1:
> > - self.name = patch.name # keep the prefixes for patch-based
> > names
> > + if number == 1:
> > + if not self.name: # keep the prefixes for patch-based names
> > + self.name = patch.name
> > +
> > + # Find out if we expect a cover letter to arrive later
> > + if not self.cover_letter:
> > + in_reply_to = email.parser.Parser().parsestr(
> > + patch.headers, True).get('In-Reply-To')
> > + if in_reply_to:
> > + self.cover_expected = True
> > +
> > self.save()
> >
> > patch.series = self
> > diff --git a/patchwork/signals.py b/patchwork/signals.py
> > index 666199b6..56157ae5 100644
> > --- a/patchwork/signals.py
> > +++ b/patchwork/signals.py
> > @@ -211,7 +211,7 @@ def create_series_created_event(sender, instance,
> > created, raw, **kwargs):
> >
> >
> > @receiver(pre_save, sender=Patch)
> > -def create_series_completed_event(sender, instance, raw, **kwargs):
> > +def create_series_completed_by_patch_event(sender, instance, raw,
> > **kwargs):
> >
> > # NOTE(stephenfin): It's actually possible for this event to be fired
> > # multiple times for a given series. To trigger this case, you would
> > need
> > @@ -240,5 +240,26 @@ def create_series_completed_event(sender, instance,
> > raw, **kwargs):
> >
> > # we can't use "series.received_all" here since we haven't actually
> > saved
> > # the instance yet so we duplicate that logic here but with an offset
> > + if (instance.series.received_total + 1) >= instance.series.total and \
> > + not instance.series.cover_expected:
> > + create_event(instance.series)
> > +
> > +
> > + at receiver(pre_save, sender=CoverLetter)
> > +def create_series_completed_by_cover_event(sender, instance, raw,
> > **kwargs):
> > +
> > + def create_event(series):
> > + return Event.objects.create(
> > + category=Event.CATEGORY_SERIES_COMPLETED,
> > + project=series.project,
> > + series=series)
> > +
> > + # Don't trigger for items loaded from fixtures or new items
> > + if raw or not instance.pk:
> > + return
> > +
> > + # We can't use "series.received_all" here since we haven't actually
> > saved
> > + # the instance yet so we duplicate that logic here. No need to check
> > + # series.cover_expected since the cover has just arrived.
> > if (instance.series.received_total + 1) >= instance.series.total:
> > create_event(instance.series)
> > diff --git a/patchwork/tests/series/base-cover-letter-delayed.mbox
> > b/patchwork/tests/series/base-cover-letter-delayed.mbox
> > new file mode 100644
> > index 00000000..e5bb7d1f
> > --- /dev/null
> > +++ b/patchwork/tests/series/base-cover-letter-delayed.mbox
> > @@ -0,0 +1,94 @@
> > +From stephenfinucane at gmail.com Sun Sep 11 23:22:13 2016
> > +Return-Path: <stephenfinucane at gmail.com>
> > +From: Stephen Finucane <stephenfinucane at gmail.com>
> > +To: stephenfinucane at hotmail.com
> > +Subject: [PATCH 1/2] test: Add some lorem ipsum
> > +Date: Sun, 11 Sep 2016 23:22:03 +0100
> > +Message-ID: <1473632524-8585-2-git-send-email-stephenfinucane at gmail.com>
> > +X-Mailer: git-send-email 2.7.4
> > +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane at gmail.com>
> > +References: <1473632524-8585-1-git-send-email-stephenfinucane at gmail.com>
> > +MIME-Version: 1.0
> > +Content-Type: text/plain
> > +Content-Length: 670
> > +Lines: 22
> > +
> > +From: Stephen Finucane <stephenfinucane at hotmail.com>
> > +
> > +---
> > + test.txt | 7 +++++++
> > + 1 file changed, 7 insertions(+)
> > +
> > +diff --git a/test.txt b/test.txt
> > +index f75ba05..a6c61c0 100644
> > +--- a/test.txt
> > ++++ b/test.txt
> > +@@ -1 +1,8 @@
> > + Hello, world.
> > ++
> > ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > ++tellus commodo bibendum ut vel dolor.
> > +--
> > +2.7.4
> > +
> > +
> > +From stephenfinucane at gmail.com Sun Sep 11 23:22:16 2016
> > +Return-Path: <stephenfinucane at gmail.com>
> > +From: Stephen Finucane <stephenfinucane at gmail.com>
> > +To: stephenfinucane at hotmail.com
> > +Subject: [PATCH 2/2] test: Convert to Markdown
> > +Date: Sun, 11 Sep 2016 23:22:04 +0100
> > +Message-ID: <1473632524-8585-3-git-send-email-stephenfinucane at gmail.com>
> > +X-Mailer: git-send-email 2.7.4
> > +In-Reply-To: <1473632524-8585-1-git-send-email-stephenfinucane at gmail.com>
> > +References: <1473632524-8585-1-git-send-email-stephenfinucane at gmail.com>
> > +MIME-Version: 1.0
> > +Content-Type: text/plain
> > +Content-Length: 1345
> > +Lines: 40
> > +
> > +From: Stephen Finucane <stephenfinucane at hotmail.com>
> > +
> > +---
> > + test.md | 8 ++++++++
> > + test.txt | 8 --------
> > + 2 files changed, 8 insertions(+), 8 deletions(-)
> > + create mode 100644 test.md
> > + delete mode 100644 test.txt
> > +
> > +diff --git a/test.md b/test.md
> > +new file mode 100644
> > +index 0000000..e5ff90e
> > +--- /dev/null
> > ++++ b/test.md
> > +@@ -0,0 +1,8 @@
> > ++# Hello, world
> > ++
> > ++Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > ++augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > ++Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > ++id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > ++eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > ++tellus commodo bibendum ut vel dolor.
> > +diff --git a/test.txt b/test.txt
> > +deleted file mode 100644
> > +index a6c61c0..0000000
> > +--- a/test.txt
> > ++++ /dev/null
> > +@@ -1,8 +0,0 @@
> > +-Hello, world.
> > +-
> > +-Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras eget
> > eleifend
> > +-augue. Nullam at metus venenatis, laoreet neque nec, convallis mauris.
> > +-Pellentesque aliquam at nisi et laoreet. Duis non nisl venenatis, rhoncus
> > risus
> > +-id, elementum felis. In hac habitasse platea dictumst. Nam sit amet
> > maximus
> > +-eros. Nam quis ligula ut tortor egestas bibendum. Nunc sed purus sit amet
> > +-tellus commodo bibendum ut vel dolor.
> > +--
> > +2.7.4
> > +
> > +
> > diff --git a/patchwork/tests/test_series.py
> > b/patchwork/tests/test_series.py
> > index f5e6b5b0..8eda806b 100644
> > --- a/patchwork/tests/test_series.py
> > +++ b/patchwork/tests/test_series.py
> > @@ -531,7 +531,8 @@ class SeriesTotalTest(_BaseTestCase):
> > def test_complete(self):
> > """Series received with expected number of patches.
> >
> > - Parse a series where all patches are received as expected.
> > + Parse a series where all patches and a cover letter are received
> > as
> > + expected.
> >
> > Input:
> >
> > @@ -548,6 +549,24 @@ class SeriesTotalTest(_BaseTestCase):
> > series = patches[0].series
> > self.assertTrue(series.received_all)
> >
> > + def test_cover_delayed(self):
> > + """
> > + Check that series are mark as not complete if a cover is expected
> > but
> > + didn't arrive yet.
> > +
> > + Input:
> > +
> > + - [PATCH 1/2] test: Add some lorem ipsum
> > + - [PATCH 2/2] test: Convert to Markdown
> > + """
> > + covers, patches, _ = self._parse_mbox(
> > + 'base-cover-letter-delayed.mbox', [0, 2, 0])
> > +
> > + self.assertSerialized(patches, [2])
> > +
> > + series = patches[0].series
> > + self.assertFalse(series.received_all)
> > +
> > def test_extra_patches(self):
> > """Series received with additional patches.
> >
> > --
> > 2.17.2
> >
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
>
More information about the Patchwork
mailing list