Re: [PATCH v4] api: Add comments to patch and cover endpoints

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Friday, April 27, 2018 5:37:57 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
> 
> On Wed, 2018-04-25 at 19:33 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> 
> Two comments below. I've actually addressed these already with my own
> patch. Assuming you're happy with said patch, I can squash it with this
> one and merge them.
> 

Either way works for me, and your follow-up patch looks good to me. I'll
send my ack right away.

Thanks,
Veronika 

> Cheers,
> Stephen
> 
> > ---
> >  patchwork/api/comment.py   |  75 ++
> >  patchwork/api/cover.py |  13 ++-
> >  patchwork/api/patch.py |  16 ++-
> >  patchwork/models.py|   3 +
> >  patchwork/tests/api/test_comment.py| 115
> >  +
> >  patchwork/tests/api/test_cover.py  |  11 +-
> >  patchwork/tests/api/test_patch.py  |  19 +++-
> >  patchwork/urls.py  |  11 ++
> >  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |   8 ++
> >  9 files changed, 262 insertions(+), 9 deletions(-)
> >  create mode 100644 patchwork/api/comment.py
> >  create mode 100644 patchwork/tests/api/test_comment.py
> >  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > 
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 000..fbb7cc8
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,75 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > USA
> > +
> > +import email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > +
> > +subject = SerializerMethodField()
> > +headers = SerializerMethodField()
> > +submitter = PersonSerializer(read_only=True)
> > +
> > +def get_subject(self, comment):
> > +return email.parser.Parser().parsestr(comment.headers,
> > +  True).get('Subject', '')
> > +
> > +def get_headers(self, comment):
> > +headers = {}
> > +
> > +if comment.headers:
> > +parsed = email.parser.Parser().parsestr(comment.headers, True)
> > +for key in parsed.keys():
> > +headers[key] = parsed.get_all(key)
> > +# Let's return a single string instead of a list if only
> > one
> > +# header with this key is present
> > +if len(headers[key]) == 1:
> > +headers[key] = headers[key][0]
> > +
> > +return headers
> > +
> > +class Meta:
> > +model = Comment
> > +fields = ('id', 'msgid', 'date', 'subject', 'submitter',
> > 'content',
> > +  'headers')
> > +read_only_fields = fields
> > +
> > +
> > +class CommentList(ListAPIView):
> > +""

Re: [PATCH] skip original Content-Transfer-Encoding for mbox

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: patchwork@lists.ozlabs.org, vkaba...@redhat.com, "yuri volchkov" 
> <yuri.volch...@gmail.com>
> Sent: Monday, May 7, 2018 5:57:55 PM
> Subject: [PATCH] skip original Content-Transfer-Encoding for mbox
> 
> In the commit 01b9cbb9 all original mail headers are copied into the
> resulted mbox file. This means that some headers are going to be
> present twice in the generated mbox. That is fine unless the original
> email arrived in base64 encoding.
> 
> Apparently git relies on the latest Content-Transfer-Encoding key. And
> since downloaded patch's actual encoding is '7bit', git fails to apply
> it with the message 'Patch is empty'.
> 
> Since patchwork adds a proper 'Content-Transfer-Encoding' anyways,
> let's skip this field while copying headers from the original mail
> 
> Signed-off-by: Yuri Volchkov <yuri.volch...@gmail.com>
> ---
> 
> Daniel Axtens writes: Yuri sent me an email saying his message didn't
> reach the list, so I'm forwarding this on his behalf. Veronika: you
> authored 01b9cbb9 so perhaps you could take a look to see if this is the
> best way. My guess is that this is probably right but you might have
> other thoughts.
> 

Hm, I didn't meet with any base64-encoded emails that weren't autoconverted
on their way so I didn't run into this issue. I agree that since the 
Content-Transfer-Encoding header is added based on the charset (which we
pass on message creation), skipping the original header is both the easiest
and correct way to solve this problem.

Acked-by: Veronika Kabatova <vkaba...@redhat.com>


> ---
>  patchwork/views/utils.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index f5ff43c..2357ab8 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -99,6 +99,8 @@ def _submission_to_mbox(submission):
>  
>  orig_headers = HeaderParser().parsestr(str(submission.headers))
>  for key, val in orig_headers.items():
> +if key == 'Content-Transfer-Encoding':
> +continue
>  mail[key] = val
>  
>  if 'Date' not in mail:
> --
> 2.17.0
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH v4] api: Add comments to patch and cover endpoints

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: "Veronika Kabatova" <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Wednesday, May 2, 2018 6:22:34 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
> 
> >> Notice that it's /patch/1/ for mbox, checks, etc, but /covers/1/ for
> >> comments.
> >> 
> >> Is this intentional - am I missing something? - or is this supposed to
> >> be /patch/1/ like the others?
> >> 
> >
> > Ookay, this is pretty embarrassing. reverse() looks to be confused when
> > you have multiple matches for the view and kwargs passed, ignoring the
> > origin of the request. I definitely didn't notice it - and Stephen probably
> > didn't either since he didn't bring it up.
> >
> > Simply changing "pk" to unique identifiers fixes it for me. Can you verify
> > it does so on your instance as well? I'll send a patch if it does.
> 
> I'm not really sure what you mean by this.
> 
> I can get the result I had in mind by changing the url pattern name in
> api_1_1_patterns in urls.py (api-comment-list -> api-patch-comment-list
> and api-cover-comment-list) - see below.
> 
> But I can't seem to do it by changing pk. However, I will readily admit
> that I have always struggled with both urlpatterns and the rest API - so
> feel free to propose whatever you had in mind and I will test it!
> 

You have to change kwargs in the api as well and fix the submission in
queryset retrieval there too. Your solution is much easier and less
error-prone so I'd go with that :)

It will need the same change in tests too. Do you want to add it or should
I send the patch for both changes?


Veronika


> Regards,
> Daniel
> 
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index 7c80064c8df0..99cf9e68e093 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -46,7 +46,7 @@ class
> CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
>  
>  def get_comments(self, cover):
>  return self.context.get('request').build_absolute_uri(
> -reverse('api-comment-list', kwargs={'pk': cover.id}))
> +reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
>  
>  class Meta:
>  model = CoverLetter
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index d1931c013545..028d68544ea6 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -94,7 +94,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
>  
>  def get_comments(self, patch):
>  return self.context.get('request').build_absolute_uri(
> -reverse('api-comment-list', kwargs={'pk': patch.id}))
> +reverse('api-patch-comment-list', kwargs={'pk': patch.id}))
>  
>  def get_check(self, instance):
>  return instance.combined_check_state
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 1dc4ffc2b7d3..e90de6b2c6ef 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -283,10 +283,10 @@ if settings.ENABLE_REST_API:
>  api_1_1_patterns = [
>  url(r'^patches/(?P[^/]+)/comments/$',
>  api_comment_views.CommentList.as_view(),
> -name='api-comment-list'),
> +name='api-patch-comment-list'),
>  url(r'^covers/(?P[^/]+)/comments/$',
>  api_comment_views.CommentList.as_view(),
> -name='api-comment-list'),
> +name='api-cover-comment-list'),
>  ]
>  
>  urlpatterns += [
> 
> 
> >
> > Thanks,
> > Veronika
> >
> >> Regards,
> >> Daniel
> >> 
> >> vkaba...@redhat.com writes:
> >> 
> >> > From: Veronika Kabatova <vkaba...@redhat.com>
> >> >
> >> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> >> > ---
> >> >  patchwork/api/comment.py   |  75 ++
> >> >  patchwork/api/cover.py |  13 ++-
> >> >  patchwork/api/patch.py |  16 ++-
> >> >  patchwork/models.py|   3 +
> >> >  patchwork/tests/api/test_comment.py| 115
> >> >  +
> >> >  patchwork/tests/api/test_cover.py  |  11 +-
> >> >  patchwork/tests/api/test_patch.py  |  19 +++-
> >> >  patchwork/urls.py  |  11 ++
> >> >  .../notes/comments-api-b7dff6ee4ce04c9b

Re: [PATCH v4] api: Add comments to patch and cover endpoints

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, April 30, 2018 6:55:11 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
> 
> Apologies if this is a silly question, but I was looking at this to
> check the perfomance, and I noticed that in the patch list endpoint
> (/api/patches/), it seems to be linking to the 'covers' comments
> endpoint rather than the 'patches' api endpoint:
> 
> ...
> "delegate": null,
> "mbox": "http://localhost:8000/patch/1/mbox/;,
> "series": [
> ...
> ],
> "comments": "http://localhost:8000/api/covers/1/comments/;,
> "check": "pending",
> "checks": "http://localhost:8000/api/patches/1/checks/;,
> "tags": {}
> ...
> 
> Notice that it's /patch/1/ for mbox, checks, etc, but /covers/1/ for
> comments.
> 
> Is this intentional - am I missing something? - or is this supposed to
> be /patch/1/ like the others?
> 

Ookay, this is pretty embarrassing. reverse() looks to be confused when
you have multiple matches for the view and kwargs passed, ignoring the
origin of the request. I definitely didn't notice it - and Stephen probably
didn't either since he didn't bring it up.

Simply changing "pk" to unique identifiers fixes it for me. Can you verify
it does so on your instance as well? I'll send a patch if it does.

Thanks,
Veronika

> Regards,
> Daniel
> 
> vkaba...@redhat.com writes:
> 
> > From: Veronika Kabatova <vkaba...@redhat.com>
> >
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > ---
> >  patchwork/api/comment.py   |  75 ++
> >  patchwork/api/cover.py |  13 ++-
> >  patchwork/api/patch.py |  16 ++-
> >  patchwork/models.py|   3 +
> >  patchwork/tests/api/test_comment.py| 115
> >  +
> >  patchwork/tests/api/test_cover.py  |  11 +-
> >  patchwork/tests/api/test_patch.py  |  19 +++-
> >  patchwork/urls.py  |  11 ++
> >  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |   8 ++
> >  9 files changed, 262 insertions(+), 9 deletions(-)
> >  create mode 100644 patchwork/api/comment.py
> >  create mode 100644 patchwork/tests/api/test_comment.py
> >  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> >
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 000..fbb7cc8
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,75 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > USA
> > +
> > +import email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > +
> > +subject = SerializerMethodField()
> > +headers = SerializerMethodField()
> > +submitter = PersonSerializer(read_only=True)
> > +
> > +def get_subject(self, comment):
> > +return email.parser.Parser().parsestr(comment.headers,
> > +   

Re: [PATCH] Avoid timezone confusion

[Veronika Kabatova]

Hi!

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, January 15, 2018 1:52:33 AM
> Subject: Re: [PATCH] Avoid timezone confusion
> 
> vkaba...@redhat.com writes:
> 
> > From: Veronika Kabatova <vkaba...@redhat.com>
> >
> > Patchwork saves patches, comments etc with UTC timezone and reports
> > this time when opening the patch details. However, internally generated
> > processes such as events are reported with the instance's local time.
> > There's nothing wrong with that and making PW timezone-aware would add
> > useless complexity, but in a world-wide collaboration a lot of confusion
> > may arise as the timezone is not reported at all. Instance's local time
> > might be very different from the local time of CI integrating with PW,
> > which is different from the local time of person dealing with it etc.
> >
> > For submission views, just add 'UTC' strings. For API responses, change
> > the serializers to still use ISO 8601 format but with timezone
> > information suffix [Z+-HHMM] - UTC for submissions and PW's local
> > timezone for internal events.
> 
> Thanks for your patch! It looks very comprehensive - thanks especially
> for including documentation!!
> 
> Would you prefer if we made internal events UTC? I think that might be
> preferable, but I haven't thought about it in great detail just yet.
> 

That would work too. Then we could only document "hey, all times are
in UTC" and things would be definitely more consistent and easier to
work with.

> >  
> >  Responses are returned as JSON. Blank fields are returned as ``null``,
> >  rather
> > -than being omitted. Timestamps use the ISO 8601 format::
> > +than being omitted. Returned timestamps use the ISO 8601 format including
> > the
> > +timezone information::
> >  
> > --MM-DDTHH:MM:SSZ
> > +-MM-DDTHH:MM:SS[Z+-HH:MM]
> > +
> > +.. note::
> > +
> > +Timezone suffix is only provided in responses to avoid users'
> > confusion and
> > +to allow proper coordination with CI tooling. Underlying data are
> > +timezone-naive and can't be filtered based on timezones, therefore
> > requests
> > +made to the API should not use the timezone suffix.
> >
> 
> In particular, I wonder if using UTC everywhere would make things like
> this simpler?
> 
> As I said I haven't put an enormous deal of thought into it, and I
> haven't done a lot of work with TZ in python before so I don't know if
> it would be crazy to implement. I just wanted your thoughts on it.
> 

Hm, let me think about it... We can set the default TIME_ZONE setting to
UTC and either

a) document that if people override it they will get inconsistent times
   and it's their problem to deal with, or

b) accept if people want to override it and use that timezone when parsing
   email submissions instead of the UTC, but then we would still need to
   report used timezone somehow because noone except instance admin would
   know what the timezone actually is (which is exactly what we want to
   avoid)


option a) is IMHO cleaner and really easy to implement too. I can send an
updated patch if you agree with it.

Thanks,
Veronika

> Regards,
> Daniel
> 
> >  Requests should use either query parameters or form-data, depending on the
> >  method. Further information is provided `below `__.
> > diff --git a/docs/deployment/installation.rst
> > b/docs/deployment/installation.rst
> > index a570dd8..38e12d1 100644
> > --- a/docs/deployment/installation.rst
> > +++ b/docs/deployment/installation.rst
> > @@ -173,7 +173,8 @@ this also:
> >  .. code-block:: shell
> >  
> > $ sudo apt-get install -y python3-django python3-psycopg2 \
> > -   python3-djangorestframework python3-django-filters
> > +   python3-djangorestframework python3-django-filters \
> > +   python3-tz
> >  
> >  .. tip::
> >  
> > diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> > index b37d6e0..4854a82 100644
> > --- a/patchwork/api/check.py
> > +++ b/patchwork/api/check.py
> > @@ -17,6 +17,7 @@
> >  # along with Patchwork; if not, write to the Free Software
> >  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> >  USA
> >  
> > +from django.utils import timezone
> >  from rest_framework.exceptions import PermissionDenied
> >  from rest_framework.generics import ListCreateAPIView
> >  from rest_framework.generics import RetrieveAPIView
> > @@ -63,6 +64,7 @@ class CheckSeriali

Re: [PATCH v2] Avoid timezone confusion

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, January 17, 2018 8:22:27 PM
> Subject: Re: [PATCH v2] Avoid timezone confusion
> 
> On Wed, 2018-01-17 at 19:18 +, Stephen Finucane wrote:
> > On Tue, 2018-01-16 at 18:58 +0100, vkaba...@redhat.com wrote:
> > > From: Veronika Kabatova <vkaba...@redhat.com>
> > > 
> > > Patchwork saves patches, comments etc with UTC timezone and reports
> > > this time when opening the patch details. However, internally
> > > generated
> > > processes such as events are reported with the instance's local
> > > time.
> > > There's nothing wrong with that and making PW timezone-aware would
> > > add
> > > useless complexity, but in a world-wide collaboration a lot of
> > > confusion
> > > may arise as the timezone is not reported at all. Instance's local
> > > time
> > > might be very different from the local time of CI integrating with
> > > PW,
> > > which is different from the local time of person dealing with it
> > > etc.
> > > 
> > > Use UTC everywhere by default instead of UTC for sumbissions and
> > > local
> > > timezone for internally generated events (which is not reported).
> > > 
> > > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > What effect does this have on existing information in the database?
> > Does this mean they'll all remain UTC+12 or are they stored in UTC
> > format already? The Django docs [1] would lead me to suggest the
> > former, given that we don't have USE_TZ set to True.
> > 
> > I guess you can test that by setting up a deployment, creating
> > information, then switching this option over. I'd do this myself but
> > I'm not going to have time this week :(
> 

The effect of the change for the existing data is the same as if the admin 
decided
to override the default TIME_ZONE setting. In the end that's what this change 
does,
move the instance time to UTC.

If the TIME_ZONE setting is overriden (production.py) nothing changes, we are 
only
changing the default.


To elaborate more, as we use timezone-naive format, the details depend on 
database
backend and what underlying type from it Django uses to store datetimes.

For mysql, they are saved as 'datetime' and this type doesn't know timezones and
returns what you feed it. Django doesn't modify the times either, generated time
is simply passed as a string. If you change timezone, 'datetime' is still the
same. So submissions will have the right time, but internally triggered changes
keep the original localized time and we can't really modify them since we can't
retrieve the original timezone used.

For postgres it's the exact opposite. Datetimes are stored as 'timestamp with
time zone' in the DB, if no timezone is specified it uses the current local one.
So it takes the times from submissions, thinking they are in local time zone and
not UTC. OTOH, internally generated events are converted correctly exactly
because of the local timezone and consecutive conversion.

I have not verified what types are used for other backends but I believe they
are similar to the mysql example.


> Might be relevant.
> 
>   https://gist.github.com/aaugustin/2971450
> 

That's something different. We need to unify the timezones used. Making PW
timezone-aware would not change anything, including the effect on old data
(exactly because the dates used are not unified, and we have no way to find
out the offsets the data were originally created with anyways).


Hope this explains the situation, just ask if I wasn't clear

Veronika


