[PATCH] Only mark series as completed after cover letter has arrived
Daniel Axtens
dja at axtens.net
Wed Aug 21 17:07:11 AEST 2019
Hi,
I'm going through old patches/mails, apologies for the huuuuge delay.
> 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.
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
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.
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