Re: [PATCH] Only mark series as completed after cover letter has arrived
- Original Message -
> From: "Daniel Axtens"
> To: vkaba...@redhat.com, patchwork@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 hge 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
> > ---
> > 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 ..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(
>
Re: [PATCH v3 4/5] tox: Integrate tox-docker
Stephen Finucane writes:
> On Mon, 2019-08-26 at 14:25 +1000, Daniel Axtens wrote:
>> Stephen Finucane writes:
>>
>> > On Thu, 2019-08-22 at 23:52 +1000, Daniel Axtens wrote:
>> > > > > .. code-block:: shell
>> > > > >
>> > > > > - $ tox -e py27-django18
>> > > > > + $ tox -e py36-django21-mysql
>> > > >
>> > > > So I'm trying this out (finally!) and it seems to want to install all
>> > > > the dependencies locally before starting a container. I don't have the
>> > > > mysql bits installed, so it fails looking for `mysql_config`. Is this
>> > > > supposed to happen or am I Doing It Wrong?
>> > > >
>> > >
>> > > Ok, so on further analysis it looks like this is the designed behaviour:
>> > > that when running tox, all the python versions and local dependencies
>> > > would live on my laptop directly rather than in a docker container.
>> >
>> > Correct.
>> >
>> > > If so, I'm not a fan. I am not primarily a web, python, or database
>> > > developer and I like having all of that stuff live in an isolated docker
>> > > container. I especially like that it's also consistent for everyone who
>> > > wants to hack on patchwork - that they can run the full suite of tests
>> > > across all the supported versions with nothing more than docker and
>> > > docker-compose. tox-docker provides, afaict, no way to do this. (Also,
>> > > less universally, I run Ubuntu, not Fedora and getting multiple python
>> > > versions is a pain, as you can see from the dockerfiles.)
>> > >
>> > > What's the main problem that you're trying to solve here? Is it that you
>> > > have to type 'docker-compose run web --tox -e py36-django21' rather than
>> > > just 'tox -e py36-django21'?
>> >
>> > Personally, I'm finding I'm having to jump through a lot of hoops to
>> > get docker working as I expect, including things like needing to create
>> > the '.env' file so it uses the right UID, and it also takes _forever_
>> > to rebuild (which I need to do rather frequently as I tinker with
>> > dependencies). Finally, it doesn't work like I'd expect a Python thing
>> > to usually work, meaning whenever I go to tinker with Patchwork after a
>> > break, I need to re-learn how to test things. Given that I have an
>> > environment that already has most of the dependencies needed to run
>> > this, I'm not really getting any of the benefits docker provides but am
>> > seeing most of the costs.
>>
>> Fair enough. How does this sound:
>>
>> We copy the original tox.ini into tools/docker/
>>
>> We make the main tox file the tox file you suggested, so that on your
>> laptop you can run `tox -e whatever` and things will go well.
>>
>> In entry-point.sh, we intercept `--tox` and use the saved tox.ini file
>> for inside docker (tox -C tools/docker/tox.ini $@)
>>
>> We have to keep the two files in sync, but then we have both systems
>> working as expected, and we can clarify in documentation when to use
>> each of them.
>
> I don't think any of that is necessary. As things stand, running e.g.
> 'tox -e py27-django111' will continue to have the same behavior as
> previously. The only change will happen if you introduce an additional
> factor and call something like 'tox -e py27-django111-mysql'. That's
> not included in any of the default environmnents by default so I don't
> think it's an issue.
I thought you were changing the default environment list - there's
a change to envlist in tox.ini:
> [tox]
> minversion = 2.0
>-envlist = pep8,docs,py{27,34}-django111,py{35,36}-django{111,20,21,22}
>+envlist =
>pep8,docs,py{27,34}-django111-{mysql,postgres},py{35,36}-django{111,20,21,22}-{mysql,postgres}
>+skip_missing_interpreters = True
> skipsdist = True
I'd really like to keep `docker-compose run web --tox` working, but
otherwise I'm happy to make whatever changes make things easy for
you. Perhaps we could specify default envlist using the TOXENV
environment variable in entrypoint.sh? Then you could change the default
tox.ini environments with no issues...
Regards,
Daniel
>
> Stephen
>
>> If you don't like that I'm happy to explore the pure python driver plus
>> pyenv approach.
>>
>> Regards,
>> Daniel
>>
>> > How about we don't strip the 'web' Dockerfile to the bones and instead
>> > add this as an alternate approach to running tests? I get faster tests
>> > and you still get full isolation. Alternatively, we can switch to pure
>> > Python DB drivers (removing the need for non-Python dependencies) in
>> > our development requirements.txt and use pyenv to provide multiple
>> > Python versions? That avoids having two ways to do the same thing but
>> > does still mean you need a little work on your end, which might not be
>> > desirable.
>> >
>> > Stephen
>> >
>> > > Regards,
>> > > Daniel
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