> Stephen
> 
> > Stephen
> > 
> > [1] https://docs.djangoproject.com/en/2.0/topics/i18n/timezones/
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Implement list filtering

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: "Daniel Axtens" , "Don Zickus" 
> Cc: patchwork@lists.ozlabs.org
> Sent: Wednesday, January 31, 2018 11:25:03 AM
> Subject: Re: [PATCH] Implement list filtering
> 
> On Wed, 2018-01-31 at 13:25 +1100, Daniel Axtens wrote:
> > Don Zickus  writes:
> > 
> > > On Mon, Jan 29, 2018 at 11:36:36PM +1100, Daniel Axtens wrote:
> > > > Hi Don,
> > > > 
> > > > > > I suppose to put a finer point on it - what is your usecase here?
> > > > > > What
> > > > > > are you trying to achieve, and can we help you do that in a way
> > > > > > that
> > > > > > requires smaller changes to Patchwork, and is less fragile? (For
> > > > > > example
> > > > > > this is going to break if someone mis-spells the keyword you're
> > > > > > looking
> > > > > > for in the subject_match.)
> > > > > 
> > > > > So here is our use case.  Internally at Red Hat we use one mailing
> > > > > list to
> > > > > post all of our kernel patches.  However, being a distro company,
> > > > > patches
> > > > > can be applied to either RHEL-6,7,X.  For the last 15 years we have
> > > > > always
> > > > > used the method:
> > > > > 
> > > > > [RHEL-7 PATCH] instead of [PATCH].
> > > > 
> > > > Ah yes. I work for Canonical (although I do Patchwork in a private
> > > > capacity only) and our kernel team does something very similar.
> > > > 
> > > > > The project inside the []s has been what we filter through our regex.
> > > > > Is it
> > > > > prone to human typos?  Yes.  Most folks have stuck this in the
> > > > > .git/config
> > > > > subjectprefix option.  That limited the typos. It isn't perfect.
> > > > > 
> > > > > We have been using a hacked up PatchWork1 for the last 7 or so years.
> > > > > This
> > > > > is one of those features we need (or something to solve our problem)
> > > > > if we
> > > > > want to migrate to a PatchWork2 instance.
> > > > > 
> > > > > I hope that adds a little bit of context on our thinking.  Thoughts?
> > > > 
> > > > That's a compelling use-case and I'm happy to look at supporting it. I
> > > > will review the patch more closely in the coming days.
> > > 
> > > Thanks for your understanding and support!
> > > 
> > > Again, we know the solution has its 'human' limitations. :-) We just
> > > never
> > > came up with a better idea. So any ideas there are welcomed too! :-)
> > 
> > [I will eventually get around to reviewing the patch proper, this is just
> > spitballing.]
> > 
> > One option that came to me last night would be to do this filtering
> > before the emails are injested into patchwork. To elaborate:
> > 
> > I assume you're injesting mail using something like the setup described
> > at
> > http://patchwork.readthedocs.io/en/master/deployment/installation/#mail-transfer-agent-mta
> > that is:
> > 
> > $ sudo cat << EOF > /etc/aliases
> > patchwork: "|/opt/patchwork/patchwork/bin/parsemail.sh"
> > EOF
> > 
> > So what you could do is pass the email to something like procmail first,
> > let that filter the messages on your regexes, and then pass the filtered
> > mail to parsemail.sh, using an explicit list-id parameter to deliver it
> > to the right project.
> 

We are using the getmail method and previously did something similar
but weren't very happy with that solution since it was harder to
manage and required direct access to the machine.

> What would be the advantage of this though? Far as I can tell, there is
> a clear pattern here for mailing lists that are shared by multiple
> projects, namely, the use of a specific subject tag. I've seen this
> pattern in use on other development mailing lists (openstack-dev jumps
> to mind).
> 
> > This doesn't involve patchwork at all, which is both a strength (it's
> > simple for me) and a weakness (it's complex for you and involves
> > spreading config across 2 places).
> 
> To be honest, given how simple and generic this patch is, I'd prefer to
> keep the logic within Patchwork. It would require much less work on an
> administrators end over all (seeing as they'll have to configure
> procmail too), and be far more transparent to end users.
> 

I agree. Our goal is to have Patchwork as a service (container or VM)
managed by DevOps or similar team. This solution would make it uselessly
hard to for example add and edit projects -- instead of having our
Patchwork admin user click a few times, we would need to contact those
people and explain to them how to change the configuration with every
change we need. And as Stephen said, it's really not a straightforward
and transparent solution.

Veronika

> Thoughts?
> 
> Stephen
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Implement list filtering

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, February 5, 2018 4:24:21 PM
> Subject: Re: [PATCH] Implement list filtering
> 
> Hi Veronika,
> 
> So, having been convinced of the merits of this approach, here's my code
> review.
> 

Hi, thanks for the comments!

> > Sometimes, multiple projects reside at the same mailing list. So far,
> > Patchwork only allowed a single project per mailing list, which made it
> > impossible for these projects to use Patchwork (unless they did some
> > dirty hacks).
> >
> > Add a new property `subject_match` to projects and implement filtering
> > on (list_id, subject_match) match instead of solely list_id. Instance
> > admin can specify a regex on a per-project basis when the project is
> > created.
> 
> Ok, so let me see if I have the logic of this patch right.
> 
> A message comes in.
> 
> A set of possible projects is found based on the list-id.
> 
> Then, starting from the first one returned by the db (with no intrinsic
> ordering), the subject_match field is examined:
>  - if subject_match is empty, this project is returned as a match
>  - if subject_match is non-empty, the project is returned as a match if
>the subject matches
>  
> If this understanding is incorrect, bail out here :)
> 
> Consider a list like netdev. It has at least a couple of userspace
> projects (iproute2, ethtool) that could conceivably be spun off into
> their own lists, and which could use subject matches, but most patches
> are for the linux kernel and do not have usable subject matches.
> 
> As I understand it, you would need to have 3 projects:
>  - for iproute2, subject-match \[[^\]]*iproute[^\]]*\]
>  - for ethtool, subject-match \[[^\]]*ethtool[^\]]*\]
> Then how would you match the rest of the list? You could have an empty
> subject-match, but then if that is loaded first out of the database, it
> would match first and patches would never make it to the iproute2 or
> ethtool projects.
> 

My original plan was that if you need both matching projects and a default
one on top of that you can just do a negative lookahead.

> I think you need some sort of default or fall-back or last-resort - the
> name is not important: just that it will match *only* if no other
> project matches.
> 
> Does that sound right?
> 

Using a default project is doable and makes sense. Would it be acceptable
if we first checked for subject matches and in case no match was found,
checked for a project with same list_id and empty subject_match or do you
have something different in mind?

> I have some other minor comments which I have included throughout:
> 
> > diff --git a/docs/api.yaml b/docs/api.yaml
> > index 3e79f0b..3373226 100644
> > --- a/docs/api.yaml
> > +++ b/docs/api.yaml
> > @@ -374,6 +374,9 @@ definitions:
> >list_id:
> >  type: string
> >  description: Mailing list identifier for project.
> > +  subject_match:
> > +type: string
> > +description: Regex used for email filtering.
> This is good.
> 
> > diff --git a/docs/deployment/management.rst
> > b/docs/deployment/management.rst
> > index c50b7b6..870d7ee 100644
> > --- a/docs/deployment/management.rst
> > +++ b/docs/deployment/management.rst
> > @@ -63,7 +63,7 @@ due to, for example, an outage.
> >  .. option:: --list-id 
> >  
> > mailing list ID. If not supplied, this will be extracted from the mail
> > -   headers.
> > +   headers. Don't use this option if you require filtering based on
> > subjects.
> >  
> >  .. option:: infile
> 
> I don't understand why this would interfere with subject
> filtering. I notice below that you've added a new method that matches by
> list-id and subject and kept the old list-id matching one - does the
> command-line option use the old method?
> 
> I think I would prefer that there only be one system of mapping mails to
> projects and for this restriction to go away, but if there's a good
> reason for it to stay I am always open to persuasion.
> 

Yes, the command line option uses the old function. The rationale is that
if there are more projects with same list_id which differ in subject match
fields, how do you know which one to choose if you check for valid project
based only on list_id?

If you check the parse_mail() function, it chooses either the
find_project_by_id() or find_project_by_header() functions based on whether
list_id option was provided, so I kept this behavior and only changed the
helper function for find_project_by_header().

It's not hard to change find_project_by_id() to respect subject matches
too if you want. I just felt that it defeats the purpose of having the
option to skip header matching at altogether.

> > diff --git a/patchwork/api/project.py b/patchwork/api/project.py
> > index 446c473..597f605 100644
> > --- a/patchwork/api/project.py
> > +++ b/patchwork/api/project.py
> > @@ -39,8 +39,9 @@ class 

Re: [PATCH v2] Avoid timezone confusion

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: "Veronika Kabatova" <vkaba...@redhat.com>, "Stephen Finucane" 
> <step...@that.guru>
> Cc: patchwork@lists.ozlabs.org
> Sent: Tuesday, February 13, 2018 12:36:37 PM
> Subject: Re: [PATCH v2] Avoid timezone confusion
> 
> Veronika Kabatova <vkaba...@redhat.com> writes:
> 
> > - Original Message -
> >> From: "Stephen Finucane" <step...@that.guru>
> >> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> >> Sent: Wednesday, January 17, 2018 8:22:27 PM
> >> Subject: Re: [PATCH v2] Avoid timezone confusion
> >> 
> >> On Wed, 2018-01-17 at 19:18 +, Stephen Finucane wrote:
> >> > On Tue, 2018-01-16 at 18:58 +0100, vkaba...@redhat.com wrote:
> >> > > From: Veronika Kabatova <vkaba...@redhat.com>
> >> > > 
> >> > > Patchwork saves patches, comments etc with UTC timezone and reports
> >> > > this time when opening the patch details. However, internally
> >> > > generated
> >> > > processes such as events are reported with the instance's local
> >> > > time.
> >> > > There's nothing wrong with that and making PW timezone-aware would
> >> > > add
> >> > > useless complexity, but in a world-wide collaboration a lot of
> >> > > confusion
> >> > > may arise as the timezone is not reported at all. Instance's local
> >> > > time
> >> > > might be very different from the local time of CI integrating with
> >> > > PW,
> >> > > which is different from the local time of person dealing with it
> >> > > etc.
> >> > > 
> >> > > Use UTC everywhere by default instead of UTC for sumbissions and
> >> > > local
> >> > > timezone for internally generated events (which is not reported).
> >> > > 
> >> > > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> >> > 
> >> > What effect does this have on existing information in the database?
> >> > Does this mean they'll all remain UTC+12 or are they stored in UTC
> >> > format already? The Django docs [1] would lead me to suggest the
> >> > former, given that we don't have USE_TZ set to True.
> >> > 
> >> > I guess you can test that by setting up a deployment, creating
> >> > information, then switching this option over. I'd do this myself but
> >> > I'm not going to have time this week :(
> >> 
> >
> > The effect of the change for the existing data is the same as if the admin
> > decided
> > to override the default TIME_ZONE setting. In the end that's what this
> > change does,
> > move the instance time to UTC.
> >
> > If the TIME_ZONE setting is overriden (production.py) nothing changes, we
> > are only
> > changing the default.
> >
> >
> > To elaborate more, as we use timezone-naive format, the details depend on
> > database
> > backend and what underlying type from it Django uses to store datetimes.
> >
> > For mysql, they are saved as 'datetime' and this type doesn't know
> > timezones and
> > returns what you feed it. Django doesn't modify the times either, generated
> > time
> > is simply passed as a string. If you change timezone, 'datetime' is still
> > the
> > same. So submissions will have the right time, but internally triggered
> > changes
> > keep the original localized time and we can't really modify them since we
> > can't
> > retrieve the original timezone used.
> >
> > For postgres it's the exact opposite. Datetimes are stored as 'timestamp
> > with
> > time zone' in the DB, if no timezone is specified it uses the current local
> > one.
> > So it takes the times from submissions, thinking they are in local time
> > zone and
> > not UTC. OTOH, internally generated events are converted correctly exactly
> > because of the local timezone and consecutive conversion.
> >
> > I have not verified what types are used for other backends but I believe
> > they
> > are similar to the mysql example.
> >
> >
> >> Might be relevant.
> >> 
> >>   https://gist.github.com/aaugustin/2971450
> >> 
> >
> > That's something different. We need to unify the timezones used. Making PW
> > timezone-aware would not change anything, including the effect on old data
> >

Re: [PATCH] Fix CRLF newlines upon submission changes

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Tuesday, February 6, 2018 11:49:15 PM
> Subject: Re: [PATCH] Fix CRLF newlines upon submission changes
> 
> Hi Veronika,
> 
> > After changing submission via admin interface, CRLF newlines are
> > suddenly present in the body. Replace them back to '\n'.
> >
> > The issue was found after modifying submission via admin interface
> > using Python 2 and downloading the respective mbox file (git choked on
> > downloaded patch because of malformed line endings). Python 3's mail
> > module uses '\n' internally so the problem doesn't manifest there, but
> > the content received by Django/JS is still saved in the database with
> > CRLF line endings which shouldn't be there.
> 
> Huh, weird. I can't say modifying a sumbission through the admin
> interface is recommended behaviour, but still worth fixing.
> 
> Please could you add a comment or docstring to the save function
> that explains why this is necessary - an abbreviated version of your
> commit message would be fine.
> 

Hi,

I'll add it and resend v2 later today.

> Would you like this queued up for the 2.0.2 stable release?
> 

Sure, the more issues fixed for stable releases, the better.


Veronika

> Regards,
> Daniel
> 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > ---
> >  patchwork/models.py | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 3bf7c72..411af63 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -328,6 +328,10 @@ class EmailMixin(models.Model):
> >  return ''.join([match.group(0) + '\n' for match in
> >  self.response_re.finditer(self.content)])
> >  
> > +def save(self, *args, **kwargs):
> > +self.content = self.content.replace('\r\n', '\n')
> > +super(EmailMixin, self).save(*args, **kwargs)
> > +
> >  class Meta:
> >  abstract = True
> >  
> > --
> > 2.13.6
> >
> > ___
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH v3] Avoid timezone confusion

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Friday, February 23, 2018 3:44:25 PM
> Subject: Re: [PATCH v3] Avoid timezone confusion
> 
> Hi Veronika,
> 
> I have become briefly sidetracked by Docker's default behaviour to set
> the timezone in the container to UTC. However, I did have 1 question:
> 
> > diff --git a/patchwork/migrations/0024_timezone_unify.py
> > b/patchwork/migrations/0024_timezone_unify.py
> > new file mode 100644
> > index 000..99f0642
> > --- /dev/null
> > +++ b/patchwork/migrations/0024_timezone_unify.py
> > @@ -0,0 +1,46 @@
> > +# -*- coding: utf-8 -*-
> > +# Generated by Django 1.10.8 on 2018-02-22 23:11
> > +from __future__ import unicode_literals
> > +
> > +import datetime
> > +from django.db import migrations, models
> > +
> > +
> > +class Migration(migrations.Migration):
> > +
> > +dependencies = [
> > +('patchwork', '0023_submissiontag'),
> > +]
> This migration is numbered 24, and depends on 23. I only see migrations
> up to 21 in master. I assume you've accidentally based this on some
> internal patches. I fixed this up so you don't need to respin, but I
> just wanted to check that there wasn't anything in the missing
> migrations that you needed for this patch.
> 

Hi, good catch! I've been working on some other features as well, namely
we have a migration for the list splitting feature that I sent the v2 a
week ago, and then I'm working on issues #57 and #113 so my testing
instance has these deployed and makemigrations script that generated the
migration took it into account.

So no, there shouldn't be any dependency, we'd just need to rename list
splitting migration no. 22 that I already sent.


Veronika

> Regards,
> Daniel
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: Subject match filtering - followup patch request

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Thursday, March 8, 2018 12:38:22 PM
> Subject: Subject match filtering - followup patch request
> 
> Hi Veronika,
> 

Hi,

> I tried to use the subject match filter and clearly got the RE wrong, as I
> get the following messages when trying to parse mail.
> 
> Traceback (most recent call last):
>   File "manage.py", line 11, in 
> execute_from_command_line(sys.argv)
>   File
>   "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py",
>   line 364, in execute_from_command_line
> utility.execute()
>   File
>   "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py",
>   line 356, in execute
> self.fetch_command(subcommand).run_from_argv(self.argv)
>   File
>   "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py",
>   line 283, in run_from_argv
> self.execute(*args, **cmd_options)
>   File
>   "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py",
>   line 330, in execute
> output = self.handle(*args, **options)
>   File
>   "/home/patchwork/patchwork/patchwork/management/commands/parsearchive.py",
>   line 100, in handle
> obj = parse_mail(msg, options['list_id'])
>   File "/home/patchwork/patchwork/patchwork/parser.py", line 959, in
>   parse_mail
> project = find_project(mail, list_id)
>   File "/home/patchwork/patchwork/patchwork/parser.py", line 205, in
>   find_project
> project = find_project_by_id_and_subject(listid, clean_subject)
>   File "/home/patchwork/patchwork/patchwork/parser.py", line 173, in
>   find_project_by_id_and_subject
> re.MULTILINE | re.IGNORECASE):
>   File "/usr/lib/python3.6/re.py", line 182, in search
> return _compile(pattern, flags).search(string)
>   File "/usr/lib/python3.6/re.py", line 301, in _compile
> p = sre_compile.compile(pattern, flags)
>   File "/usr/lib/python3.6/sre_compile.py", line 562, in compile
> p = sre_parse.parse(p, flags)
>   File "/usr/lib/python3.6/sre_parse.py", line 855, in parse
> p = _parse_sub(source, pattern, flags & SRE_FLAG_VERBOSE, 0)
>   File "/usr/lib/python3.6/sre_parse.py", line 416, in _parse_sub
> not nested and not items))
>   File "/usr/lib/python3.6/sre_parse.py", line 523, in _parse
> source.tell() - here)
> sre_constants.error: unterminated character set at position 2
> 
> Please could you add a hook somehwere (perhaps the .save() of the
> Project model, but wherever is easy is fine) that checks if the RE
> compiles before committing it to the database?
> 

Sounds reasonable. I added it to my TODO list and will take a look
at it as I have time, hopefully sometime next week.

> No rush.
> 
> Thanks in advance.
> 
> Regards,
> Daniel
> 

Veronika
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH v3] Avoid timezone confusion

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, March 7, 2018 3:39:01 PM
> Subject: Re: [PATCH v3] Avoid timezone confusion
> 
> Hi Veronika,
> 
> Thank you for your patience.
> 
> > Patchwork saves patches, comments etc with UTC timezone and reports
> > this time when opening the patch details. However, internally generated
> > processes such as events are reported with the instance's local time.
> > There's nothing wrong with that and making PW timezone-aware would add
> > useless complexity, but in a world-wide collaboration a lot of confusion
> > may arise as the timezone is not reported at all. Instance's local time
> > might be very different from the local time of CI integrating with PW,
> > which is different from the local time of person dealing with it etc.
> >
> > Use UTC everywhere by default instead of UTC for sumbissions and local
> > timezone for internally generated events (which is not reported).
> 
> I found that Docker set up containers in the UTC timezone, which made
> things difficult to test. I patched the dockerfile to put the container
> in UTC+11, which should match the TZ value that Django uses.
> 
> Without your patch, events were created with local time, stored in - as
> far as I can tell - timezone-unaware format in the database.
> 
> I then applied your patch.
> 
> With the patch, the events were created with UTC time, again stored
> AFAICT TZ-unaware.
> 
> That all checks out - I was a bit worried Django was going to do
> something 'clever' and try to store in UTC and convert back and forth,
> and our conversion would lead to some sort of awful double-convert. But
> everything is in order so we can proceed :)
> 

Glad it works for you! Having unified timezones saved us a lot of time
when debugging things internally so we wanted to help out.

> So I:
>  - made the fixup to the migration number and dependency, as discussed.
>  - made a minor edit to the doc fixup (s/sooner/earlier)
>  - squashed your two patches
>  - added
> Tested-by: Daniel Axtens <d...@axtens.net>
>  - pushed to master at 8465e33c23310e4873d464fe2581842df2e9c6f8
> 

Awesome, thanks! I think you missed Stephen's Reviewed-by [1] in the
commit. It doesn't matter too much to me, just wanted to bring it up
in case he'd like it there :)


[1] https://patchwork.ozlabs.org/patch/876744/#1864659

Thanks again,

Veronika


> Thanks again for your contributions to patchwork!
> 
> Regards,
> Daniel
> 
> >
> >
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > ---
> > Changes: Implement Daniel's idea about using datetime.datetime.utcnow
> > ---
> >  docs/api/rest.rst  |  3 +-
> >  patchwork/migrations/0024_timezone_unify.py| 46
> >  ++
> >  patchwork/models.py| 12 +++---
> >  patchwork/notifications.py |  4 +-
> >  patchwork/signals.py   |  2 +-
> >  patchwork/templates/patchwork/submission.html  |  4 +-
> >  patchwork/tests/test_checks.py | 10 ++---
> >  patchwork/tests/test_expiry.py |  8 ++--
> >  patchwork/tests/test_notifications.py  |  2 +-
> >  patchwork/tests/utils.py   |  6 +--
> >  .../notes/unify-timezones-0f7022f0c2a371be.yaml|  5 +++
> >  11 files changed, 77 insertions(+), 25 deletions(-)
> >  create mode 100644 patchwork/migrations/0024_timezone_unify.py
> >  create mode 100644
> >  releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> >
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index 3d7292e..d526b27 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -107,7 +107,8 @@ Schema
> >  --
> >  
> >  Responses are returned as JSON. Blank fields are returned as ``null``,
> >  rather
> > -than being omitted. Timestamps use the ISO 8601 format::
> > +than being omitted. Timestamps use the ISO 8601 format, times are by
> > default
> > +in UTC::
> >  
> >  -MM-DDTHH:MM:SSZ
> >  
> > diff --git a/patchwork/migrations/0024_timezone_unify.py
> > b/patchwork/migrations/0024_timezone_unify.py
> > new file mode 100644
> > index 000..99f0642
> > --- /dev/null
> > +++ b/patchwork/migrations/0024_timezone_unify.py
> > @@ -0,0 +1,46 @@
> > +# -*- coding: utf-8 -*-
> > +# Generated by Django 1.10.8 on 2018-02-22 23:11
> > +from __future__ 

Re: [PATCH 0/2] Patchwork 2.1.0-rc1: Extra Special OzLabs Edition

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: patchwork@lists.ozlabs.org
> Sent: Thursday, April 5, 2018 7:02:59 PM
> Subject: [PATCH 0/2] Patchwork 2.1.0-rc1: Extra Special OzLabs Edition
> 
> TL;DR: I want to spin a new version of patchwork for OzLabs so they can
> do the migration that speeds up patch listing. I've pushed it to
> https://github.com/daxtens/patchwork as I'm not sufficiently confident
> in my release-fu to push it to the main repo just yet. Let me know if
> it looks good.
> 
> From the release notes:
> ==
> 
> Patchwork 2.1.0 - the Extra Special OzLabs Edition
> 
> The key part of this release is a major performance fix -
> denormalising the project field into patch model so that counting a
> project's patches doesn't require a JOIN. This requires a migration
> and so isn't suitable for a stable backport. Event listing in the API
> has also been sped up by refactoring the queries.
> 
> This release also includes the feature development that had accured in
> the mean time, as laid out below*, and numerous bug fixes.
> 
> * not in this email, see the patches.
> 
> 
> Some other thoughts/notes:
> 
>  * I've pushed this to https://github.com/daxtens/patchwork as I'm not
>sufficiently confident in my release-fu to push it to the main repo
>just yet. This includes a tag, v2.1.0-rc1. Stephen, absent any
>rants from you I will push it to the real repo mid-next week
> 
>  * I know the patch counting isn't a full fix of everything that could
>or should be denormalised, but it's an ongoing problem and I don't
>want another 6 months of maintainer-guilt about the ongoing impact
>it's having.
> 
>  * Normally releases are named after a fabric, but it's hard to find
>good fabrics beginning with E, at least according to the wikipedia
>list. I am very open to renaming/rewording stuff, hence the emails
>with the patches!
> 

What about Eolienne [1] ? Another possibility might be elastane, but
that IMHO doesn't sound very fancy.

[1] https://en.wikipedia.org/wiki/Eolienne


Veronika

>  * There's a decent chance I've missed a version number somewhere -
>please check your favourite locations!
> 
>  * What else do we want in 2.1? I think at least the return code fix -
>it needs a v3 from me. (I would have done that first but I really
>wanted to get this out tonight/this morning for general awareness
>and comment.) I'm happy to take any more fixes or small things, but
>probably not big things. Feel free to post them and we can review
>them and get them ready to merge as soon as we tag 2.1.0 though!
> 
>  * Testing and general feedback would be appreciated. :)
> 
> Thanks all for your contributions to Patchwork!
> 
> Regards,
> Daniel
> 
> Daniel Axtens (2):
>   docs: Prepare for 2.1.0-rc1
>   Patchwork v2.1.0-rc1
> 
>  docs/conf.py   |  4 ++--
>  docs/index.rst |  1 +
>  docs/releases/extra-special-ozlabs-edition.rst | 33
>  ++
>  docs/releases/index.rst|  1 +
>  patchwork/__init__.py  |  2 +-
>  5 files changed, 38 insertions(+), 3 deletions(-)
>  create mode 100644 docs/releases/extra-special-ozlabs-edition.rst
> 
> --
> 2.14.1
> 
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Use parsed subject for mboxes

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: "Veronika Kabatova" <vkaba...@redhat.com>, "Daniel Axtens" 
> <d...@axtens.net>
> Cc: patchwork@lists.ozlabs.org
> Sent: Monday, April 9, 2018 4:09:29 PM
> Subject: Re: [PATCH] Use parsed subject for mboxes
> 
> On Mon, 2018-04-09 at 09:00 -0400, Veronika Kabatova wrote:
> > - Original Message -
> > > From: "Daniel Axtens" <d...@axtens.net>
> > > To: "Stephen Finucane" <step...@that.guru>, patchwork@lists.ozlabs.org
> > > Sent: Sunday, April 8, 2018 5:21:48 AM
> > > Subject: Re: [PATCH] Use parsed subject for mboxes
> > > 
> > > Stephen Finucane <step...@that.guru> writes:
> > > 
> > > > With a recent change, we started using the original subject header
> > > > instead of the one we had already cleaned up at the parsing stage.
> > > 
> > > Does anyone care about the cleaned up header in the mbox? Git strips it
> > > all off anyway... Is there any docs on why we did things that way
> > > originally?
> > > 
> > 
> > I agree with Daniel and Johannes. For the mboxes, we should use the
> > original
> > subject for consistency with the original patch sent. Scripts might get
> > confused by the difference, and there might be ones requiring "PATCH" in
> > the
> > subject etc. Unless there is an important reason to use parsed subject in
> > the
> > mbox, I'd rather revert this and fix the tests. Especially as it's not even
> > the mbox tests that fail (which I've run) but the bundles tests using the
> > patch's name to assert the patch was added to the bundle (using
> > X-Patchwork-Id for those tests would be more appropriate choice).
> 
> Yeah, that's fine with me. It doesn't look like git-am or the likes
> care and, as Daniel noted earlier in the thread, no one really reads
> mboxes by themselves. TBh, I just wanted to fix the tests and this did
> look initially like a regression. If someone's happy to pick this up, I
> can review/apply.
> 

Fixing this turned out to be a bit trickier than I initially thought but
I'll send a patch later today.

> Stephen
> 
> PS: Might want to add a release note about this change in behavior
> though.
> 

Will add.


Thanks,
Veronika
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Add comments REST API

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: "Stephen Finucane" <step...@that.guru>, "Veronika Kabatova" 
> <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Tuesday, April 10, 2018 4:56:59 PM
> Subject: Re: [PATCH] Add comments REST API
> 
> Stephen Finucane <step...@that.guru> writes:
> 
> > On Mon, 2018-04-09 at 09:29 -0400, Veronika Kabatova wrote:
> >> - Original Message -
> >> > From: "Stephen Finucane" <step...@that.guru>
> >> > To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> >> > Sent: Monday, April 9, 2018 3:02:57 PM
> >> > Subject: Re: [PATCH] Add comments REST API
> >> > 
> >> > On Fri, 2018-03-23 at 13:33 +0100, vkaba...@redhat.com wrote:
> >> > > From: Veronika Kabatova <vkaba...@redhat.com>
> >> > > 
> >> > > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> >> 
> >> Thanks for feedback!
> >> 
> >> > Looks pretty good. As you note, it does need some tests. I'm also
> >> > curious about the idea of nesting this under the patches endpoint. Let
> >> > me know what you think.
> >> > 
> >> 
> >> Hmm, that would require the same change for cover letters too. How do
> >> you imagine it? Having all related comments under the detailed patch or
> >> cover, or something else?
> >
> > Indeed. I'd expect all replies to a given patch to be under that patch
> > and so on. This is what we present in the web UI so it probably makes
> > sense to do the same for the REST API. Unless there's a strong reason
> > _not_ to, this is the way I'd go, yeah.
> >
> >> I opted for separate API because some people may not care about comments
> >> (since AFAIK noone requested it yet), but we very much do. At the same
> >> time I didn't want to push our needs on others. If you don't think it's
> >> an issue I can rework this and put it under the patch/cover endpoints.
> >
> 
> We do have 1 pending request re: the comments API:
> https://github.com/getpatchwork/patchwork/issues/143
> 
> I don't know how that would interact with either design, but I wanted to
> flag it as a user story to consider.
> 

Thanks for posting that! I checked the issue list briefly but the subject
didn't seem relevant so I missed it. Taking this feature into consideration:

(previous solution) keeping the API as /comments, the tools can filter
  through it and then send requests for patch updates
(currently working on) putting the comments into patches / covers
  endpoints as a serialized list, tools can query the patch and check the
  comments there directly, then send the request to the same /patch/ID
  endpoint used previously

It would be the best if those changes were triggered automatically with
comment parsing but until we have a safe way of doing that and a proof of
concept, having a tool run periodically and marking patches should work
fine with both implementations.

I don't think the hidden endpoint Stephen mentioned below will play well
with this request or add anything useful - we have a hard time getting
checks associated with given patch since the link from patch goes to the
list of all checks which can't be filtered by patch ID (instead of listing
checks associated with the patch directly). Whichever way we go with the
comments API implementation, the hidden endpoint will be more confusing
than useful, IMHO.


I'll go ahead and implement the changes discussed previously if you guys
don't have additional ideas.

Thanks,
Veronika


> Regards,
> Daniel
> 
> > Yup, I didn't do it because I wasn't sure how helpful it was. I'm a-ok
> > adding it though so go for it.
> >
> > Stephen
> >
> > PS: You should probably put a 'comments' url in the 'patches' and
> > 'covers' resources, similar to what we do for 'checks' of 'patches'.
> > ___
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Include all email headers in mboxes

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Thursday, April 5, 2018 11:58:15 AM
> Subject: Re: [PATCH] Include all email headers in mboxes
> 
> vkaba...@redhat.com writes:
> 
> > From: Veronika Kabatova <vkaba...@redhat.com>
> >
> > Solves issue #165 (Exported mboxes should include In-Reply-To,
> > References, etc headers). Instead of including only a few chosen ones,
> > all received headers are added to mboxes.
> 
> Thanks for the patch.
> 
> I'm a little worried that this will get really messy - I've included a
> snippet of headers from an email from an unrelated bug below. Maybe we
> don't care - I guess this isn't really for human consumption but is for
> consumption by e.g. git-am.
> 
> Alternatively we can blacklist headers: I don't think there's anything
> worth having in Received, X-*, List-*, DKIM, ARC, SPF, etc. But I wonder
> if this is just whack-a-mole in reverse.
> 
> Thoughts?
> 

Hi,

I've thought about ignoring eg headers starting with X-* but ultimately
decided I don't want to exclude them. Maybe people use custom headers for
patch apply hooks or similar craziness, or have CI / tools that download
the mbox and require a specific header set (for example reply to the author
and mailing list if the patch is broken unless X-ignore header is set) etc.
Whatever subset we decide to include, there might be someone who needs one
of the headers that wasn't added. Granted, you can extract the headers from
the API itself but we can say the same about extracting the patch from
there since git am eats input from stdin too.

From my experience, blacklisting never works. Things evolve and new "messy"
headers will be added, or people will just use a different nonstandard
header name if they need custom headers and so on.

As you said, I doubt anyone actually needs to read the mbox files.
Everything included there is already visible in WebUI or API. Also,
skipping the header section / searching the text should work if somebody
really needs to consume raw mboxes without tools.


Veronika

> Regards,
> Daniel
> 
> 
> Delivered-To: d...@axtens.net
> Received: by 10.74.139.216 with SMTP id p24csp390579ooj;
> Wed, 21 Feb 2018 01:07:26 -0800 (PST)
> X-Google-Smtp-Source:
> AH8x227WmScjPDkd1bgpNsd0DYg4ae/OzHPrTT68ezy+A7p6CMpHdsmU6pB6dZluayRkf9LDxR5l
> X-Received: by 10.99.55.1 with SMTP id e1mr2155932pga.237.1519204046116;
> Wed, 21 Feb 2018 01:07:26 -0800 (PST)
> ARC-Seal: i=1; a=rsa-sha256; t=1519204046; cv=none;
> d=google.com; s=arc-20160816;
> b=PXNfl2kupZqmeFoDWy2tLz6100KxBfLj/Rmu4/OTqiLwcPVNIsGBGE2FN/e/Gj2LCu
>  NDdGSTuaDdYxI0wlCEqBZBrVatoda/PHBX2Fh2HZ2rzEahAs5R2w+YucxRTAi8UT7MTo
>  xxxHqQ1DehoZmG9J3l0Ckh1cXmAplnmLB7+MufnD7FT7ajNufPhGiZ2ovDCcqb0/6KxG
>  8cfsjAnNAZAF3M8VF8r2KSubGuplPq4sB7aF9eklL0HmnXNVLYkyiwZbJuI/vVh6yBkc
>  rKSdP02SlazOIL8FJuJQC5reac17qfbyjg/tbxyCC+/Lmc/DwexMfA4NPhEgqPZgwFWr
>  9ILQ==
> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
> s=arc-20160816;
> h=list-id:precedence:sender:content-transfer-encoding:mime-version
>  :message-id:date:subject:to:from:dkim-signature
>  :arc-authentication-results;
> bh=CFYy3bhn7eoWPF6AJMH1sMf+K6u6usD+1M2NOg/UHAw=;
> b=VtdyXugEOzZm0I0t+YzlUoOWX+xI4UC50ZF9w5gd+zlNTsJMDGtykiwjZujU4FzMiP
>  CdCwQqdlTuB5aY99HtOyBl1jjGz2zo1bGxRwr1aGFE+pbyUN5FFlTntYxZ0AD5wbDP3S
>  2hn6mDxzugFSJ9+LbDaa8lS4g6RENzaxHTPt7B6paAkbSFSZsBtY9oASno7RvI3ctlfz
>  LCb1gAnhvL5p2H5vpNeIOr9eTQIOqhh7+kmmq7tMky2/aB95az+AaU/dKJQoUTsIY7qT
>  gOTZAdHc4+fZHD2CeP9bX8MTUXy1Yc3fNy7ETbYqoe0+Gl2ke8m9nw84rHTmidoygKrH
>  CwoQ==
> ARC-Authentication-Results: i=1; mx.google.com;
>dkim=pass header.i=@gmail.com header.s=20161025 header.b=YTP6W3WD;
>spf=pass (google.com: best guess record for domain of
>linux-kernel-ow...@vger.kernel.org designates 209.132.180
> .67 as permitted sender) smtp.mailfrom=linux-kernel-ow...@vger.kernel.org;
>dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com
> Return-Path: <linux-kernel-ow...@vger.kernel.org>
> Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67])
> by mx.google.com with ESMTP id s9si2120330pgn.281.2018.02.21.01.07.26
> for <d...@axtens.net>;
> Wed, 21 Feb 2018 01:07:26 -0800 (PST)
> Received-SPF: pass (google.com: best guess record for domain of
> linux-kernel-ow...@vger.kernel.org designates 209.132.
> 180.67 as permitted sender) client-ip=209.132.180.67;
> Authentication-Results: mx.google.com;
>dkim=pass header.i=@gmail.com header.

Re: [PATCH] Include all email headers in mboxes

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: "Johannes Berg" <johan...@sipsolutions.net>, vkaba...@redhat.com, 
> patchwork@lists.ozlabs.org
> Sent: Thursday, April 5, 2018 3:47:03 PM
> Subject: Re: [PATCH] Include all email headers in mboxes
> 
> Johannes Berg <johan...@sipsolutions.net> writes:
> 
> > On Thu, 2018-04-05 at 19:58 +1000, Daniel Axtens wrote:
> >> vkaba...@redhat.com writes:
> >> 
> >> > From: Veronika Kabatova <vkaba...@redhat.com>
> >> > 
> >> > Solves issue #165 (Exported mboxes should include In-Reply-To,
> >> > References, etc headers). Instead of including only a few chosen ones,
> >> > all received headers are added to mboxes.
> >> 
> >> Thanks for the patch.
> >> 
> >> I'm a little worried that this will get really messy - I've included a
> >> snippet of headers from an email from an unrelated bug below. Maybe we
> >> don't care - I guess this isn't really for human consumption but is for
> >> consumption by e.g. git-am.
> >> 
> >> Alternatively we can blacklist headers: I don't think there's anything
> >> worth having in Received, X-*, List-*, DKIM, ARC, SPF, etc. But I wonder
> >> if this is just whack-a-mole in reverse.
> >> 
> >> Thoughts?
> >
> > I'm not really sure human consumption would be a worry for mbox files?
> > Having the full headers - since it's sort of an email archive already -
> > would be useful though.
> >
> > In particular, the change to keep the original Subject would be useful
> > for us, as we have a script that automatically replies to the email
> > saying "thank you, I've applied your patch" or similar.
> 
> Hmm, I think the hasher.py and patchwork-update-commits scripts were
> designed to facilitate this sort of thing in a slightly more robust
> way. But I've never really felt like I understood it, and I don't recall
> any docs.
> 
> I hope that the change to the subject mangling doesn't break anything
> else, but I can't imagine it would. [0]
> 
> > That said, using a dict for this is in general not quite right, since
> > many header lines are valid multiple times, e.g. "Received:", but I'm
> > not sure they're even all stored.
> 
> Right, you and Veronika have convinced me. Clearly as a developer of
> patchwork my view is a bit skewed - I have to look at them fairly
> frequently when people report parsing bugs so I forget that other people
> don't do this.
> 
> Veronika - can you check on repeated headers issue that Johannes raises?
> 

Right, this won't be too friendly in case of duplicate header keys. We do
store all headers with duplicate keys so I can easily change that and post
updated patch shortly.

However, since this was brought up, the same situation is present in
the API -- only one of the duplicate headers is shown. Based on my testing,
the code on our side does return correct values so this should be a problem
with the REST framework or something similar. I might take a look at it
later but can't promise I'll be able to fix it easily.


Thanks,
Veronika


> Thanks all.
> 
> Regards,
> Daniel
> 
> [0] Obligatory xkcd - https://xkcd.com/1172/
> >
> > johannes
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [RFC 0/2] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, April 11, 2018 6:43:25 PM
> Subject: Re: [RFC 0/2] Rework tagging infrastructure
> 
> On Fri, 2018-03-16 at 15:38 +0100, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> 
> I'm going to review this this week. However, this doesn't apply cleanly
> to head of master any more (sorry :(). Any chance you could send
> updated versions of these?
> 

Will rebase and resend shortly.

Veronika

> Stephen
> 
> > (TL;DR at the end)
> > 
> > This RFC describes an approach to rework tagging. It attempts to solve
> > GitHub
> > issues #57 [1] and #113 [2] as well as some other things we encountered.
> > I'm
> > sending the incomplete version (eg I haven't fixed the tests) to discuss
> > the
> > approach first.
> > 
> > Right now, tags are extracted from all the comments on the patch and the
> > patch
> > itself, and they are reextracted from all the sources every time a comment
> > is
> > added or removed. It makes saving slower, and might contribute to races
> > with
> > writes to database when we are parsing multiple emails at the same time.
> > This
> > gets even more prevalent if we want to solve the issue #113 (tags on cover
> > letter should increment counters on every patch in series) -- for each
> > added
> > comment on the cover letter, we would reextract tags from all the other
> > sources,
> > for each patch in series; and for a change on comments related to the patch
> > directly, we would need to take the tags on the cover letter and it's
> > comments
> > into account as well (I implemented this solution in my fork but I really
> > don't like it).
> > 
> > The current approach has several other issues as well, some of which are
> > mentioned in the issue #57, eg duplicate tags are counted more times.
> > Taking
> > into account the tags on cover letters would also be easier if we could
> > store
> > tags against them and just query them on demand, instead of reextracting
> > everything all over again.
> > 
> > Solutions for some other things we found missing solve the issues mentioned
> > above too. If we want to determine if the tag is duplicate, we need to save
> > the
> > associated value. Having the value would help us to use arbitrary strings
> > as
> > tags (for example links to issue trackers, like `Bugzilla: ` if the
> > patch
> > solves a known bug). The key-values approach to storing tags is mentioned
> > [3],
> > this email additionally mentions a currently nonexistent /comments REST API
> > which we would like to implement some time in the future. For the comments
> > API
> > we would also find it very useful to have the tags extracted from the
> > comments
> > available directly so we can query for them, which means we would either
> > need to
> > reextract the tags on every API call, or we could store the tags against
> > the
> > comments as they are extracted and only query them as needed. Keeping tags
> > related to comments where they originated from avoids the need to reextract
> > tags with addition of new comments, as well as gets rid of the
> > `patch_responses`
> > property used when converting comments to mbox (we finally get all the
> > custom
> > tags there instead of only the few hardcoded ones).
> > 
> > TL;DR:
> > Our goals:
> > - Avoid tag reextraction with each added comment
> > - Fix issues #57 and #113
> > - Prepare things for addition of /comments API
> > - Add tags to patch (currently returns {}) and cover letter APIs
> > 
> > If you agree with the general idea, I'd like to get some help with DB query
> > optimizations. Tags are more distributed, so counting them in PatchQuerySet
> > is
> > harder. I wanted to use annotation with Django's conditional selection, but
> > it
> > doesn't seem to work very well and generated invalid SQL in my attempts
> > (hence
> > my temporary solution with counting the tags on the fly only for the
> > patches
> > that are being displayed). That said, I'm not a DB expert so any faster
> > working
> > solutions are more than welcome.
> > 
> > 
> > Thoughts? Ideas?
> > 
> > 
> > [1] https://github.com/getpatchwork/patchwork/issues/57
> > [2] https://github.com/getpatchwork/patchwork/issues/113
> > [3] https://lists.ozlabs.org/pipermail/patchwork/2018-January/004741.html

Re: [PATCH] Use parsed subject for mboxes

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: "Stephen Finucane" , patchwork@lists.ozlabs.org
> Sent: Sunday, April 8, 2018 5:21:48 AM
> Subject: Re: [PATCH] Use parsed subject for mboxes
> 
> Stephen Finucane  writes:
> 
> > With a recent change, we started using the original subject header
> > instead of the one we had already cleaned up at the parsing stage.
> 
> Does anyone care about the cleaned up header in the mbox? Git strips it
> all off anyway... Is there any docs on why we did things that way
> originally?
> 

I agree with Daniel and Johannes. For the mboxes, we should use the original
subject for consistency with the original patch sent. Scripts might get
confused by the difference, and there might be ones requiring "PATCH" in the
subject etc. Unless there is an important reason to use parsed subject in the
mbox, I'd rather revert this and fix the tests. Especially as it's not even
the mbox tests that fail (which I've run) but the bundles tests using the
patch's name to assert the patch was added to the bundle (using
X-Patchwork-Id for those tests would be more appropriate choice).

Veronika

> Regards,
> Daniel
> 
> > Revert this aspect of that change.
> >
> > Signed-off-by: Stephen Finucane 
> > Fixes: 01b9cbb9 ("Include all email headers in mboxes")
> > ---
> >  patchwork/views/utils.py | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> > index f5ff43c1..7f89004b 100644
> > --- a/patchwork/views/utils.py
> > +++ b/patchwork/views/utils.py
> > @@ -101,6 +101,9 @@ def _submission_to_mbox(submission):
> >  for key, val in orig_headers.items():
> >  mail[key] = val
> >  
> > +# specifically overwrite the subject with our own nicely formatted
> > name
> > +mail['Subject'] = submission.name
> > +
> >  if 'Date' not in mail:
> >  mail['Date'] = email.utils.formatdate(utc_timestamp)
> >  
> > --
> > 2.14.3
> >
> > ___
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Add comments REST API

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, April 9, 2018 3:02:57 PM
> Subject: Re: [PATCH] Add comments REST API
> 
> On Fri, 2018-03-23 at 13:33 +0100, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> 

Thanks for feedback!

> Looks pretty good. As you note, it does need some tests. I'm also
> curious about the idea of nesting this under the patches endpoint. Let
> me know what you think.
> 

Hmm, that would require the same change for cover letters too. How do
you imagine it? Having all related comments under the detailed patch or
cover, or something else?

I opted for separate API because some people may not care about comments
(since AFAIK noone requested it yet), but we very much do. At the same
time I didn't want to push our needs on others. If you don't think it's
an issue I can rework this and put it under the patch/cover endpoints.

> Stephen
> 
> > ---
> >  docs/api/rest.rst  |   6 +-
> >  patchwork/api/comment.py   | 117
> >  +
> >  patchwork/api/filters.py   |  16 +++
> >  patchwork/api/index.py |   1 +
> >  patchwork/models.py|   3 +
> >  patchwork/urls.py  |  10 ++
> >  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |  10 ++
> >  7 files changed, 161 insertions(+), 2 deletions(-)
> >  create mode 100644 patchwork/api/comment.py
> >  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > 
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index d526b27..e33aefc 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -48,7 +48,8 @@ Patchwork instance hosted at `patchwork.example.com`,
> > run:
> >  "people": "https://patchwork.example.com/api/1.0/people/;,
> >  "projects": "https://patchwork.example.com/api/1.0/projects/;,
> >  "series": "https://patchwork.example.com/api/1.0/series/;,
> > -"users": "https://patchwork.example.com/api/1.0/users/;
> > +"users": "https://patchwork.example.com/api/1.0/users/;,
> > +"comments": "https://patchwork.example.com/api/1.0/comments/;
> >  }
> >  
> >  
> > @@ -71,7 +72,8 @@ well-supported. To repeat the above example using
> > `requests`:, run
> >  "people": "https://patchwork.example.com/api/1.0/people/;,
> >  "projects": "https://patchwork.example.com/api/1.0/projects/;,
> >  "series": "https://patchwork.example.com/api/1.0/series/;,
> > -"users": "https://patchwork.example.com/api/1.0/users/;
> > +"users": "https://patchwork.example.com/api/1.0/users/;,
> > +"comments": "https://patchwork.example.com/api/1.0/comments/;
> >  }
> >  
> >  Tools like `curl` and libraries like `requests` can be used to build
> >  anything
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 000..4252ecd
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,117 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > USA
> > +
> > +import email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.generics import Retrieve

Re: [PATCH] tests: Replace incorrect tests

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: patchwork@lists.ozlabs.org
> Cc: vkaba...@redhat.com, "Stephen Finucane" <step...@that.guru>
> Sent: Tuesday, April 17, 2018 10:41:10 AM
> Subject: [PATCH] tests: Replace incorrect tests
> 
> In commit 683792d1, the 'test_api.py' was split into multiple
> 'api/test_xyz.py' files. As part of this change, the tests for cover
> letter were mistakenly included in place of tests for checks. Correct
> this oversight.
> 
> Signed-off-by: Stephen Finucane <step...@that.guru>
> Reported-by: Veronika Kabatova <vkaba...@redhat.com>
> Fixes: 683792d1 ("tests: Split 'test_rest_api'")
> ---

Thanks for the fast reaction and fix!

Veronika

>  patchwork/tests/api/test_check.py | 140
>  ++
>  1 file changed, 81 insertions(+), 59 deletions(-)
> 
> diff --git a/patchwork/tests/api/test_check.py
> b/patchwork/tests/api/test_check.py
> index 6e3d68b8..7b8139ad 100644
> --- a/patchwork/tests/api/test_check.py
> +++ b/patchwork/tests/api/test_check.py
> @@ -38,84 +38,106 @@ else:
>  
>  
>  @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> -class TestCoverLetterAPI(APITestCase):
> +class TestCheckAPI(APITestCase):
>  fixtures = ['default_tags']
>  
> -@staticmethod
> -def api_url(item=None):
> +def api_url(self, item=None):
>  if item is None:
> -return reverse('api-cover-list')
> -return reverse('api-cover-detail', args=[item])
> -
> -def assertSerialized(self, cover_obj, cover_json):
> -self.assertEqual(cover_obj.id, cover_json['id'])
> -self.assertEqual(cover_obj.name, cover_json['name'])
> -self.assertIn(cover_obj.get_mbox_url(), cover_json['mbox'])
> -
> -# nested fields
> -
> -self.assertEqual(cover_obj.submitter.id,
> - cover_json['submitter']['id'])
> +return reverse('api-check-list', args=[self.patch.id])
> +return reverse('api-check-detail', kwargs={
> +'patch_id': self.patch.id, 'check_id': item.id})
> +
> +def setUp(self):
> +super(TestCheckAPI, self).setUp()
> +project = create_project()
> +self.user = create_maintainer(project)
> +self.patch = create_patch(project=project)
> +
> +def _create_check(self):
> +values = {
> +'patch': self.patch,
> +'user': self.user,
> +}
> +return create_check(**values)
> +
> +def assertSerialized(self, check_obj, check_json):
> +self.assertEqual(check_obj.id, check_json['id'])
> +self.assertEqual(check_obj.get_state_display(), check_json['state'])
> +self.assertEqual(check_obj.target_url, check_json['target_url'])
> +self.assertEqual(check_obj.context, check_json['context'])
> +self.assertEqual(check_obj.description, check_json['description'])
>  
>  def test_list(self):
> -"""Validate we can list cover letters."""
> +"""Validate we can list checks on a patch."""
>  resp = self.client.get(self.api_url())
>  self.assertEqual(status.HTTP_200_OK, resp.status_code)
>  self.assertEqual(0, len(resp.data))
>  
> -person_obj = create_person(email='t...@example.com')
> -project_obj = create_project(linkname='myproject')
> -cover_obj = create_cover(project=project_obj, submitter=person_obj)
> +check_obj = self._create_check()
>  
> -# anonymous user
> -resp = self.client.get(self.api_url())
> -self.assertEqual(status.HTTP_200_OK, resp.status_code)
> -self.assertEqual(1, len(resp.data))
> -self.assertSerialized(cover_obj, resp.data[0])
> -
> -# authenticated user
> -user = create_user()
> -self.client.force_authenticate(user=user)
>  resp = self.client.get(self.api_url())
>  self.assertEqual(status.HTTP_200_OK, resp.status_code)
>  self.assertEqual(1, len(resp.data))
> -self.assertSerialized(cover_obj, resp.data[0])
> -
> -# test filtering by project
> -resp = self.client.get(self.api_url(), {'project': 'myproject'})
> -self.assertEqual([cover_obj.id], [x['id'] for x in resp.data])
> -resp = self.client.get(self.api_url(), {'project':
> 'invalidproject'})
> -self.assertEqual(0, len(resp.data))
> -
> -# test filtering by submitter, both ID and email
> -resp = self.client.get(self.api_url(), {'submitter': person_obj.id})
>

Re: [PATCH] tests: Replace incorrect tests

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: "Veronika Kabatova" <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Tuesday, April 17, 2018 11:51:24 AM
> Subject: Re: [PATCH] tests: Replace incorrect tests
> 
> On Tue, 2018-04-17 at 05:24 -0400, Veronika Kabatova wrote:
> > Thanks for the fast reaction and fix!
> > 
> > Veronika
> 
> No problem. Can I get an Acked-by: if you're happy with it? :)
> 

Sure!

Acked-by: Veronika Kabatova <vkaba...@redhat.com>

> Stephen
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [RFC 0/2 REBASE] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Veronika Kabatova" <vkaba...@redhat.com>
> To: "Stephen Finucane" <step...@that.guru>
> Cc: patchwork@lists.ozlabs.org
> Sent: Monday, April 16, 2018 3:06:34 PM
> Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> 
> - Original Message -
> > From: "Stephen Finucane" <step...@that.guru>
> > To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> > Sent: Monday, April 16, 2018 12:49:03 PM
> > Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> > 
> > On Thu, 2018-04-12 at 17:24 +0200, vkaba...@redhat.com wrote:
> > > From: Veronika Kabatova <vkaba...@redhat.com>
> > > 
> > > TL;DR:
> > > Our goals:
> > > - Avoid tag reextraction with each added comment
> > > - Fix issues #57 and #113
> > > - Prepare tags addition to comments in the API
> > > - Add tags to patch (currently returns {}) and cover letter APIs
> > > 
> > > If you agree with the general idea, I'd like to get some help with DB
> > > query
> > > optimizations where determined needed. Tags are more distributed, so
> > > counting
> > > them in PatchQuerySet is harder. I wanted to use annotation with Django's
> > > conditional selection, but it doesn't seem to work very well and
> > > generated
> > > invalid SQL in my attempts (hence my solution with counting the tags on
> > > the
> > > fly
> > > only for the patches that are being displayed). That said, I'm not a DB
> > > expert
> > > so any more optimized solutions are more than welcome.
> > > 
> > > 
> > > Thoughts? Ideas?
> > 
> > Took at look at this. As things stand, I'm not sure how we could
> > improve the performance of this substantially. The problem is that
> > we're trying to treat these as both a generic thing (cover letters,
> > patches _and_ comments can all have associated tags) but also somewhat
> > interrelated (a patch need to know about the tags attached to its
> > series' cover letter and its comments). This leads to the kind of
> > convoluted queries which impact your performance.
> > 
> > Let's take a look at the things we need to solve the goals you
> > highlighted above.
> > 
> >  * Keep track of where we found of the tag (patch, follow-up comment,
> >series/cover letter)
> > * This would resolve our tag re-extraction issue and potentially
> >   offer a path to resolving #113 (A/R/T on cover letter should
> >   increment the counters of every patch)
> > 
> >  * Keep track of the value-side of the equation (i.e. an email for
> >something like 'Reviewed-by:')
> > * This would solve #57 (Tags should be stored on a per-Person
> >   basis)
> > 
> > As noted above, we have a clash between specificness and genericness of
> > this field. I think we should double down on the former, even if it
> > means some level of duplication. To this end, what about linking 'Tag'
> > to 'Patch' via a ManyToMany field and use a "through" table? If we do
> > this, we can track optional 'cover' and 'comment' attributes for
> > identifying the source of the tag, if it wasn't included in the patch
> > itself. We could also store a value here. This would result in some
> > duplication (e.g. for a 10 series patch, a reply to the cover letter
> > would generate 10 PatchTag entries with series set for each) but it
> > would be simple to query. Does this make sense to you?
> > 
> 
> Hi,
> 
> I need to take a proper look later but for the first look, this solution
> might work out. I'll reimplement it and see if it works well for us and
> either send v2 for the RFC or let you know if I find any caveats.
> 

So I finally started digging into this alternative solution and run into
an issue with tags extracted from cover letters (we - and maybe others do
that too - often put eg. links to issue trackers in the cover letter
instead of putting them into each patch).

Usually, the cover letter is received before the patches from series. This
means if we only had the link between Tag and Patch (with the additional
information in the intermediate model), there will be no place to save tags
from cover letter at the time of parsing.

Unless I'm overlooking something, we'd need to have the link from Tag to
both Patch and CoverLetter. This should still have much better performance
than my original solution (and will get rid of the duplication of yours).

Does this proposal make sense, or am I missing something?

Thanks,
Veronika
> 
> > Stephen
> > 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [RFC 0/2 REBASE] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: "Veronika Kabatova" <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Tuesday, April 17, 2018 5:46:12 PM
> Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> 
> On Tue, 2018-04-17 at 11:35 -0400, Veronika Kabatova wrote:
> > > > Unless I'm overlooking something, we'd need to have the link from Tag
> > > > to
> > > > both Patch and CoverLetter. This should still have much better
> > > > performance
> > > > than my original solution (and will get rid of the duplication of
> > > > yours).
> > > > 
> > > > Does this proposal make sense, or am I missing something?
> > > 
> > > That mostly makes sense. My main concern is what happens when you want
> > > to show tags for a patch when those tags were created again the cover
> > > letter. If that's the case, are we going to have to query on
> > > 'patch.series.cover_letter.tags'? I imagine that's going to be slow
> > > (lots of JOINs). We could store it on the series instead, but I'm not
> > > sure how much that would improve things. Any ideas how to work around
> > > this?
> > > 
> > 
> > I was thinking about filtering on the SubmissionTag (or whatever the
> > intermediate model will be named) based on submission IDs of the patch
> > and cover (or comment IDs in case of comments API), instead of going
> > through the relations. That said, my database knowledge is very...
> > abstract... so I have no idea how much it helps with the underlying
> > queries.
> > 
> > If you (or whoever else) can offer any insight that would be great!
> 
> We'd still need to get information about the cover letter though, and
> that requires going through the series (one join). Maybe we already
> have that JOIN though, so this warrants some validation.
> 

We already have the series in the API. For the view, we are prefetching
them, but only after annotation with tag counts. Will it help to change
the order there?

> Another idea I've had is to store a series attribute in addition to the
> cover letter, comment and patch attributes. That way we could do
> something like this for patches:
> 
>tags = Tag.objects.filter(series=patch.series,
>  Q(patch=patch) | Q(patch=None))
> 
> e.g. if the patch is part of our series and doesn't belong to _another_
> patch, it must be a series-wide patch? You'd need to do additional
> filtering on this for duplicates, of course, but I imagine that's easy
> enough. You'd also want to make liberal use of the 'only' and 'defer'
> functions to make sure we avoid as many joins as possible, however, I
> don't think this would require a join on the 'patchwork_series' table
> as we only use the ID column (which we'd have).
> 

I find having separate tables for cover letter and patch tags easier to
wrap around (and avoiding most of the duplication), but in case you think
the above won't help with performance, I'll go this way and see how it
works out. Yeah, getting out only distinct values is the easiest part :)

Veronika

> Thoughts?
> Stephen
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [RFC 0/2 REBASE] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: "Veronika Kabatova" <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Tuesday, April 17, 2018 5:17:19 PM
> Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> 
> On Tue, 2018-04-17 at 09:50 -0400, Veronika Kabatova wrote:
> > - Original Message -
> > > From: "Veronika Kabatova" <vkaba...@redhat.com>
> > > To: "Stephen Finucane" <step...@that.guru>
> > > Cc: patchwork@lists.ozlabs.org
> > > Sent: Monday, April 16, 2018 3:06:34 PM
> > > Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> > > 
> > > - Original Message -
> > > > From: "Stephen Finucane" <step...@that.guru>
> > > > To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> > > > Sent: Monday, April 16, 2018 12:49:03 PM
> > > > Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> > > > 
> > > > On Thu, 2018-04-12 at 17:24 +0200, vkaba...@redhat.com wrote:
> > > > > From: Veronika Kabatova <vkaba...@redhat.com>
> > > > > 
> > > > > TL;DR:
> > > > > Our goals:
> > > > > - Avoid tag reextraction with each added comment
> > > > > - Fix issues #57 and #113
> > > > > - Prepare tags addition to comments in the API
> > > > > - Add tags to patch (currently returns {}) and cover letter APIs
> > > > > 
> > > > > If you agree with the general idea, I'd like to get some help with DB
> > > > > query
> > > > > optimizations where determined needed. Tags are more distributed, so
> > > > > counting
> > > > > them in PatchQuerySet is harder. I wanted to use annotation with
> > > > > Django's
> > > > > conditional selection, but it doesn't seem to work very well and
> > > > > generated
> > > > > invalid SQL in my attempts (hence my solution with counting the tags
> > > > > on
> > > > > the
> > > > > fly
> > > > > only for the patches that are being displayed). That said, I'm not a
> > > > > DB
> > > > > expert
> > > > > so any more optimized solutions are more than welcome.
> > > > > 
> > > > > 
> > > > > Thoughts? Ideas?
> > > > 
> > > > Took at look at this. As things stand, I'm not sure how we could
> > > > improve the performance of this substantially. The problem is that
> > > > we're trying to treat these as both a generic thing (cover letters,
> > > > patches _and_ comments can all have associated tags) but also somewhat
> > > > interrelated (a patch need to know about the tags attached to its
> > > > series' cover letter and its comments). This leads to the kind of
> > > > convoluted queries which impact your performance.
> > > > 
> > > > Let's take a look at the things we need to solve the goals you
> > > > highlighted above.
> > > > 
> > > >  * Keep track of where we found of the tag (patch, follow-up comment,
> > > >series/cover letter)
> > > > * This would resolve our tag re-extraction issue and potentially
> > > >   offer a path to resolving #113 (A/R/T on cover letter should
> > > >   increment the counters of every patch)
> > > > 
> > > >  * Keep track of the value-side of the equation (i.e. an email for
> > > >something like 'Reviewed-by:')
> > > > * This would solve #57 (Tags should be stored on a per-Person
> > > >   basis)
> > > > 
> > > > As noted above, we have a clash between specificness and genericness of
> > > > this field. I think we should double down on the former, even if it
> > > > means some level of duplication. To this end, what about linking 'Tag'
> > > > to 'Patch' via a ManyToMany field and use a "through" table? If we do
> > > > this, we can track optional 'cover' and 'comment' attributes for
> > > > identifying the source of the tag, if it wasn't included in the patch
> > > > itself. We could also store a value here. This would result in some
> > > > duplication (e.g. for a 10 series patch, a reply to the cover letter
> > > > would generate 10 PatchTag entries with series set for each) but it
> > > > would

Re: [PATCH 0/5] Random API improvements

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: "Veronika Kabatova" <vkaba...@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Saturday, April 7, 2018 6:55:10 PM
> Subject: Re: [PATCH 0/5] Random API improvements
> 
> On Mon, 2018-03-26 at 08:22 -0400, Veronika Kabatova wrote:
> > - Original Message -
> > > From: "Stephen Finucane" <step...@that.guru>
> > > To: patchwork@lists.ozlabs.org
> > > Sent: Sunday, March 25, 2018 8:28:18 PM
> > > Subject: [PATCH 0/5] Random API improvements
> > > 
> > > A number of improvements for various aspects of the API. The biggest
> > > change here is probably the inclusion of API versioning functionality,
> > > though there are also improvements for testing and the likes.
> > > 
> > > This is a pre-requisite for forthcoming fixes for #156 [1].
> > > 
> > > [1] https://github.com/getpatchwork/patchwork/issues/156
> > > 
> > > Stephen Finucane (5):
> > >   tests: Split 'test_rest_api'
> > 
> > Hi,
> > 
> > it seems to me like you forgot to send the patch mentioned above,
> > I can't see it in the PW instance on ozlabs nor the archive.
> > 
> > Veronika
> 
> Not sure why this didn't send. I received it but it didn't make it to
> the list. Maybe some encoding issue? In any case, these were all
> trivial so I went ahead and applied them.
> 

Hi,

resurrecting the thread since that patch made tests for checks disappear
and instead we have the cover letter tests present twice.

Can you please put them back? :)

Thanks,
Veronika

> Stephen
> 
> > >   REST: Use versioning for modified responses
> > >   docs: Add information on REST API versioning
> > >   docs: Add note on backing up the docker database
> > >   REST: Order 'filters' code
> > > 
> > >  docs/development/contributing.rst   |  15 +
> > >  docs/development/installation.rst   |  13 +-
> > >  docs/development/releasing.rst  |  23 +
> > >  patchwork/api/base.py   |  27 ++
> > >  patchwork/api/cover.py  |   7 +-
> > >  patchwork/api/embedded.py   |  25 +-
> > >  patchwork/api/filters.py|  86 ++--
> > >  patchwork/api/project.py|   7 +-
> > >  patchwork/tests/api/__init__.py |   0
> > >  patchwork/tests/api/test_bundle.py  | 134 ++
> > >  patchwork/tests/api/test_check.py   | 121 +
> > >  patchwork/tests/api/test_cover.py   | 131 ++
> > >  patchwork/tests/api/test_patch.py   | 208 +
> > >  patchwork/tests/api/test_person.py  | 114 +
> > >  patchwork/tests/api/test_project.py | 179 
> > >  patchwork/tests/api/test_user.py|  89 
> > >  patchwork/tests/test_rest_api.py| 869
> > >  
> > >  patchwork/urls.py   |   2 +-
> > >  18 files changed, 1122 insertions(+), 928 deletions(-)
> > >  create mode 100644 patchwork/tests/api/__init__.py
> > >  create mode 100644 patchwork/tests/api/test_bundle.py
> > >  create mode 100644 patchwork/tests/api/test_check.py
> > >  create mode 100644 patchwork/tests/api/test_cover.py
> > >  create mode 100644 patchwork/tests/api/test_patch.py
> > >  create mode 100644 patchwork/tests/api/test_person.py
> > >  create mode 100644 patchwork/tests/api/test_project.py
> > >  create mode 100644 patchwork/tests/api/test_user.py
> > >  delete mode 100644 patchwork/tests/test_rest_api.py
> > > 
> > > --
> > > 2.14.3
> > > 
> > > ___
> > > Patchwork mailing list
> > > Patchwork@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/patchwork
> > > 
> 
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [RFC 0/2 REBASE] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, April 16, 2018 12:49:03 PM
> Subject: Re: [RFC 0/2 REBASE] Rework tagging infrastructure
> 
> On Thu, 2018-04-12 at 17:24 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > (TL;DR at the end)
> > 
> > This RFC describes an approach to rework tagging. It attempts to solve
> > GitHub
> > issues #57 [1] and #113 [2] as well as some other things we encountered.
> > I'm
> > sending the incomplete version (eg I haven't fixed the tests) to discuss
> > the
> > approach first.
> > 
> > Right now, tags are extracted from all the comments on the patch and the
> > patch
> > itself, and they are reextracted from all the sources every time a comment
> > is
> > added or removed. It makes saving slower, and might contribute to races
> > with
> > writes to database when we are parsing multiple emails at the same time.
> > This
> > gets even more prevalent if we want to solve the issue #113 (tags on cover
> > letter should increment counters on every patch in series) -- for each
> > added
> > comment on the cover letter, we would reextract tags from all the other
> > sources,
> > for each patch in series; and for a change on comments related to the patch
> > directly, we would need to take the tags on the cover letter and it's
> > comments
> > into account as well (I implemented this solution in my fork but I really
> > don't like it).
> > 
> > The current approach has several other issues as well, some of which are
> > mentioned in the issue #57, eg duplicate tags are counted more times.
> > Taking
> > into account the tags on cover letters would also be easier if we could
> > store
> > tags against them and just query them on demand, instead of reextracting
> > everything all over again.
> > 
> > Solutions for some other things we found missing solve the issues mentioned
> > above too. If we want to determine if the tag is duplicate, we need to save
> > the
> > associated value. Having the value would help us to use arbitrary strings
> > as
> > tags (for example links to issue trackers, like `Bugzilla: ` if the
> > patch
> > solves a known bug). The key-values approach to storing tags is mentioned
> > [3],
> > this email additionally mentions a comments REST API (currently worked on).
> > For
> > the comments API we would also find it very useful to have the tags
> > extracted
> > from the comments available directly so we can query for them, which means
> > we
> > would either need to reextract the tags on every API call, or we could
> > store
> > the tags against the comments as they are extracted and only query them as
> > needed. Keeping tags related to comments where they originated from avoids
> > the
> > need to reextract tags with addition of new comments, as well as gets rid
> > of
> > the `patch_responses` property used when converting comments to mbox (we
> > finally get all the custom tags there instead of only the few hardcoded
> > ones).
> > 
> > TL;DR:
> > Our goals:
> > - Avoid tag reextraction with each added comment
> > - Fix issues #57 and #113
> > - Prepare tags addition to comments in the API
> > - Add tags to patch (currently returns {}) and cover letter APIs
> > 
> > If you agree with the general idea, I'd like to get some help with DB query
> > optimizations where determined needed. Tags are more distributed, so
> > counting
> > them in PatchQuerySet is harder. I wanted to use annotation with Django's
> > conditional selection, but it doesn't seem to work very well and generated
> > invalid SQL in my attempts (hence my solution with counting the tags on the
> > fly
> > only for the patches that are being displayed). That said, I'm not a DB
> > expert
> > so any more optimized solutions are more than welcome.
> > 
> > 
> > Thoughts? Ideas?
> 
> Took at look at this. As things stand, I'm not sure how we could
> improve the performance of this substantially. The problem is that
> we're trying to treat these as both a generic thing (cover letters,
> patches _and_ comments can all have associated tags) but also somewhat
> interrelated (a patch need to know about the tags attached to its
> series' cover letter and its comments). This leads to the kind of
> convoluted queries which impact your performance.
> 
> Let's take a look at the th

Re: [PATCH v2 1/2] api: Add comments to patch and cover endpoints

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, April 16, 2018 1:15:24 AM
> Subject: Re: [PATCH v2 1/2] api: Add comments to patch and cover endpoints
> 
> On Wed, 2018-04-11 at 19:45 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> 
> I thought I'd replied to this already but I clearly didn't. This all
> looks pretty good but I think I might have confused you. It seems with
> this version our patch/cover letter responses gain an entire nested
> version of the comments. This seems overly detailed, given that it's
> not something most people need. What I meant instead is to add a
> comments URL like so:
> 

Yes, that's why I double questioned your idea :)

>   /patch/{patchID}/comments
> 
> ...and include this in the patch response:
> 
>   {
> "id": 123,
> ...
> "comments": "https://www.example.com/api/patches/123/comments;
>   }
> 
> Any reason not to go for this option?
> 

I guess not, will move the comments there.

> Also, if you do end up respinning this just squash tests/migrations in
> with this patch. I'd be applying them together anyway.
> 

Will do.

Thanks,
Veronika

> Stephen
> 
> > ---
> >  patchwork/api/cover.py | 14 ++--
> >  patchwork/api/embedded.py  | 38
> >  ++
> >  patchwork/api/patch.py | 15 ++---
> >  patchwork/models.py|  3 ++
> >  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |  8 +
> >  5 files changed, 70 insertions(+), 8 deletions(-)
> >  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > 
> > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > index fc7ae97..5a6e377 100644
> > --- a/patchwork/api/cover.py
> > +++ b/patchwork/api/cover.py
> > @@ -25,6 +25,7 @@ from rest_framework.serializers import
> > SerializerMethodField
> >  
> >  from patchwork.api.base import BaseHyperlinkedModelSerializer
> >  from patchwork.api.filters import CoverLetterFilter
> > +from patchwork.api.embedded import CommentSerializer
> >  from patchwork.api.embedded import PersonSerializer
> >  from patchwork.api.embedded import ProjectSerializer
> >  from patchwork.api.embedded import SeriesSerializer
> > @@ -58,5 +59,6 @@ class
> > CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> >  class CoverLetterDetailSerializer(CoverLetterListSerializer):
> >  
> >  headers = SerializerMethodField()
> > +comments = CommentSerializer(many=True, read_only=True)
> >  
> >  def get_headers(self, instance):
> > @@ -65,9 +67,14 @@ class
> > CoverLetterDetailSerializer(CoverLetterListSerializer):
> >  
> >  class Meta:
> >  model = CoverLetter
> > -fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > 'content')
> > +fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > +  'content',
> > +  'comments')
> >  read_only_fields = fields
> >  extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
> > +versioned_fields = {
> > +'1.1': ('mbox', 'comments'),
> > +}
> >  
> >  
> >  class CoverLetterList(ListAPIView):
> > @@ -91,5 +98,6 @@ class CoverLetterDetail(RetrieveAPIView):
> >  serializer_class = CoverLetterDetailSerializer
> >  
> >  def get_queryset(self):
> > -return CoverLetter.objects.all().prefetch_related('series')\
> > -.select_related('project', 'submitter')
> > +return CoverLetter.objects.all().prefetch_related(
> > +'series', 'comments'
> > +).select_related('project', 'submitter')
> > diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
> > index d79724c..afd511a 100644
> > --- a/patchwork/api/embedded.py
> > +++ b/patchwork/api/embedded.py
> > @@ -23,6 +23,8 @@ A collection of serializers. None of the serializers here
> > should reference
> >  nested fields.
> >  """
> >  
> > +import email.parser
> > +
> >  from rest_framework.serializers import CharField
> >  from rest_framework.serializers import SerializerM

Re: [RFC v2 1/1] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, April 25, 2018 12:13:40 PM
> Subject: Re: [RFC v2 1/1] Rework tagging infrastructure
> 
> On Thu, 2018-04-19 at 18:36 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > Solve #113 and #57 GitHub issues, fix up returning tags in the API,
> > keep track of tag origin to later be able to add tags to comments in
> > the API.
> > 
> > Use relations Tag-Patch and Tag-CoverLetter to avoid duplication of
> > tags for each patch in series.
> > 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> 
> Comments below. Most of them are little improvements I'd like to see,
> but there is one big issue around the difference between SeriesPatch
> and Patch that we need to think about.
> 

Thanks for comments! I'll check out and include the small improvements
later, but let's figure out the major things first.

> Stephen
> 
> > ---
> >  docs/usage/overview.rst |   9 +-
> >  patchwork/api/cover.py  |  24 +++-
> >  patchwork/api/patch.py  |  35 +-
> >  patchwork/management/commands/retag.py  |  15 ++-
> >  patchwork/migrations/0026_rework_tagging.py | 123 +++
> >  patchwork/models.py | 176
> >  +++-
> >  patchwork/templatetags/patch.py |   3 +-
> >  patchwork/views/__init__.py |   3 -
> >  patchwork/views/utils.py|  16 ++-
> >  9 files changed, 277 insertions(+), 127 deletions(-)
> >  create mode 100644 patchwork/migrations/0026_rework_tagging.py
> > 
> > diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
> > index cc193f3..d7db840 100644
> > --- a/docs/usage/overview.rst
> > +++ b/docs/usage/overview.rst
> > @@ -110,10 +110,11 @@ one delegate can be assigned to a patch.
> >  Tags
> >  
> >  
> > -Tags are specially formatted metadata appended to the foot the body of a
> > patch
> > -or a comment on a patch. Patchwork extracts these tags at parse time and
> > -associates them with the patch. You add extra tags to an email by replying
> > to
> > -the email. The following tags are available on a standard Patchwork
> > install:
> > +Tags are specially formatted metadata appended to the foot the body of a
> > patch,
> > +cover letter or a comment related to them. Patchwork extracts these tags
> > at
> > +parse time and associates them with patches. You add extra tags to an
> > email by
> > +replying to the email. The following tags are available on a standard
> > Patchwork
> > +install:
> >  
> >  Acked-by:
> >  
> > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > index fc7ae97..4d82277 100644
> > --- a/patchwork/api/cover.py
> > +++ b/patchwork/api/cover.py
> > @@ -29,6 +29,7 @@ from patchwork.api.embedded import PersonSerializer
> >  from patchwork.api.embedded import ProjectSerializer
> >  from patchwork.api.embedded import SeriesSerializer
> >  from patchwork.models import CoverLetter
> > +from patchwork.models import SubmissionTag
> >  
> >  
> >  class CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> > @@ -37,18 +38,36 @@ class
> > CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> >  submitter = PersonSerializer(read_only=True)
> >  mbox = SerializerMethodField()
> >  series = SeriesSerializer(many=True, read_only=True)
> > +tags = SerializerMethodField()
> >  
> >  def get_mbox(self, instance):
> >  request = self.context.get('request')
> >  return request.build_absolute_uri(instance.get_mbox_url())
> >  
> > +def get_tags(self, instance):
> > +tags = instance.project.tags
> > +if not tags:
> > +return {}
> > +
> > +all_tags = {}
> > +related_tags = SubmissionTag.objects.filter(
> > +submission=instance).values_list('tag__name',
> > 'value').distinct()
> > +for tag in tags:
> > +all_tags[tag.name] = [related_tag[1] for related_tag in
> > +  related_tags if related_tag[0] ==
> > tag.name]
> > +# Don't show tags that are not present
> > +if not all_tags[tag.name]:
> > +del(all_tags[tag.name])
> > +
> > 

Re: [PATCH v3] api: Add comments to patch and cover endpoints

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" <step...@that.guru>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Wednesday, April 25, 2018 10:53:14 AM
> Subject: Re: [PATCH v3] api: Add comments to patch and cover endpoints
> 
> On Tue, 2018-04-17 at 12:50 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova <vkaba...@redhat.com>
> > 
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > ---
> > New changes: move things under /patches|covers/ID/comments
> 
> Almost there - just some queries to clean up.
> 
> > ---
> >  patchwork/api/comment.py   |  80 ++
> >  patchwork/api/cover.py |  13 ++-
> >  patchwork/api/patch.py |  16 ++-
> >  patchwork/models.py|   3 +
> >  patchwork/tests/api/test_comment.py| 115
> >  +
> >  patchwork/tests/api/test_cover.py  |  11 +-
> >  patchwork/tests/api/test_patch.py  |  19 +++-
> >  patchwork/urls.py  |  11 ++
> >  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |   8 ++
> >  9 files changed, 267 insertions(+), 9 deletions(-)
> >  create mode 100644 patchwork/api/comment.py
> >  create mode 100644 patchwork/tests/api/test_comment.py
> >  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > 
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 000..a650168
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,80 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > USA
> > +
> > +import email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.api.embedded import ProjectSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > +
> > +project = ProjectSerializer(source='submission.project',
> > read_only=True)
> 
> This results in N * 2 addition queries for N comments because of the
> extra reads to 'submission' and 'project'. You can resolve this
> somewhat, as noted below, but I wonder if this is even necessary?
> Couldn't we infer it from the parent patch?
> 

Right, the users can just work with the project mentioned in the patch
detail since they need to get there anyways to get to the comments. I'll
remove that.

> > +tags = SerializerMethodField()
> > +subject = SerializerMethodField()
> > +headers = SerializerMethodField()
> > +submitter = PersonSerializer(read_only=True)
> > +
> > +def get_tags(self, comment):
> > +# TODO implement after we get support for tags on comments
> > +return {}
> > +
> > +def get_subject(self, comment):
> > +return email.parser.Parser().parsestr(comment.headers,
> > +  True).get('Subject', '')
> > +
> > +def get_headers(self, comment):
> > +headers = {}
> > +
> > +if comment.headers:
> > +parsed = email.parser.Parser().parsestr(comment.headers, True)
> > +for key in parsed.keys():
> > +headers[key] = parsed.get_all(key)
> > +# Let's return a sing

Re: [PATCH] Add comments REST API

[Veronika Kabatova]

- Original Message -
> From: vkaba...@redhat.com
> To: patchwork@lists.ozlabs.org
> Cc: "Veronika Kabatova" <vkaba...@redhat.com>
> Sent: Friday, March 23, 2018 1:33:38 PM
> Subject: [PATCH] Add comments REST API
> 
> From: Veronika Kabatova <vkaba...@redhat.com>
> 
> Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> ---
>  docs/api/rest.rst  |   6 +-
>  patchwork/api/comment.py   | 117
>  +
>  patchwork/api/filters.py   |  16 +++
>  patchwork/api/index.py |   1 +
>  patchwork/models.py|   3 +
>  patchwork/urls.py  |  10 ++
>  .../notes/comments-api-b7dff6ee4ce04c9b.yaml   |  10 ++
>  7 files changed, 161 insertions(+), 2 deletions(-)
>  create mode 100644 patchwork/api/comment.py
>  create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml

Before someone mentions it, I'm aware the tests are missing. I wanted to get
feedback about the functionality first so I don't need to rework them later
(and with the recent Stephen's API tests rework, it was definitely a good
idea to wait).

Veronika
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: Patchwork 3: potential feature removals

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" 
> To: patchwork@lists.ozlabs.org
> Sent: Thursday, March 22, 2018 12:17:20 AM
> Subject: Patchwork 3: potential feature removals
> 
> Hi all,
> 

Hi,

> Patchwork is currently quite complex. This is adding to the bug surface
> and impacting reliablity - which as the recent issues have
> reinforced, is a fundamental feature for our users.
> 
> Patchwork is also maintained by a small, loosely-coordinated team of
> unpaid volunteers, and this stuff is increasing the maintenance burden,
> making working on patchwork more difficult and significantly more
> unpleasant.
> 
> I'm considering the following for the next major version of Patchwork -
> basically what will come after 2.n bug and stability fixes. Semantic
> versioning requires me to call it Patchwork 3, even though I expect
> user-facing changes to be very small compared to the 1->2 change. I'm
> looking at a timeline of the tail end of this year, but being patchwork,
> who knows when it'll eventually happen.
> 
> Please let me know if anything is particularly of concern or interest to
> you.
> 
>  1) Support Django 2.0+ only. This implies Py3 only.
> 
>  2) Drop patch status change notifications, and the associated opt-in/outs.
> 
>  3) Drop XMLRPC. pwclient will be rewritten to use the REST API.
> 
>  4) Drop the ability to disable tags for a project. Every project will
> use the tags configured for the instance.
> 
>  5) Drop Patchwork specific headers, except X-Patchwork-Hint: ignore.
> (i.e. X-Patchwork-State and X-Patchwork-Delegate.) I'm not sure if
> they're used and I think they would be quite surprising to a lot of
> project maintainers and users.
> 
>  6) (Possibly) drop support for piecing together unthreaded
> series. (When someone sends patches not in reply to the first
> patch/cover letter.) I think this will make correct series parsing
> easier in the presence of parallelism; if not I'm happy to keep
> it. (I think it might actually be nigh-on impossible to do both
> correct parallel parsing and support unthreaded series, but I
> haven't properly tried yet.)
> 
> I would also really, really like to drop MySQL, unless anyone uses it in
> production. If you are using it in production, please, please let me
> know. Please also let me know if you'd be willing to migrate to Postgres
> if I provide a migration script.
> 

I like these ideas. Upstream support for Py2 ends in 2020 so moving
towards Py3-only codebase makes sense. As long as the
X-Patchwork-Hint: ignore
stays, I don't care much for the features mentioned (and some of them,
like the delegate header can be worked around, eg this one by the
autodelegation rules).

> I'm still happy to take patches for new features from the community -
> please don't interpret this as any sort of feature freeze! (Having said
> that, I'd be even happier to take bug fixes, performance fixes, and
> general cleanups!)

Happy to hear that, since we still miss a bunch of features we'd
eventually liked to get upstream :) (and of course bug fixes if we run
into any issues). That said, the reason why I'm writing this email is
because I'd like to help out with the cleanup as the times comes.
Please don't hesitate to reach out.

Veronika

> 
> Regards,
> Daniel
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH v3] Avoid timezone confusion

[Veronika Kabatova]

- Original Message -
> From: "Daniel Axtens" <d...@axtens.net>
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Saturday, February 24, 2018 2:58:56 AM
> Subject: Re: [PATCH v3] Avoid timezone confusion
> 
> Hi Veronika,
> 
> I am testing this now and I think I'm happy with it.
> 
> However, I set up a container in UTC+11, and while watching the events
> api endpoint:
>  - I added some patches without this patch
>  - I applied this patch and migrated
>  - I added some more patches
>  - I noticed that the new events were no longer added to the front of
>  the events list; they were now 11 hours back.
> 
> This is, on reflection, a straight-forward consequence of moving from
> UTC+11 to UTC without attempting to migrate old data.
> 
> I still think migration is an incredibly difficult and error-prone waste
> of time, so I still don't want to go down that road. What I do want is a
> piece of documentation in the release notes to warn people that if you
> are in UTC+n, your events feed will be out of order for n hours.
> 

Yes, the "delayed" events are expected. Additional documentation about
it seems fair.

> I don't know of any users of the event feed at the moment, so I'm not
> worried about breaking anything: I just want a couple of sentences so
> that any sysadmins/users who are especially observant don't freak out
> that they're suddenly losing events.
> 

We tried it but the events API was very slow and unreliable so we
switched back to polling series. I believe Stephen worked on fixing
up the API so hopefully there will be a working solution merged soon :)

> You can do a respin of this, or just send a new patch on top of this and
> I will apply both at the same time.
> 

I'll send an additional patch with releasnote changes and cc you so
you can find it easily.

> Regards,
> Daniel
> 
> vkaba...@redhat.com writes:
> 
> > From: Veronika Kabatova <vkaba...@redhat.com>
> >
> > Patchwork saves patches, comments etc with UTC timezone and reports
> > this time when opening the patch details. However, internally generated
> > processes such as events are reported with the instance's local time.
> > There's nothing wrong with that and making PW timezone-aware would add
> > useless complexity, but in a world-wide collaboration a lot of confusion
> > may arise as the timezone is not reported at all. Instance's local time
> > might be very different from the local time of CI integrating with PW,
> > which is different from the local time of person dealing with it etc.
> >
> > Use UTC everywhere by default instead of UTC for sumbissions and local
> > timezone for internally generated events (which is not reported).
> >
> > Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> > ---
> > Changes: Implement Daniel's idea about using datetime.datetime.utcnow
> > ---
> >  docs/api/rest.rst  |  3 +-
> >  patchwork/migrations/0024_timezone_unify.py| 46
> >  ++
> >  patchwork/models.py| 12 +++---
> >  patchwork/notifications.py |  4 +-
> >  patchwork/signals.py   |  2 +-
> >  patchwork/templates/patchwork/submission.html  |  4 +-
> >  patchwork/tests/test_checks.py | 10 ++---
> >  patchwork/tests/test_expiry.py |  8 ++--
> >  patchwork/tests/test_notifications.py  |  2 +-
> >  patchwork/tests/utils.py   |  6 +--
> >  .../notes/unify-timezones-0f7022f0c2a371be.yaml|  5 +++
> >  11 files changed, 77 insertions(+), 25 deletions(-)
> >  create mode 100644 patchwork/migrations/0024_timezone_unify.py
> >  create mode 100644
> >  releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> >
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index 3d7292e..d526b27 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -107,7 +107,8 @@ Schema
> >  --
> >  
> >  Responses are returned as JSON. Blank fields are returned as ``null``,
> >  rather
> > -than being omitted. Timestamps use the ISO 8601 format::
> > +than being omitted. Timestamps use the ISO 8601 format, times are by
> > default
> > +in UTC::
> >  
> >  -MM-DDTHH:MM:SSZ
> >  
> > diff --git a/patchwork/migrations/0024_timezone_unify.py
> > b/patchwork/migrations/0024_timezone_unify.py
> > new file mode 100644
> > index 000..99f0642
> > --- /dev/null
> > +++ b/patchwork/migrations/

Re: [PATCH] Don't passthrough 'Content-Type: multipart/signed' header

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: patchwork@lists.ozlabs.org
> Cc: "Stephen Finucane" , "Veronika Kabatova" 
> 
> Sent: Sunday, November 4, 2018 3:27:04 PM
> Subject: [PATCH] Don't passthrough 'Content-Type: multipart/signed' header
> 
> We don't GPG signatures, therefore this header is incorrect. Stop
> passing it through.
> 
> Test for the other dropped header are also included.
> 
> Signed-off-by: Stephen Finucane 
> Cc: Veronika Kabatova 
> Closes: #221
> ---
>  patchwork/tests/test_mboxviews.py | 15 +++
>  patchwork/views/utils.py  |  6 ++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/patchwork/tests/test_mboxviews.py
> b/patchwork/tests/test_mboxviews.py
> index 50444d65..87c75eca 100644
> --- a/patchwork/tests/test_mboxviews.py
> +++ b/patchwork/tests/test_mboxviews.py
> @@ -111,6 +111,21 @@ class MboxHeaderTest(TestCase):
>  header = 'List-Id: Patchwork development
>  '
>  self._test_header_passthrough(header)
>  
> +def _test_header_dropped(self, header):
> +patch = create_patch(headers=header + '\n')
> +response = self.client.get(reverse('patch-mbox', args=[patch.id]))
> +self.assertNotContains(response, header)
> +
> +def test_header_dropped_content_transfer_encoding(self):
> +"""Validate dropping of 'Content-Transfer-Encoding' header."""
> +header = 'Content-Transfer-Encoding: quoted-printable'
> +self._test_header_dropped(header)
> +
> +def test_header_dropped_content_type_multipart_signed(self):
> +"""Validate dropping of 'Content-Type=multipart/signed' header."""
> +header = 'Content-Type: multipart/signed'
> +self._test_header_dropped(header)
> +
>  def test_patchwork_id_header(self):
>  """Validate inclusion of generated 'X-Patchwork-Id' header."""
>  patch = create_patch()
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index 3c5d2982..1da1aaab 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -84,8 +84,14 @@ def _submission_to_mbox(submission):
>  
>  orig_headers = HeaderParser().parsestr(str(submission.headers))
>  for key, val in orig_headers.items():
> +# we set this ourselves
>  if key == 'Content-Transfer-Encoding':
>  continue
> +# we don't save GPG signatures described in RFC1847 [1] so this
> +# Content-Type value is invalid
> +# [1] https://tools.ietf.org/html/rfc1847
> +if key == 'Content-Type' and val == 'multipart/signed':
> +continue
>  mail[key] = val
>  

Good catch!

Acked-by: Veronika Kabatova 

>  if 'Date' not in mail:
> --
> 2.19.1
> 
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Add help text to project and patch_project fields

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: "Daniel Axtens" , vkaba...@redhat.com, 
> patchwork@lists.ozlabs.org
> Sent: Sunday, September 30, 2018 12:23:13 AM
> Subject: Re: [PATCH] Add help text to project and patch_project fields
> 
> On Sat, 2018-09-29 at 01:24 +1000, Daniel Axtens wrote:
> > vkaba...@redhat.com writes:
> > 
> > > From: Veronika Kabatova 
> > > 
> > > Duplication of the field to avoid performance issues caused some
> > > unexpected results when moving patches between projects in the admin
> > > interface. It's easy to change the "project" field and click save,
> > > instead of double checking which field needs to be modified and kept in
> > > sync with the one being changed. This lead to patch showing correct
> > > project in the API and patch detail in the web UI, but the patch itself
> > > was listed in the wrong project when looking at the patch listings.
> > > 
> > > Because of the discussions of the denormalization of the tables, trying
> > > to code automatic modification of the other field if one is being
> > > changed seems like too much work for very little benefit. What we can do
> > > instead is mention this dependency in fields' help text. Modification of
> > > the project is not an everyday task so it shouldn't put too much burden
> > > on the users and this simple reminder can prevent unexpected breakage
> > > and bug reports.
> > > 
> > 
> > Acked-by: Daniel Axtens 
> > > Signed-off-by: Veronika Kabatova 
> > 
> > Stephen, I think this should go to stable/2.1. Thoughts?
> 
> I don't think it can. I haven't checked yet but I'm pretty sure this
> requires a migration and we can't backport those.
> 
> Rather than doing this, could we not just override the 'save' function
> to always save the other field?
> 

We'd need to figure out which one of the fields is the changed one and
modify the other one to match it, which based on [1] doesn't look as easy
as it sounds. "I'll scroll till I find the field I want" and "here's the
long content, I'll go to the end and scroll up" would lead to the same but
opposite issue instead of fixing the problem, if we decided to hardcode one
of the fields as the one to be copied over.

Because of the above, I opted for the simplest help text solution. I'm not
opposed to implement a more complicated solution that doesn't require any
steps from the user (it was the first solution I looked into) if you think
the complications are worth it, or have a better idea how to work around it.


Veronika


[1] 
https://stackoverflow.com/questions/1355150/django-when-saving-how-can-you-check-if-a-field-has-changed

> Stephen
> 
> > Regards,
> > Daniel
> > 
> > > ---
> > >  patchwork/models.py | 12 ++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/patchwork/models.py b/patchwork/models.py
> > > index a043844d..e30a9739 100644
> > > --- a/patchwork/models.py
> > > +++ b/patchwork/models.py
> > > @@ -350,7 +350,11 @@ class FilenameMixin(object):
> > >  class Submission(FilenameMixin, EmailMixin, models.Model):
> > >  # parent
> > >  
> > > -project = models.ForeignKey(Project, on_delete=models.CASCADE)
> > > +project = models.ForeignKey(Project, on_delete=models.CASCADE,
> > > +help_text='If modifying this field on
> > > Patch, '
> > > +'don\'t forget to modify the "Patch
> > > project" '
> > > +'field appropriately as well to ensure
> > > proper '
> > > +'functionality!')
> > >  
> > >  # submission metadata
> > >  
> > > @@ -405,7 +409,11 @@ class Patch(Submission):
> > >  
> > >  # duplicate project from submission in subclass so we can count the
> > >  # patches in a project without needing to do a JOIN.
> > > -patch_project = models.ForeignKey(Project, on_delete=models.CASCADE)
> > > +patch_project = models.ForeignKey(Project, on_delete=models.CASCADE,
> > > +  help_text='"Project" field needs
> > > to be '
> > > +  'kept in sync and changed manually
> > > in '
> > > +  'case of modifications to ensure
> > > proper '
> > > +  'functionality!')
> > >  
> > >  objects = PatchManager()
> > >  
> > > --
> > > 2.17.1
> > > 
> > > ___
> > > Patchwork mailing list
> > > Patchwork@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/patchwork
> > 
> > ___
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 
> 
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Add help text to project and patch_project fields

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: "Veronika Kabatova" 
> Cc: "Daniel Axtens" , patchwork@lists.ozlabs.org
> Sent: Monday, October 1, 2018 11:57:09 AM
> Subject: Re: [PATCH] Add help text to project and patch_project fields
> 
> On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
> > 
> > - Original Message -
> > > From: "Stephen Finucane" 
> > > To: "Daniel Axtens" , vkaba...@redhat.com,
> > > patchwork@lists.ozlabs.org
> > > Sent: Sunday, September 30, 2018 12:23:13 AM
> > > Subject: Re: [PATCH] Add help text to project and patch_project fields
> > > 
> > > On Sat, 2018-09-29 at 01:24 +1000, Daniel Axtens wrote:
> > > > vkaba...@redhat.com writes:
> > > > 
> > > > > From: Veronika Kabatova 
> > > > > 
> > > > > Duplication of the field to avoid performance issues caused some
> > > > > unexpected results when moving patches between projects in the admin
> > > > > interface. It's easy to change the "project" field and click save,
> > > > > instead of double checking which field needs to be modified and kept
> > > > > in
> > > > > sync with the one being changed. This lead to patch showing correct
> > > > > project in the API and patch detail in the web UI, but the patch
> > > > > itself
> > > > > was listed in the wrong project when looking at the patch listings.
> > > > > 
> > > > > Because of the discussions of the denormalization of the tables,
> > > > > trying
> > > > > to code automatic modification of the other field if one is being
> > > > > changed seems like too much work for very little benefit. What we can
> > > > > do
> > > > > instead is mention this dependency in fields' help text. Modification
> > > > > of
> > > > > the project is not an everyday task so it shouldn't put too much
> > > > > burden
> > > > > on the users and this simple reminder can prevent unexpected breakage
> > > > > and bug reports.
> > > > > 
> > > > 
> > > > Acked-by: Daniel Axtens 
> > > > > Signed-off-by: Veronika Kabatova 
> > > > 
> > > > Stephen, I think this should go to stable/2.1. Thoughts?
> > > 
> > > I don't think it can. I haven't checked yet but I'm pretty sure this
> > > requires a migration and we can't backport those.
> > > 
> > > Rather than doing this, could we not just override the 'save' function
> > > to always save the other field?
> > > 
> > 
> > We'd need to figure out which one of the fields is the changed one and
> > modify the other one to match it, which based on [1] doesn't look as easy
> > as it sounds. "I'll scroll till I find the field I want" and "here's the
> > long content, I'll go to the end and scroll up" would lead to the same but
> > opposite issue instead of fixing the problem, if we decided to hardcode one
> > of the fields as the one to be copied over.
> > 
> > Because of the above, I opted for the simplest help text solution. I'm not
> > opposed to implement a more complicated solution that doesn't require any
> > steps from the user (it was the first solution I looked into) if you think
> > the complications are worth it, or have a better idea how to work around
> > it.
> 
> Ah, good point. I'd like to explore this before we go down any other
> route though. How about this: in the save functions for Submission and
> Patch we search for 'updated_from_child' and 'updated_from_parent'
> kwargs. If these are present, we only update ourselves (to avoid
> recursively calling each other). If they're not present, we update the
> other table (calling 'save' with the paramter) and then ourselves. That
> make sense?
> 

I might be misunderstanding this. If you edit a patch via "patches" admin,
patch's save method is always called. So it doesn't matter which field was
actually changed, the "project" one would be consistently overwritten by
"patch_project" based on this logic.

Can you please clarify if I'm missing something obvious?


Thanks,
Veronika

> Stephen
> 
> PS: I'm still hacking on the tags feature and trying to get performance
> somewhere we'd like it. As things stand, it's looking increasingly
> likely that we're going to need to move a single table inheritance
> model for Patch/CoverLetter

Re: [PATCH RESEND 1/2] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: "Veronika Kabatova" 
> Cc: patchwork@lists.ozlabs.org
> Sent: Wednesday, September 19, 2018 11:38:32 PM
> Subject: Re: [PATCH RESEND 1/2] Rework tagging infrastructure
> 
> On Wed, 2018-09-12 at 04:57 -0400, Veronika Kabatova wrote:
> > 
> > - Original Message -
> > > From: "Stephen Finucane" 
> > > To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> > > Sent: Tuesday, September 11, 2018 7:55:46 PM
> > > Subject: Re: [PATCH RESEND 1/2] Rework tagging infrastructure
> > > 
> > > On Mon, 2018-07-09 at 18:10 +0200, vkaba...@redhat.com wrote:
> > > > From: Veronika Kabatova 
> > > > 
> > > > Solve #113 and #57 GitHub issues, fix up returning tags in the API,
> > > > keep track of tag origin to be able to add tags to comments in the API.
> > > > 
> > > > Use relations Tag-Patch and Tag-CoverLetter to avoid duplication of
> > > > tags for each patch in series, and use `series` attribute of
> > > > SubmissionTag as a notion of a tag which is related to each patch in
> > > > the
> > > > series (because it comes from cover letter or it's comments)
> > > > 
> > > > Signed-off-by: Veronika Kabatova 
> > > 
> > > To start, apologies if I'm repeating myself on any of the comments
> > > below: it's been a long time since I first reviewed this (sorry!).
> > > Also, for the next revision of this, this can definitely be broken up
> > > on purpose :) Given that tags aren't currently shown in the API, I'd
> > > move the API changes into a separate patch. Similarly, docs (though not
> > > release notes) aren't needed and can be broken up and moved into a
> > > separate patch. Now to the review...
> > > 
> > 
> > Sounds good!
> > 
> > /me goes on to comment on everything
> 
> I've been playing with the new revision all evening and, unfortunately,
> have been hitting some pretty dire performance issues. I've outlined
> what I think to be the cause of the issues below, along with some steps
> that might help resolve these issues. Also, many of the comments I have
> on the new revision are follow ups to comments from the earlier
> revision (this one) so I'm replying here to avoid repeating myself :)
> 
> From what I can see, there are two issues here:
> 
>1. The main issue we still have here is that fetching and combining
>   tags for a patch is really expensive. This is because we need to
>   fetch tags for the patch, its comments, the patch's series and this
>   series' comments. We do this in 'Patch.all_tags'.
>2. Related to (1), because Tags are mapped to Submission, we suddenly
>   need to join on the Submission table again, something which Daniel
>   fixed in commit c97dad1cd.
> 
> I realized I've been pushing you in this direction but I didn't forsee
> either of these issues upfront. However, I do have an option that might
> resolve this.
> 
> Currently, SubmissionTag has three foreign keys:
> 
>  * submission
>  * series, NULLABLE
>  * comment, NULLABLE
> 
> As noted above, this means getting all tags for a given patch means
> fetching tags for the patch, its comments, the patch's series and this
> series' comments. From what I can tell, we can minimize the cost of
> only the first of these (using prefetch_related). The rest involve a
> query _per patch_ in the list which is really, really expensive.
> 
> I propose changing SubmissionTag to SeriesTag. This will have three
> foreign keys:
> 
>  * series
>  * patch, NULLABLE
>  * comment, NULLABLE
> 
> These would be set like so:
> 
>  * When you receive a cover letter, you set just the series field
>  * When you receive a patch, you set the series and patch fields
>  * When you receive a cover letter comment, you set the series and
>comment fields
>  * When you receive a patch comment, you set all fields
> 

Interesting idea. I managed to create a working prototype here:
https://github.com/veruu/patchwork/tree/tags_changes

The tests are only partially fixed, but I'll finish that part later when
we agree on the general approach.

> Once we do this, we should be able to something like the below to get
> all tags for a given patch:
> 
>SELECT * FROM patchwork_patch p
>INNER JOIN patchwork_seriestag t ON p.submission_ptr_id = t.patch_id
>UNION
>SELECT * FROM patchwork_patch p
>INNER JOIN patchwork_seriestag t ON p.series_id = t.series_id AND
>t.patch_id IS NULL;
> 
> Apologies for it being in SQL - I sti

Re: [PATCH] REST: Only list checks for the given patch

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: patchwork@lists.ozlabs.org
> Cc: "Michael Ellerman" 
> Sent: Wednesday, September 12, 2018 5:46:56 PM
> Subject: [PATCH] REST: Only list checks for the given patch
> 
> This is either a regression or it never worked. In any case, fix it and
> add a test to ensure it doesn't happen again.
> 

This was driving me nuts for the past year but I thought it's a feature I
don't appreciate and not a bug. Anyways, the patch fixes the problem on my
instance and I don't see any problem with the filter addition either.

Reviewed-by: Veronika Kabatova 

> Signed-off-by: Stephen Finucane 
> Cc: Michael Ellerman 
> Closes: #203
> ---
>  patchwork/api/check.py| 3 ++-
>  patchwork/tests/api/test_check.py | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index 5e461de6..712238eb 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -83,7 +83,8 @@ class CheckMixin(object):
>  filter_class = CheckFilterSet
>  
>  def get_queryset(self):
> -return Check.objects.prefetch_related('patch', 'user')
> +patch_id = self.kwargs['patch_id']
> +return Check.objects.prefetch_related('user').filter(patch=patch_id)
>  
>  
>  class CheckListCreate(CheckMixin, ListCreateAPIView):
> diff --git a/patchwork/tests/api/test_check.py
> b/patchwork/tests/api/test_check.py
> index 6f4aa5de..4c6a934c 100644
> --- a/patchwork/tests/api/test_check.py
> +++ b/patchwork/tests/api/test_check.py
> @@ -54,9 +54,9 @@ class TestCheckAPI(APITestCase):
>  self.user = create_maintainer(project)
>  self.patch = create_patch(project=project)
>  
> -def _create_check(self):
> +def _create_check(self, patch=None):
>  values = {
> -'patch': self.patch,
> +'patch': patch if patch else self.patch,
>  'user': self.user,
>  }
>  return create_check(**values)
> @@ -76,6 +76,7 @@ class TestCheckAPI(APITestCase):
>  self.assertEqual(0, len(resp.data))
>  
>  check_obj = self._create_check()
> +self._create_check(create_patch())  # second, unrelated patch
>  
>  resp = self.client.get(self.api_url())
>  self.assertEqual(status.HTTP_200_OK, resp.status_code)
> --
> 2.17.1
> 
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH RESEND 2/2] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Tuesday, September 11, 2018 7:59:57 PM
> Subject: Re: [PATCH RESEND 2/2] Rework tagging infrastructure
> 
> On Mon, 2018-07-09 at 18:10 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova 
> > 
> > Solve #113 and #57 GitHub issues, fix up returning tags in the API,
> > keep track of tag origin to be able to add tags to comments in the API.
> > 
> > Use relations Tag-Patch and Tag-CoverLetter to avoid duplication of
> > tags for each patch in series, and use `series` attribute of
> > SubmissionTag as a notion of a tag which is related to each patch in the
> > series (because it comes from cover letter or it's comments)
> > 
> > Signed-off-by: Veronika Kabatova 
> > ---
> >  docs/usage/overview.rst|   9 +-
> >  patchwork/management/commands/retag.py |  16 ++-
> >  patchwork/migrations/0027_rework_tagging.py| 146
> >  +
> >  patchwork/tests/api/test_patch.py  |   2 +-
> >  patchwork/tests/test_mboxviews.py  |  18 ++-
> >  patchwork/tests/test_parser.py |  23 ++--
> >  patchwork/tests/test_tags.py   |  64 -
> >  .../notes/tagging-rework-9907e9dc3f835566.yaml |  18 +++
> >  8 files changed, 233 insertions(+), 63 deletions(-)
> >  create mode 100644 patchwork/migrations/0027_rework_tagging.py
> >  create mode 100644 releasenotes/notes/tagging-rework-9907e9dc3f835566.yaml
> > 
> > diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
> > index e84e13d..d3ad2f6 100644
> > --- a/docs/usage/overview.rst
> > +++ b/docs/usage/overview.rst
> > @@ -119,10 +119,11 @@ one delegate can be assigned to a patch.
> >  Tags
> >  
> >  
> > -Tags are specially formatted metadata appended to the foot the body of a
> > patch
> > -or a comment on a patch. Patchwork extracts these tags at parse time and
> > -associates them with the patch. You add extra tags to an email by replying
> > to
> > -the email. The following tags are available on a standard Patchwork
> > install:
> > +Tags are specially formatted metadata appended to the foot the body of a
> > patch,
> > +cover letter or a comment related to them. Patchwork extracts these tags
> > at
> > +parse time and associates them with patches. You add extra tags to an
> > email by
> > +replying to the email. The following tags are available on a standard
> > Patchwork
> > +install:
> 
> This can be a separate patch.
> 

Ack

> >  ``Acked-by:``
> >For example::
> > diff --git a/patchwork/management/commands/retag.py
> > b/patchwork/management/commands/retag.py
> > index 8617ff4..150dbf1 100644
> > --- a/patchwork/management/commands/retag.py
> > +++ b/patchwork/management/commands/retag.py
> > @@ -19,11 +19,13 @@
> >  
> >  from django.core.management.base import BaseCommand
> >  
> > +from patchwork.models import Cover
> >  from patchwork.models import Patch
> > +from patchwork.models import SeriesPatch
> >  
> >  
> >  class Command(BaseCommand):
> > -help = 'Update the tag (Ack/Review/Test) counts on existing patches'
> > +help = 'Update tags on existing patches'
> 
> s/patches/submissions/ ?

Thanks!

> 
> >  args = '[...]'
> >  
> >  def handle(self, *args, **options):
> > @@ -37,7 +39,17 @@ class Command(BaseCommand):
> >  count = query.count()
> >  
> >  for i, patch in enumerate(query.iterator()):
> > -patch.refresh_tag_counts()
> > +patch.refresh_tags()
> > +for comment in patch.comments.all():
> > +comment.refresh_tags()
> > +
> > +series_patches = SeriesPatch.objects.filter(patch_id=patch.id)
> > +for series_patch in series_patches:
> > +cover = series_patch.series.cover_letter
> > +cover.refresh_tags()
> > +for comment in cover.comments.all():
> > +comment.refresh_tags()
> > +
> >  if (i % 10) == 0:
> >  self.stdout.write('%06d/%06d\r' % (i, count), ending='')
> >  self.stdout.flush()
> > diff --git a/patchwork/migrations/0027_rework_tagging.py
> > b/patchwork/migrations/0027_rework_tagging.py
> > new file mode 100644
> > index 000..009546f
> > -

Re: [PATCH v2 2/2] docs: Note new requirement to include a SPDX line

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: patchwork@lists.ozlabs.org
> Sent: Monday, September 17, 2018 7:19:45 PM
> Subject: Re: [PATCH v2 2/2] docs: Note new requirement to include a SPDX line
> 
> On Mon, 2018-09-17 at 18:17 +0100, Stephen Finucane wrote:
> > Add some wording around the requirement to include this line instead
> > of
> > the license header. Also note the requirement that all code be
> > GPLv2-licensed and add a CONTRIBUTING document, which GitHub likes.
> > 
> > Signed-off-by: Stephen Finucane 
> > Cc: Daniel Axtens 
> 
> Looks like patch 1/2 (or the earlier v1 rendition) didn't make it to
> the list. It's basically the following diff for all files:
> 
>-# This file is part of the Patchwork package.
>-#
>-# Patchwork is free software; you can redistribute it and/or modify
>-# it under the terms of the GNU General Public License as published by
>-# the Free Software Foundation; either version 2 of the License, or
>-# (at your option) any later version.
>-#
>-# Patchwork is distributed in the hope that it will be useful,
>-# but WITHOUT ANY WARRANTY; without even the implied warranty of
>-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>-# GNU General Public License for more details.
>-#
>-# You should have received a copy of the GNU General Public License
>-# along with Patchwork; if not, write to the Free Software
>-# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
>USA
>+# SPDX-License-Identifier: GPL-2.0
> 

This seems to be the same problem why my first tagging patch didn't make it
to the list - the email is too large and doesn't fit the mailing list
thresholds. Given how many files contain the preamble, the patch would need
to be split a lot to get it to the list.

Veronika

> Stephen
> 
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH v3 2/2] docs: Note new requirement to include a SPDX line

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: patchwork@lists.ozlabs.org
> Cc: "Stephen Finucane" , "Daniel Axtens" 
> , "Veronika Kabatova"
> 
> Sent: Tuesday, September 18, 2018 5:46:16 PM
> Subject: [PATCH v3 2/2] docs: Note new requirement to include a SPDX line
> 
> Add some wording around the requirement to include this line instead of
> the license header. Also note the requirement that all code be licensed
> using the 'GPL-2.0-or-later' license and add a CONTRIBUTING document,
> which GitHub likes.
> 
> Signed-off-by: Stephen Finucane 
> Cc: Daniel Axtens 
> Cc: Veronika Kabatova 
> ---
> v3:
> - Update to reflect use of 'GPL-2.0-or-later', rather than 'GPL-2.0'
> ---

Thanks for looking into it so quick after the comment!
To both updated patches (one of which fails to show up on the list):

Reviewed-by: Veronika Kabatova 

>  CONTRIBUTING.rst  |  6 ++
>  docs/development/contributing.rst | 21 ++---
>  2 files changed, 24 insertions(+), 3 deletions(-)
>  create mode 100644 CONTRIBUTING.rst
> 
> diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> new file mode 100644
> index ..131e2dcb
> --- /dev/null
> +++ b/CONTRIBUTING.rst
> @@ -0,0 +1,6 @@
> +Contributing
> +
> +
> +For guidelines on contributing, refer to the `contributors documentation`__.
> +
> +__ https://patchwork.readthedocs.io/en/latest/development/contributing/
> diff --git a/docs/development/contributing.rst
> b/docs/development/contributing.rst
> index 7e2a72cf..5089bba8 100644
> --- a/docs/development/contributing.rst
> +++ b/docs/development/contributing.rst
> @@ -4,13 +4,25 @@ Contributing
>  Coding Standards
>  
>  
> -**Follow PEP8**. All code is currently PEP8 compliant and it should stay
> this
> -way.
> +**Follow PEP8**. All code is currently `PEP 8`_ compliant and it should stay
> +this way.
> +
> +All code must be licensed using `GPL v2.0 or later`_ and must have a `SPDX
> +License Identifier`_ stating this. A copyright line should be included on
> new
> +files and may be added for significant changes to existing files.
> +
> +.. code-block:: python
> +
> +   # Patchwork - automated patch tracking system
> +   # Copyright (C) 2000 Jane Doe 
> +   # Copyright (C) 2001 Joe Bloggs 
> +   #
> +   # SPDX-License-Identifier: GPL-2.0-or-later
>  
>  Changes that fix semantic issues will be generally be happily received, but
>  please keep such changes separate from functional changes.
>  
> -`pep8` targets are provided via tox. Refer to the :ref:`testing` section
> +``pep8`` targets are provided via tox. Refer to the :ref:`testing` section
>  below for more information on usage of this tool.
>  
>  .. _testing:
> @@ -148,6 +160,9 @@ announcements.
>  Further information about the Patchwork mailing list is available can be
>  found on
>  `lists.ozlabs.org`_.
>  
> +.. _PEP 8: https://pep8.org/
> +.. _GPL v2.0 or later: https://spdx.org/licenses/GPL-2.0-or-later.html
> +.. _SPDX License Identifier: https://spdx.org/using-spdx-license-identifier
>  .. _tox: https://tox.readthedocs.io/en/latest/
>  .. _reno: https://docs.openstack.org/developer/reno/
>  .. _QEMU guidelines: http://wiki.qemu.org/Contribute/SubmitAPatch
> --
> 2.17.1
> 
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH RESEND 1/2] Rework tagging infrastructure

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Tuesday, September 11, 2018 7:55:46 PM
> Subject: Re: [PATCH RESEND 1/2] Rework tagging infrastructure
> 
> On Mon, 2018-07-09 at 18:10 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova 
> > 
> > Solve #113 and #57 GitHub issues, fix up returning tags in the API,
> > keep track of tag origin to be able to add tags to comments in the API.
> > 
> > Use relations Tag-Patch and Tag-CoverLetter to avoid duplication of
> > tags for each patch in series, and use `series` attribute of
> > SubmissionTag as a notion of a tag which is related to each patch in the
> > series (because it comes from cover letter or it's comments)
> > 
> > Signed-off-by: Veronika Kabatova 
> 
> To start, apologies if I'm repeating myself on any of the comments
> below: it's been a long time since I first reviewed this (sorry!).
> Also, for the next revision of this, this can definitely be broken up
> on purpose :) Given that tags aren't currently shown in the API, I'd
> move the API changes into a separate patch. Similarly, docs (though not
> release notes) aren't needed and can be broken up and moved into a
> separate patch. Now to the review...
> 

Sounds good!

/me goes on to comment on everything

> > ---
> >  patchwork/api/comment.py|  18 -
> >  patchwork/api/cover.py  |  19 -
> >  patchwork/api/filters.py|  68 +++-
> >  patchwork/api/patch.py  |  18 -
> >  patchwork/models.py | 173
> >  
> >  patchwork/templatetags/patch.py |   3 +-
> >  patchwork/views/__init__.py |   3 -
> >  patchwork/views/utils.py|  10 ++-
> >  8 files changed, 191 insertions(+), 121 deletions(-)
> > 
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > index 5a5adb1..e03207e 100644
> > --- a/patchwork/api/comment.py
> > +++ b/patchwork/api/comment.py
> > @@ -26,6 +26,7 @@ from patchwork.api.base import
> > BaseHyperlinkedModelSerializer
> >  from patchwork.api.base import PatchworkPermission
> >  from patchwork.api.embedded import PersonSerializer
> >  from patchwork.models import Comment
> > +from patchwork.models import SubmissionTag
> >  
> >  
> >  class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > @@ -34,6 +35,7 @@ class
> > CommentListSerializer(BaseHyperlinkedModelSerializer):
> >  subject = SerializerMethodField()
> >  headers = SerializerMethodField()
> >  submitter = PersonSerializer(read_only=True)
> > +tags = SerializerMethodField()
> >  
> >  def get_web_url(self, instance):
> >  request = self.context.get('request')
> > @@ -43,6 +45,20 @@ class
> > CommentListSerializer(BaseHyperlinkedModelSerializer):
> >  return email.parser.Parser().parsestr(comment.headers,
> >True).get('Subject', '')
> >  
> > +def get_tags(self, instance):
> > +if not instance.submission.project.use_tags:
> > +return {}
> > +
> > +tags = SubmissionTag.objects.filter(
> > +comment=instance
> > +).values_list('tag__name', 'value')
> > +
> > +result = {}
> > +for name, value in tags:
> > +result.setdefault(name, []).append(value)
> > +
> > +return result
> 
> This smells of something that should be placed in models.py. Is there
> any reason we _can't_ do it there, e.g. via a property?
> 

I guess we could (it's been a while since I thought about this code too,
but right now I can't think of any reason why not).

> > +
> >  def get_headers(self, comment):
> >  headers = {}
> >  
> > @@ -60,7 +76,7 @@ class
> > CommentListSerializer(BaseHyperlinkedModelSerializer):
> >  class Meta:
> >  model = Comment
> >  fields = ('id', 'web_url', 'msgid', 'date', 'subject',
> >  'submitter',
> > -  'content', 'headers')
> > +  'content', 'headers', 'tags')
> >  read_only_fields = fields
> >  versioned_fields = {
> >  '1.1': ('web_url', ),
> 
> You definitely need changes to 'get_queryset' here. I can help you work
> on these but tl;dr: look at what we do for 'check_set' in
> 'patchwork/api/patch.py'.

Did I mention I'm not a DB master? Any help with performance is
appreciated :)

> 
> &g

Re: [PATCH v2 2/2] docs: Note new requirement to include a SPDX line

[Veronika Kabatova]

- Original Message -
> From: "Stephen Finucane" 
> To: "Veronika Kabatova" 
> Cc: patchwork@lists.ozlabs.org
> Sent: Monday, September 17, 2018 11:24:11 PM
> Subject: Re: [PATCH v2 2/2] docs: Note new requirement to include a SPDX line
> 
> On Mon, 2018-09-17 at 13:34 -0400, Veronika Kabatova wrote:
> > 
> > - Original Message -
> > > From: "Stephen Finucane" 
> > > To: patchwork@lists.ozlabs.org
> > > Sent: Monday, September 17, 2018 7:19:45 PM
> > > Subject: Re: [PATCH v2 2/2] docs: Note new requirement to include a SPDX
> > > line
> > > 
> > > On Mon, 2018-09-17 at 18:17 +0100, Stephen Finucane wrote:
> > > > Add some wording around the requirement to include this line instead
> > > > of
> > > > the license header. Also note the requirement that all code be
> > > > GPLv2-licensed and add a CONTRIBUTING document, which GitHub likes.
> > > > 
> > > > Signed-off-by: Stephen Finucane 
> > > > Cc: Daniel Axtens 
> > > 
> > > Looks like patch 1/2 (or the earlier v1 rendition) didn't make it to
> > > the list. It's basically the following diff for all files:
> > > 
> > >-# This file is part of the Patchwork package.
> > >-#
> > >-# Patchwork is free software; you can redistribute it and/or modify
> > >-# it under the terms of the GNU General Public License as published
> > >by
> > >-# the Free Software Foundation; either version 2 of the License, or
> > >-# (at your option) any later version.
> > >-#
> > >-# Patchwork is distributed in the hope that it will be useful,
> > >-# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > >-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > >-# GNU General Public License for more details.
> > >-#
> > >-# You should have received a copy of the GNU General Public License
> > >-# along with Patchwork; if not, write to the Free Software
> > >-# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > >02111-1307
> > >USA
> > >+# SPDX-License-Identifier: GPL-2.0
> > > 
> > 
> > This seems to be the same problem why my first tagging patch didn't make it
> > to the list - the email is too large and doesn't fit the mailing list
> > thresholds. Given how many files contain the preamble, the patch would need
> > to be split a lot to get it to the list.
> 
> Yup, that's what I'm thinking. It's trivial though so unless anyone
> else wants to review this though, I'll just wait for Daniel to take a
> look and then merge it.
> 

I actually do have a comment. The text of the license is GPL-2.0+, while the
new identifier and documentation changes imply GPL-2.0-only. The SPDX page
contains both licenses and the differences between them (see the "Standard
License Header" sections [1] [2] and that the GPL-2.0+ is the one used with
PW).

So, if the changes should be equivalent, GPL-2.0+ identifier (and appropriate
documentation wording) should be used instead. If part of the change is to
limit the license to GPL-2.0-only (which is fine by me), then the commit
message should say so, since it's easy to miss the difference.


Please let me know if my understanding about the topic is correct or if I
missed any discussions about the license change.

Veronika


[1] https://spdx.org/licenses/GPL-2.0-only.html
[2] https://spdx.org/licenses/GPL-2.0-or-later.html


> Stephen
> 
> > Veronika
> > 
> > > Stephen
> > > 
> > > ___
> > > Patchwork mailing list
> > > Patchwork@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/patchwork
> > > 
> 
> 
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: Debugging dropped mails

[Veronika Kabatova]

- Original Message -
> From: "Petr Vorel" 
> To: "Konstantin Ryabitsev" , 
> patchwork@lists.ozlabs.org
> Cc: "Jeremy Kerr" 
> Sent: Wednesday, April 10, 2019 11:20:00 AM
> Subject: Re: Debugging dropped mails
> 
> Hi Konstantin,
> 
> > On Tue, Apr 09, 2019 at 08:20:19PM +0200, Petr Vorel wrote:
> > > any idea how can I debug dropped mails in patchwork instances?
> > > Few weeks ago I started to be unable to get patches to
> > > patchwork.ozlabs.org and
> > > patchwork.kernel.org. Sometimes only gmail.com address is having
> > > troubles,
> > > sometimes both my email addresses (I cc my gmail address).
> 
> > > Of course, I'm registered to mailing lists, I see my mail in mailing list
> > > archives. Problems are on some kernel mailing lists, but on others as
> > > well
> > > (buildroot, LTP).
> 
> > Can you give me a link to one of the messages that show up in mailing
> > list archives but aren't on patchwork.kernel.org?
> Sure. It's this patch, send from both my mails to linux-kbuild: suse [1],
> gmail [2].
> It's not in patchwork [3]
> 

Hi,

checking the emails you linked and Patchwork's parser method [0], it's not
that the emails are lost but rather the diffs are not recognized and thus
dropped (verified by testing it with internal Patchwork instance I maintain).

The "old mode" line doesn't match any of [1] and thus it looks to Patchwork as
if the patch was empty.

I'm inclined to say that Patchwork parsing should support all of the extended
header lines described in git docs [2] and that the current behavior is a bug.



Hope this helps,
Veronika


[0] 
https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L717
[1] 
https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L785
[2] https://git-scm.com/docs/git-diff#_generating_patches_with_p

> Kind regards,
> Petr
> 
> [1] https://marc.info/?l=linux-kbuild=155467856208923=2
> [2] https://marc.info/?l=linux-kbuild=155479414711694=2
> [3]
> https://patchwork.kernel.org/project/linux-kbuild/list/?series==172899=*===
> ___
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: Debugging dropped mails

[Veronika Kabatova]

- Original Message -
> From: "Petr Vorel" 
> To: "Veronika Kabatova" 
> Cc: "Konstantin Ryabitsev" , 
> patchwork@lists.ozlabs.org, "Jeremy Kerr"
> 
> Sent: Wednesday, April 10, 2019 3:50:25 PM
> Subject: Re: Debugging dropped mails
> 
> Hi Veronika,
> 
> > checking the emails you linked and Patchwork's parser method [0], it's not
> > that the emails are lost but rather the diffs are not recognized and thus
> > dropped (verified by testing it with internal Patchwork instance I
> > maintain).
> 
> > The "old mode" line doesn't match any of [1] and thus it looks to Patchwork
> > as
> > if the patch was empty.
> 
> > I'm inclined to say that Patchwork parsing should support all of the
> > extended
> > header lines described in git docs [2] and that the current behavior is a
> > bug.
> Thanks for debugging! I agree that this is a bug and should be fixed.
> And this probably explain my problems with kernel.org instance. But it does
> not
> explain my problems with ozlabs instance start with index:
> Patches not taken (from my gmail address) to LTP: [3], [4]. There are not in
> patchwork [5]. Last ok from gmail was this one [6], taken here [7].
> I also have problem to send mails to Buildroot [8] (haven't tested with suse
> address).
> I guess I had problems with LTP for some time, Buildroot started quite
> recently.
> 
> Any idea what can be a problem?
> 

My 2.1 instance eats the linked patch just fine. The Ozlabs instance is running
version 2.0.2 and there were a few parser fixes and changes introduced in later
releases. Maybe the problem was fixed there?

Unfortunately I can't help you more as I don't see an obvious issue with the
patch nor do I have access to the Ozlabs instance.


Veronika

> > Hope this helps,
> > Veronika
> 
> 
> > [0]
> > https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L717
> > [1]
> > https://github.com/getpatchwork/patchwork/blob/master/patchwork/parser.py#L785
> > [2] https://git-scm.com/docs/git-diff#_generating_patches_with_p
> 
> Kind regards,
> Petr
> 
> [3] http://lists.linux.it/pipermail/ltp/2019-March/011448.html
> [4] http://lists.linux.it/pipermail/ltp/2019-March/011489.html
> [5] https://patchwork.ozlabs.org/project/ltp/list/?submitter=70792=*
> [6] http://lists.linux.it/pipermail/ltp/2019-March/011450.html
> [7] https://patchwork.ozlabs.org/patch/1060538/
> [8] http://lists.busybox.net/pipermail/buildroot/2019-March/245679.html
> 
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork

Re: [PATCH] Only mark series as completed after cover letter has arrived

[Veronika Kabatova]

- 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
> > +
> > +
>