Re: [PATCH RESEND 1/2] Rework tagging infrastructure
- 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 still haven't figured out how to do > this in Django's ORM. However, in brief that means: > >Join the two tables where the tag belongs to either that patch or >the patches series. > > If we wanted to get tags for a cover letter, we could then do this: > >SELECT * patchwork_coverletter c >INNER JOIN patchwork_seriestag t ON c.series_id = c.series_id AND >c.patch_id IS NULL; > > I _think_ that would work, but the question is figuring out how to > convert that to something the ORM will like. I'll work on this myself > over the
Re: [PATCH 00/11] Add labels support
On Sat, Sep 22, 2018 at 12:53:46AM +1000, Daniel Axtens wrote: > Don Zickus writes: > > > On Tue, Sep 11, 2018 at 12:21:26PM -0600, Stephen Finucane wrote: > >> > Personally I would *really* like labels to land. They unlock a lot of > >> > potential improvements to workflow for maintainers, eg. automated > >> > tagging, tagging based on test results etc. As well as finer grained > >> > reporting of status to submitters, eg. instead of "new" -> "under > >> > review" -> "accepted", I can mark a patch as "under review" and > >> > "applied-to-some-branch", then "under review" and > >> > "in-testing" etc. etc. > >> > > >> > Would it simplify (or not?) the implementation if states became a > >> > special case of labels? > >> > >> Oh, that's a really good idea, actually. The model I had been following > >> for the "Remove State" series was to add two new fields to the "Patch" > >> model: 'is_open' and 'resolution', the former being a boolean > >> open/closed value and the later being an enum of the default state > >> fields. I'd be happy to move this entire thing to labels though. Does > >> anyone else (Daniel, Don) have thoughts on this? > > > > I would like to see a little more detail on the format of this would look > > like. Sorta like how you describe what the data input looks like for > > 'checks'. > > > > This probably works for us, just want to be sure. > > > > I've thought about this a bit more. Here is a brain dump. > > Lets suppose we're redesigning the entire idea of the metadata about a > patch that Patchwork tracks. States, Checks, Tags, whatever. > > At one end of the spectrum of options is that we just allow you to > attach arbitrary strings to patches, and give you a UI to filter on > them. So some examples: > > Patch | Tags > |-- > Fix foo -> bar | state:new, check:checkpatch:passed:, > check:tox:failed:, > | "reviewer:Jane Smith", for-stable > |-- > [RFC] framisets | rfc, "state:Not Applicable", applied-tech-preview > |-- > [v2] framisets | v2, "tag:Acked-by: Daniel Axtens ", ... > --- > > Then everything just becomes a matter of people standardising on string > formats and making them meaningful to the UI. We sorta did this internally, except a little simpler all tags were just key : value acked-by: build: test: superseded-by: bugzilla: but we kept 'state' as is. and all tag ids mapped to either a comment or patch and searching wasn't a big performance issue, I think. Our tooling had to be consistent on the key names. > > Another step along the spectrum would be to do that for some things, but > keep other bits as separate tables/models. The strongest argument would > be to say that Checks are good as they are and we should exclude them > from being labels. The next step would be to say that for efficient > filtering, States or some close analogue need to be separate > fields. Stephen, I think this is closest to your approach where you add > a field to determine whether or not a patch should be shown by default > in the UI (is_open). > > Going all the way to the other end of the spectrum, everthing should get > its own DB field and we should support labels by just adding an array of > strings to the existing patch/series models. And we should clean up tag > support. > > Each approach has its own strengths and weaknesses. In evaluating them, > I'm particularly cognisant of: > > - not breaking people's workflows, so you have to be able to do the >same things in the same ways and move towards new features at your >own pace. We should avoid foisting backwards-incompatible changes on >people. > > - performance. > > The first approach is I think conceptually quite nice. We can just > define particular types of labels that get treated specially in the UI > (state:, check:, tag:, etc.), migrate things, and people can go for > their lives. However, it seems like it might be an implementation > nightmare. For example, how you do keep track of every State used in a > project if they're strings in a list of strings? And I don't know how > you'd implement it in a performant way. Either you store the labels as > lists of strings and have to do string searches on every patch in your > project just to list them, or you have some join table that's probably > even worse. (Or there's DB magic here that I am not aware of.) > > I also have a soft spot for the final approach of doing the simplest > possible thing and just adding a list of strings that people can > additionally search on. > > However, I do think it would be a shame to pass up a chance to simplify > things. I think (or at least I currently think) that tags should become > labels. I would accept a reasonable
[PATCH v3 4/5] tests: Remove 'create_series_patch'
The 'SeriesPatch' object was recently removed, but the 'create_series_patch' was retained in order to minimize the changes necessary. This can now be removed and the logic moved to the 'create_patch' and 'create_cover' functions instead. Signed-off-by: Stephen Finucane --- patchwork/tests/api/test_series.py | 12 +++ patchwork/tests/test_events.py | 34 ++-- patchwork/tests/test_mboxviews.py | 28 patchwork/tests/utils.py | 51 -- 4 files changed, 65 insertions(+), 60 deletions(-) diff --git a/patchwork/tests/api/test_series.py b/patchwork/tests/api/test_series.py index 4d576fc0..21ce2547 100644 --- a/patchwork/tests/api/test_series.py +++ b/patchwork/tests/api/test_series.py @@ -10,10 +10,10 @@ from django.urls import reverse from patchwork.tests.utils import create_cover from patchwork.tests.utils import create_maintainer -from patchwork.tests.utils import create_project +from patchwork.tests.utils import create_patch from patchwork.tests.utils import create_person +from patchwork.tests.utils import create_project from patchwork.tests.utils import create_series -from patchwork.tests.utils import create_series_patch from patchwork.tests.utils import create_user if settings.ENABLE_REST_API: @@ -69,10 +69,9 @@ class TestSeriesAPI(APITestCase): project_obj = create_project(linkname='myproject') person_obj = create_person(email='t...@example.com') -cover_obj = create_cover() series_obj = create_series(project=project_obj, submitter=person_obj) -series_obj.add_cover_letter(cover_obj) -create_series_patch(series=series_obj) +create_cover(series=series_obj) +create_patch(series=series_obj) # anonymous users resp = self.client.get(self.api_url()) @@ -118,9 +117,8 @@ class TestSeriesAPI(APITestCase): def test_detail(self): """Validate we can get a specific series.""" -cover = create_cover() series = create_series() -series.add_cover_letter(cover) +create_cover(series=series) resp = self.client.get(self.api_url(series.id)) self.assertEqual(status.HTTP_200_OK, resp.status_code) diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py index bd4f9be1..7d03d65d 100644 --- a/patchwork/tests/test_events.py +++ b/patchwork/tests/test_events.py @@ -44,45 +44,46 @@ class PatchCreateTest(_BaseTestCase): def test_patch_dependencies_present_series(self): """Patch dependencies already exist.""" -series_patch = utils.create_series_patch() +series = utils.create_series() +patch = utils.create_patch(series=series) # This should raise both the CATEGORY_PATCH_CREATED and # CATEGORY_PATCH_COMPLETED events -events = _get_events(patch=series_patch.patch) +events = _get_events(patch=patch) self.assertEqual(events.count(), 2) self.assertEqual(events[0].category, Event.CATEGORY_PATCH_CREATED) -self.assertEqual(events[0].project, series_patch.patch.project) +self.assertEqual(events[0].project, patch.project) self.assertEqual(events[1].category, Event.CATEGORY_PATCH_COMPLETED) -self.assertEqual(events[1].project, series_patch.patch.project) +self.assertEqual(events[1].project, patch.project) self.assertEventFields(events[0]) self.assertEventFields(events[1]) # This shouldn't be affected by another update to the patch -series_patch.patch.commit_ref = 'aac76f0b0f8dd657ff07bb' -series_patch.patch.save() +patch.commit_ref = 'aac76f0b0f8dd657ff07bb32df369705696d4831' +patch.save() -events = _get_events(patch=series_patch.patch) +events = _get_events(patch=patch) self.assertEqual(events.count(), 2) def test_patch_dependencies_out_of_order(self): series = utils.create_series() -series_patch_3 = utils.create_series_patch(series=series, number=3) -series_patch_2 = utils.create_series_patch(series=series, number=2) +patch_3 = utils.create_patch(series=series, number=3) +patch_2 = utils.create_patch(series=series, number=2) # This should only raise the CATEGORY_PATCH_CREATED event for # both patches as they are both missing dependencies -for series_patch in [series_patch_2, series_patch_3]: -events = _get_events(patch=series_patch.patch) +for patch in [patch_2, patch_3]: +events = _get_events(patch=patch) self.assertEqual(events.count(), 1) self.assertEqual(events[0].category, Event.CATEGORY_PATCH_CREATED) self.assertEventFields(events[0]) -series_patch_1 = utils.create_series_patch(series=series, number=1) +patch_1 = utils.create_patch(series=series, number=1) # We should now see the
[PATCH v3 5/5] views: Add support for boolean 'series' parameters
Previously, we allowed users to download patch mboxes with dependencies
included using a 'series' parameter. This accepted either a numeric ID,
corresponding to the ID of the patch series that dependencies should be
included from, or a wildcard value ('*').
/patch/{patchID}/mbox/?series=123
/patch/{patchID}/mbox/?series=*
With switch to a 1:N series-patch relationship, this is clearly
unnecessary now but must be retained to avoid breaking users. However,
that doesn't mean we can't things a little clearer. Add support for
boolean parameters, which make more sense for this kind of relationship:
/patch/{patchID}/mbox/?series=true
/patch/{patchID}/mbox/?series=1
/patch/{patchID}/mbox/?series=false
/patch/{patchID}/mbox/?series=0
Signed-off-by: Stephen Finucane
---
TODO: I'm not sure if I should do this or introduce a new parameter
(maybe 'dependencies'?). Thoughts?
---
patchwork/tests/test_mboxviews.py | 17 +
patchwork/views/patch.py | 2 +-
patchwork/views/utils.py | 2 +-
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/patchwork/tests/test_mboxviews.py
b/patchwork/tests/test_mboxviews.py
index 5c29226b..8f6a51f2 100644
--- a/patchwork/tests/test_mboxviews.py
+++ b/patchwork/tests/test_mboxviews.py
@@ -223,6 +223,23 @@ class MboxSeriesDependencies(TestCase):
self.assertContains(response, patch_a.content)
self.assertContains(response, patch_b.content)
+def test_patch_with_boolean_series(self):
+_, patch_a, patch_b = self._create_patches()
+
+for value in ('true', '1'):
+response = self.client.get('%s?series=%s' % (
+reverse('patch-mbox', args=[patch_b.id]), value))
+
+self.assertContains(response, patch_a.content)
+self.assertContains(response, patch_b.content)
+
+for value in ('false', '0'):
+response = self.client.get('%s?series=%s' % (
+reverse('patch-mbox', args=[patch_b.id]), value))
+
+self.assertNotContains(response, patch_a.content)
+self.assertContains(response, patch_b.content)
+
def test_patch_with_invalid_series(self):
series, patch_a, patch_b = self._create_patches()
diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
index 277b2816..446a0c4b 100644
--- a/patchwork/views/patch.py
+++ b/patchwork/views/patch.py
@@ -130,7 +130,7 @@ def patch_mbox(request, patch_id):
series_id = request.GET.get('series')
response = HttpResponse(content_type='text/plain')
-if series_id:
+if series_id and series_id.lower() not in ('false', '0'):
if not patch.series:
raise Http404('Patch does not have an associated series. This is '
'because the patch was processed with an older '
diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
index 3c5d2982..d6f54ea6 100644
--- a/patchwork/views/utils.py
+++ b/patchwork/views/utils.py
@@ -127,7 +127,7 @@ def series_patch_to_mbox(patch, series_id):
Returns:
A string for the mbox file.
"""
-if series_id != '*':
+if series_id.lower() not in ('*', 'true', '1'):
try:
series_id = int(series_id)
except ValueError:
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
[PATCH v3 2/5] tests: Hardcode expected values
Compare against known values to ensure bugs introduced in the function
are caught.
Signed-off-by: Stephen Finucane
Reviewed-by: Daniel Axtens
---
patchwork/tests/test_series.py | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
index bb44e39d..ff3412e6 100644
--- a/patchwork/tests/test_series.py
+++ b/patchwork/tests/test_series.py
@@ -623,10 +623,6 @@ class SeriesNameTestCase(TestCase):
def _parse_mail(self, mail):
return parser.parse_mail(mail, self.project.listid)
-@staticmethod
-def _format_name(cover):
-return models.Series._format_name(cover)
-
def test_cover_letter(self):
"""Cover letter name set as series name.
@@ -642,7 +638,7 @@ class SeriesNameTestCase(TestCase):
mbox = self._get_mbox('base-cover-letter.mbox')
cover = self._parse_mail(mbox[0])
-cover_name = self._format_name(cover)
+cover_name = 'A sample series'
self.assertEqual(cover.series.first().name, cover_name)
self._parse_mail(mbox[1])
@@ -697,7 +693,7 @@ class SeriesNameTestCase(TestCase):
self.assertEqual(patch.series.first().name, patch.name)
cover = self._parse_mail(mbox[2])
-self.assertEqual(cover.series.first().name, self._format_name(cover))
+self.assertEqual(cover.series.first().name, 'A sample series')
mbox.close()
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
[PATCH v3 0/5] Convert Series-Patch relationship to 1:N
This is mostly a tech debt reduction exercise. As noted in the main patch, the M:N relationship between series and patches was, in hindsight, a design decision made for the wrong reasons. It's one of the things holding us back on the improved tagging series and it makes a lot of things more difficult to do than they should be. Time to fix it, IMO. Changes since v2: - Update SQL 'grant-all' scripts for model changes Stephen Finucane (5): tests: Add more tests for series-ified mbox views tests: Hardcode expected values models: Convert Series-Patch relationship to 1:N tests: Remove 'create_series_patch' views: Add support for boolean 'series' parameters lib/sql/grant-all.mysql.sql | 2 - lib/sql/grant-all.postgres.sql| 4 -- patchwork/admin.py| 2 +- patchwork/api/cover.py| 19 +++-- patchwork/api/patch.py| 20 -- .../0031_add_patch_series_fields.py | 32 + ...migrate_data_from_series_patch_to_patch.py | 35 ++ .../0033_remove_patch_series_model.py | 58 patchwork/models.py | 60 ++-- patchwork/parser.py | 6 +- patchwork/signals.py | 40 +-- .../templates/patchwork/download_buttons.html | 17 + patchwork/templates/patchwork/patch-list.html | 12 ++-- patchwork/templates/patchwork/submission.html | 26 ++- patchwork/tests/api/test_series.py| 12 ++-- patchwork/tests/test_detail.py| 16 - patchwork/tests/test_events.py| 39 +++ patchwork/tests/test_mboxviews.py | 69 +-- patchwork/tests/test_series.py| 38 +- patchwork/tests/utils.py | 45 +++- patchwork/views/cover.py | 1 - patchwork/views/patch.py | 5 +- patchwork/views/utils.py | 17 ++--- 23 files changed, 360 insertions(+), 215 deletions(-) create mode 100644 patchwork/migrations/0031_add_patch_series_fields.py create mode 100644 patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py create mode 100644 patchwork/migrations/0033_remove_patch_series_model.py -- 2.17.1 ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
[PATCH v3 1/5] tests: Add more tests for series-ified mbox views
Cover some testing gaps identified during the migration from a M:N to a
1:N series-patch relationship, namely:
- Downloading a patch's mbox with dependencies using a numerical series
ID
- Downloading a series' mbox
Signed-off-by: Stephen Finucane
Reviewed-by: Daniel Axtens
---
patchwork/tests/test_mboxviews.py | 40 ++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/patchwork/tests/test_mboxviews.py
b/patchwork/tests/test_mboxviews.py
index eadfd81c..9de9c762 100644
--- a/patchwork/tests/test_mboxviews.py
+++ b/patchwork/tests/test_mboxviews.py
@@ -17,6 +17,7 @@ from patchwork.tests.utils import create_comment
from patchwork.tests.utils import create_patch
from patchwork.tests.utils import create_project
from patchwork.tests.utils import create_person
+from patchwork.tests.utils import create_series
from patchwork.tests.utils import create_series_patch
from patchwork.tests.utils import create_user
@@ -197,16 +198,40 @@ class MboxCommentPostcriptUnchangedTest(TestCase):
class MboxSeriesDependencies(TestCase):
-def test_patch_with_dependencies(self):
+@staticmethod
+def _create_patches():
patch_a = create_series_patch()
patch_b = create_series_patch(series=patch_a.series)
+return patch_a.series, patch_a, patch_b
+
+def test_patch_with_wildcard_series(self):
+_, patch_a, patch_b = self._create_patches()
+
response = self.client.get('%s?series=*' % reverse(
'patch-mbox', args=[patch_b.patch.id]))
self.assertContains(response, patch_a.patch.content)
self.assertContains(response, patch_b.patch.content)
+def test_patch_with_numeric_series(self):
+series, patch_a, patch_b = self._create_patches()
+
+response = self.client.get('%s?series=%d' % (
+reverse('patch-mbox', args=[patch_b.patch.id]), series.id))
+
+self.assertContains(response, patch_a.patch.content)
+self.assertContains(response, patch_b.patch.content)
+
+def test_patch_with_invalid_series(self):
+series, patch_a, patch_b = self._create_patches()
+
+for value in ('foo', str(series.id + 1)):
+response = self.client.get('%s?series=%s' % (
+reverse('patch-mbox', args=[patch_b.patch.id]), value))
+
+self.assertEqual(response.status_code, 404)
+
def test_legacy_patch(self):
"""Validate a patch with non-existent dependencies raises a 404."""
# we're explicitly creating a patch without a series
@@ -216,3 +241,16 @@ class MboxSeriesDependencies(TestCase):
'patch-mbox', args=[patch.id]))
self.assertEqual(response.status_code, 404)
+
+
+class MboxSeries(TestCase):
+
+def test_series(self):
+series = create_series()
+patch_a = create_series_patch(series=series)
+patch_b = create_series_patch(series=series)
+
+response = self.client.get(reverse('series-mbox', args=[series.id]))
+
+self.assertContains(response, patch_a.patch.content)
+self.assertContains(response, patch_b.patch.content)
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 1/2] parsearchive: Fix logging
On Sat, 2018-09-22 at 01:18 +1000, Daniel Axtens wrote: > Stephen Finucane writes: > > > On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: > > > Hi Stephen, > > > > > > > I've rebased a couple of things onto these two patches and am starting > > > > to rely on it locally, so I'm just going to apply both. It's easy to > > > > revert either of the if necessary. > > > > > > They're breaking the build: > > > > > > https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 > > > > > > I don't think it's worth reverting them because of this, but please > > > could you do a fixup? > > > > Yup, will take a look at this this evening. I saw it last night but it > > was too late to investigate it. This didn't happen locally so I'm > > guessing the issue is something to do with how Travis runs things. > > Worst case scenario, I will revert and can retry later. > > I can repro at least the pep8 stuff with > docker-compose run web --tox -e pep8 > > That seems to highlight the issue that ends up erroring the test, so > fixing the pep8 warnings should hopefully be sufficient. Yeah, these were modifications I squeezed in at the last minute and didn't test thoroughly. Patch submitted. Stephen ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
[PATCH] settings: Don't configure logging for parsearchive
A recent change added additional ERROR level logs to the 'parsearchive'
tool. This highlighted an issue, whereby we had configured all modules
in 'patchwork.management.command' to email administrators on ERROR logs.
We clearly shouldn't be doing this for the 'parsearchive' command or for
anything other than 'parsemail', so fix this.
Along the way, we remove a now-unnecessary 'extra' argument to one of
the logging calls in 'parsearchive' and resolve a pep8 issue.
Signed-off-by: Stephen Finucane
Fixes: 133091da ("parsearchive: Fix logging")
Cc: Daniel Axtens
---
patchwork/management/commands/parsearchive.py | 3 +--
patchwork/parser.py | 1 -
patchwork/settings/base.py| 2 +-
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/patchwork/management/commands/parsearchive.py
b/patchwork/management/commands/parsearchive.py
index e8904112..37db2b0d 100644
--- a/patchwork/management/commands/parsearchive.py
+++ b/patchwork/management/commands/parsearchive.py
@@ -97,8 +97,7 @@ class Command(BaseCommand):
logger.warning('Duplicate mail for message ID %s', exc.msgid)
except (ValueError, Exception) as exc:
errors += 1
-logger.warning('Invalid mail: %s', exc.message,
- extra={'mail': mail.as_string()})
+logger.warning('Invalid mail: %s', exc.message)
if verbosity < 3 and (i % 10) == 0:
self.stdout.write('%06d/%06d\r' % (i, count), ending='')
diff --git a/patchwork/parser.py b/patchwork/parser.py
index eb78702f..2ba1db74 100644
--- a/patchwork/parser.py
+++ b/patchwork/parser.py
@@ -1153,7 +1153,6 @@ def parse_mail(mail, list_id=None):
author = get_or_create_author(mail)
-
try:
comment = Comment.objects.create(
submission=submission,
diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
index 3eb1f0e2..1a62404f 100644
--- a/patchwork/settings/base.py
+++ b/patchwork/settings/base.py
@@ -185,7 +185,7 @@ LOGGING = {
'level': 'WARNING',
'propagate': False,
},
-'patchwork.management.commands': {
+'patchwork.management.commands.parsemail': {
'handlers': ['console', 'mail_admins'],
'level': 'WARNING',
'propagate': True,
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 0/5] Convert Series-Patch relationship to 1:N
Stephen Finucane writes:
> This is mostly a tech debt reduction exercise. As noted in the main
> patch, the M:N relationship between series and patches was, in
> hindsight, a design decision made for the wrong reasons. It's one of the
> things holding us back on the improved tagging series and it makes a lot
> of things more difficult to do than they should be. Time to fix it, IMO.
>
> Changes since v1:
> - Rebase on master. This results in a patch ("models: Remove
> 'SeriesMixin'") being dropped and the migrations being renumbered.
>
> Stephen Finucane (5):
> tests: Add more tests for series-ified mbox views
> tests: Hardcode expected values
> models: Convert Series-Patch relationship to 1:N
FYI, This patch hasn't hit my email client, but has hit Patchwork and
the archive. Bizzare. I will attempt to review it at some point
regardless, but not tonight - it's late enough already.
One brief point on that patch - are you updating the grant-all scripts
in lib/sql? A quick squiz suggest you aren't, but I think you need to.
Regards,
Daniel
> tests: Remove 'create_series_patch'
> views: Add support for boolean 'series' parameters
>
> patchwork/admin.py| 2 +-
> patchwork/api/cover.py| 19 +++--
> patchwork/api/patch.py| 20 --
> .../0031_add_patch_series_fields.py | 32 +
> ...migrate_data_from_series_patch_to_patch.py | 35 ++
> .../0033_remove_patch_series_model.py | 58
> patchwork/models.py | 60 ++--
> patchwork/parser.py | 6 +-
> patchwork/signals.py | 40 +--
> .../templates/patchwork/download_buttons.html | 17 +
> patchwork/templates/patchwork/patch-list.html | 12 ++--
> patchwork/templates/patchwork/submission.html | 26 ++-
> patchwork/tests/api/test_series.py| 12 ++--
> patchwork/tests/test_detail.py| 16 -
> patchwork/tests/test_events.py| 39 +++
> patchwork/tests/test_mboxviews.py | 69 +--
> patchwork/tests/test_series.py| 38 +-
> patchwork/tests/utils.py | 45 +++-
> patchwork/views/cover.py | 1 -
> patchwork/views/patch.py | 5 +-
> patchwork/views/utils.py | 17 ++---
> 21 files changed, 360 insertions(+), 209 deletions(-)
> create mode 100644 patchwork/migrations/0031_add_patch_series_fields.py
> create mode 100644
> patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py
> create mode 100644 patchwork/migrations/0033_remove_patch_series_model.py
>
> --
> 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 v2 5/5] views: Add support for boolean 'series' parameters
Stephen Finucane writes:
> Previously, we allowed users to download patch mboxes with dependencies
> included using a 'series' parameter. This accepted either a numeric ID,
> corresponding to the ID of the patch series that dependencies should be
> included from, or a wildcard value ('*').
>
> /patch/{patchID}/mbox/?series=123
> /patch/{patchID}/mbox/?series=*
>
> With switch to a 1:N series-patch relationship, this is clearly
> unnecessary now but must be retained to avoid breaking users. However,
> that doesn't mean we can't things a little clearer. Add support for
> boolean parameters, which make more sense for this kind of relationship:
>
> /patch/{patchID}/mbox/?series=true
> /patch/{patchID}/mbox/?series=1
> /patch/{patchID}/mbox/?series=false
> /patch/{patchID}/mbox/?series=0
>
> Signed-off-by: Stephen Finucane
> ---
> TODO: I'm not sure if I should do this or introduce a new parameter
> (maybe 'dependencies'?). Thoughts?
So my question is:
"Previously, specifying a wrong series number would lead to getting
no patches. Is it a problem if specifying a wrong series number now
leads to you getting the series patches?"
I would say that the answer to that question is "No, if you specify
?series=", you should get the whole series.
Therefore, I'd make the whole thing simpler by dropping the 0/false
thing and just returning the series if ?series is present.
Regards,
Daniel
> ---
> patchwork/tests/test_mboxviews.py | 17 +
> patchwork/views/patch.py | 2 +-
> patchwork/views/utils.py | 2 +-
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/patchwork/tests/test_mboxviews.py
> b/patchwork/tests/test_mboxviews.py
> index 2add3950..9d941bf8 100644
> --- a/patchwork/tests/test_mboxviews.py
> +++ b/patchwork/tests/test_mboxviews.py
> @@ -237,6 +237,23 @@ class MboxSeriesDependencies(TestCase):
> self.assertContains(response, patch_a.content)
> self.assertContains(response, patch_b.content)
>
> +def test_patch_with_boolean_series(self):
> +_, patch_a, patch_b = self._create_patches()
> +
> +for value in ('true', '1'):
> +response = self.client.get('%s?series=%s' % (
> +reverse('patch-mbox', args=[patch_b.id]), value))
> +
> +self.assertContains(response, patch_a.content)
> +self.assertContains(response, patch_b.content)
> +
> +for value in ('false', '0'):
> +response = self.client.get('%s?series=%s' % (
> +reverse('patch-mbox', args=[patch_b.id]), value))
> +
> +self.assertNotContains(response, patch_a.content)
> +self.assertContains(response, patch_b.content)
> +
> def test_patch_with_invalid_series(self):
> series, patch_a, patch_b = self._create_patches()
>
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 9fed4312..91b98e38 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -144,7 +144,7 @@ def patch_mbox(request, patch_id):
> series_id = request.GET.get('series')
>
> response = HttpResponse(content_type='text/plain')
> -if series_id:
> +if series_id and series_id.lower() not in ('false', '0'):
> if not patch.series:
> raise Http404('Patch does not have an associated series. This is
> '
>'because the patch was processed with an older '
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index bfcf6aba..4644c621 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -141,7 +141,7 @@ def series_patch_to_mbox(patch, series_id):
> Returns:
> A string for the mbox file.
> """
> -if series_id != '*':
> +if series_id.lower() not in ('*', 'true', '1'):
> try:
> series_id = int(series_id)
> except ValueError:
> --
> 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 v2 2/5] tests: Hardcode expected values
Also LGTM.
Reviewed-by: Daniel Axtens
Regards,
Daniel
Stephen Finucane writes:
> Compare against known values to ensure bugs introduced in the function
> are caught.
>
> Signed-off-by: Stephen Finucane
> ---
> patchwork/tests/test_series.py | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
> index 6892a615..8b71fef1 100644
> --- a/patchwork/tests/test_series.py
> +++ b/patchwork/tests/test_series.py
> @@ -637,10 +637,6 @@ class SeriesNameTestCase(TestCase):
> def _parse_mail(self, mail):
> return parser.parse_mail(mail, self.project.listid)
>
> -@staticmethod
> -def _format_name(cover):
> -return models.Series._format_name(cover)
> -
> def test_cover_letter(self):
> """Cover letter name set as series name.
>
> @@ -656,7 +652,7 @@ class SeriesNameTestCase(TestCase):
> mbox = self._get_mbox('base-cover-letter.mbox')
>
> cover = self._parse_mail(mbox[0])
> -cover_name = self._format_name(cover)
> +cover_name = 'A sample series'
> self.assertEqual(cover.series.first().name, cover_name)
>
> self._parse_mail(mbox[1])
> @@ -711,7 +707,7 @@ class SeriesNameTestCase(TestCase):
> self.assertEqual(patch.series.first().name, patch.name)
>
> cover = self._parse_mail(mbox[2])
> -self.assertEqual(cover.series.first().name, self._format_name(cover))
> +self.assertEqual(cover.series.first().name, 'A sample series')
>
> mbox.close()
>
> --
> 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 v2 1/5] tests: Add more tests for series-ified mbox views
Stephen Finucane writes:
LGTM and tests are good.
Reviewed-by: Daniel Axtens
Regards,
Daniel
> Cover some testing gaps identified during the migration from a M:N to a
> 1:N series-patch relationship, namely:
>
> - Downloading a patch's mbox with dependencies using a numerical series
> ID
> - Downloading a series' mbox
>
> Signed-off-by: Stephen Finucane
> ---
> patchwork/tests/test_mboxviews.py | 40 ++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/patchwork/tests/test_mboxviews.py
> b/patchwork/tests/test_mboxviews.py
> index b7af746b..ecf46674 100644
> --- a/patchwork/tests/test_mboxviews.py
> +++ b/patchwork/tests/test_mboxviews.py
> @@ -31,6 +31,7 @@ from patchwork.tests.utils import create_comment
> from patchwork.tests.utils import create_patch
> from patchwork.tests.utils import create_project
> from patchwork.tests.utils import create_person
> +from patchwork.tests.utils import create_series
> from patchwork.tests.utils import create_series_patch
> from patchwork.tests.utils import create_user
>
> @@ -211,16 +212,40 @@ class MboxCommentPostcriptUnchangedTest(TestCase):
>
> class MboxSeriesDependencies(TestCase):
>
> -def test_patch_with_dependencies(self):
> +@staticmethod
> +def _create_patches():
> patch_a = create_series_patch()
> patch_b = create_series_patch(series=patch_a.series)
>
> +return patch_a.series, patch_a, patch_b
> +
> +def test_patch_with_wildcard_series(self):
> +_, patch_a, patch_b = self._create_patches()
> +
> response = self.client.get('%s?series=*' % reverse(
> 'patch-mbox', args=[patch_b.patch.id]))
>
> self.assertContains(response, patch_a.patch.content)
> self.assertContains(response, patch_b.patch.content)
>
> +def test_patch_with_numeric_series(self):
> +series, patch_a, patch_b = self._create_patches()
> +
> +response = self.client.get('%s?series=%d' % (
> +reverse('patch-mbox', args=[patch_b.patch.id]), series.id))
> +
> +self.assertContains(response, patch_a.patch.content)
> +self.assertContains(response, patch_b.patch.content)
> +
> +def test_patch_with_invalid_series(self):
> +series, patch_a, patch_b = self._create_patches()
> +
> +for value in ('foo', str(series.id + 1)):
> +response = self.client.get('%s?series=%s' % (
> +reverse('patch-mbox', args=[patch_b.patch.id]), value))
> +
> +self.assertEqual(response.status_code, 404)
> +
> def test_legacy_patch(self):
> """Validate a patch with non-existent dependencies raises a 404."""
> # we're explicitly creating a patch without a series
> @@ -230,3 +255,16 @@ class MboxSeriesDependencies(TestCase):
> 'patch-mbox', args=[patch.id]))
>
> self.assertEqual(response.status_code, 404)
> +
> +
> +class MboxSeries(TestCase):
> +
> +def test_series(self):
> +series = create_series()
> +patch_a = create_series_patch(series=series)
> +patch_b = create_series_patch(series=series)
> +
> +response = self.client.get(reverse('series-mbox', args=[series.id]))
> +
> +self.assertContains(response, patch_a.patch.content)
> +self.assertContains(response, patch_b.patch.content)
> --
> 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 v2 1/2] parsearchive: Fix logging
Stephen Finucane writes: > On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: >> Hi Stephen, >> >> > I've rebased a couple of things onto these two patches and am starting >> > to rely on it locally, so I'm just going to apply both. It's easy to >> > revert either of the if necessary. >> >> They're breaking the build: >> >> https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 >> >> I don't think it's worth reverting them because of this, but please >> could you do a fixup? > > Yup, will take a look at this this evening. I saw it last night but it > was too late to investigate it. This didn't happen locally so I'm > guessing the issue is something to do with how Travis runs things. > Worst case scenario, I will revert and can retry later. I can repro at least the pep8 stuff with docker-compose run web --tox -e pep8 That seems to highlight the issue that ends up erroring the test, so fixing the pep8 warnings should hopefully be sufficient. Regards, Daniel > > Stephen > >> Regards, >> Daniel >> > >> > Stephen ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2] requirements: Start using fixed versions
Stephen Finucane writes:
> On Sat, 2018-09-22 at 00:19 +1000, Daniel Axtens wrote:
>> Stephen Finucane writes:
>>
>> > Given that 'tox' doesn't actually read any of these, there's no reason
>> > to use ranges of requirements. Instead, use the latest and greatest for
>> > live instances and rely on tox to validate behavior with older versions.
>>
>> I don't really understand the motivation for this, but I am not
>> primarily a python developer, so I'm going to assume it's standard
>> practise in the Python world. (And I see the pyup bot constantly making
>> PRs for this sort of stuff, so that makes me more confident that this is
>> the case.) On that basis, given that v2 fixes Postgres, I have merged
>> it.
>
> I've only grasped this myself recently so I'll just dump my impression
> of things here.
>
> When it comes to deciding how to manage requirements, you've got to
> assess the two types of software you might encounter.
>
> * libraries
> * applications
>
> Libraries are consumed by other projects and should therefore aim to
> support as wide a range of requirements as possible/practical. This
> allows the library itself to be as widely used as possible.
> Applications, on the other hand, are consumers of libraries and are
> rarely consumed themselves. For that reason, they don't need this
> flexibility.
>
> Given that Patchwork is an application, we fall into the latter camp.
> In an ideal world, this means we wouldn't need to support more than a
> single version of each of our requirements, allowing us to test against
> one set of dependencies and require those. However, we don't get to
> dictate how our users install their dependencies, meaning we need to
> support a broader range of dependencies to allow people to install from
> PyPI, apt, dnf/yum, etc. That said, given that we validate this by way
> of tox, we can use 'requirements.txt' as the source of "optimal" (read:
> latest and greatest) requirements, while the 'deps' section in tox
> defines the requirements for various other configurations.
>
> Does that make sense?
Huh, nifty. Sounds good to me, thanks for the explanation.
Daniel
>
> Stephen
>
>> Regards,
>> Daniel
>>
>> >
>> > The selenium dependency, which is no longer required since commit
>> > bab2895f, is removed. The psycopg2 dependency is updated to use
>> > psycopg2-binary, as this avoids the need for the libpg library and
>> > removes a deprecation warning.
>> >
>> > Signed-off-by: Stephen Finucane
>> > ---
>> > v2:
>> > - Include psycopg2-binary in requirements-test.txt
>> > - Include various django* requirements in requirements-dev.txt instead
>> > of simply including requirements-prod.txt, to prevent conflicts with
>> > psycopg2-binary dependencies
>> > - Update dependencies to latest PATCH versions
>> > ---
>> > requirements-dev.txt | 8
>> > requirements-prod.txt | 10 +-
>> > requirements-test.txt | 7 +++
>> > tox.ini | 1 -
>> > 4 files changed, 12 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/requirements-dev.txt b/requirements-dev.txt
>> > index f4ad751a..70b747f5 100644
>> > --- a/requirements-dev.txt
>> > +++ b/requirements-dev.txt
>> > @@ -1,5 +1,5 @@
>> > -Django>=1.11,<2.1; python_version >= '3.4'
>> > -Django>=1.11,<2.0; python_version < '3.0'
>> > -djangorestframework>=3.4,<3.9
>> > -django-filter>=1.0,<1.2
>> > +Django==2.0.8; python_version >= '3.4'
>> > +Django==1.11.15; python_version < '3.0'
>> > +djangorestframework==3.8.2
>> > +django-filter==1.1.0
>> > -r requirements-test.txt
>> > diff --git a/requirements-prod.txt b/requirements-prod.txt
>> > index e7a75354..efe6743c 100644
>> > --- a/requirements-prod.txt
>> > +++ b/requirements-prod.txt
>> > @@ -1,6 +1,6 @@
>> > -Django>=1.11,<2.1; python_version >= '3.4'
>> > -Django>=1.11,<2.0; python_version < '3.0'
>> > -djangorestframework>=3.4,<3.9
>> > -django-filter>=1.0,<1.2
>> > -psycopg2>=2.7,<2.8
>> > +Django==2.0.8; python_version >= '3.4'
>> > +Django==1.11.15; python_version < '3.0'
>> > +djangorestframework==3.8.2
>> > +django-filter==1.1.0
>> > +psycopg2-binary==2.7.5
>> > sqlparse==0.2.4
>> > diff --git a/requirements-test.txt b/requirements-test.txt
>> > index 94dc3db7..295cceff 100644
>> > --- a/requirements-test.txt
>> > +++ b/requirements-test.txt
>> > @@ -1,5 +1,4 @@
>> > -mysqlclient>=1.3,<1.4
>> > -psycopg2-binary>=2.7,<2.8
>> > +mysqlclient==1.3.13
>> > +psycopg2-binary==2.7.5
>> > django-debug-toolbar==1.9.1
>> > -python-dateutil>2.0,<3.0
>> > -selenium>=3.0,<4.0
>> > +python-dateutil==2.7.3
>> > diff --git a/tox.ini b/tox.ini
>> > index 3684d716..3a783d26 100644
>> > --- a/tox.ini
>> > +++ b/tox.ini
>> > @@ -19,7 +19,6 @@ setenv =
>> > py27: PYTHONWARNINGS = once
>> > py{34,36}:PYTHONWARNINGS = once,ignore::ImportWarning:backports
>> > py35:PYTHONWARNINGS =
>> > once,ignore::ResourceWarning:unittest.suite,ignore::ImportWarning:backports
>> > -
>> > passenv =
>> > http_proxy HTTP_PROXY https_proxy
Re: [PATCH] Find patches, cover letters and comments by msgid
Hi Stephen,
>> /message/ does not require a project, but therefore if a mail
>> has been sent to multiple projects, the project that you will be
>> redirected to is arbitrary.
>
> Personally, I'm not a fan of the latter as I don't see what value it
> would bring. Is there any reason to keep it or could we drop it.
As Don points out, without this the feature is basically useless for
kernel work. Given that kernel work represents the bulk of both OzLabs
PW and kernel.org PW, it's reasonably critical.
>> These URLs also just redirect to the usual canonical patchwork URL -
>> it doesn't supplant or replace them. However, I'd be open to moving to
>> this scheme as the 'normal'/default URL in the future.
>
> Personally, I'd reverse the order of things and make these the new
> canonical URLs. There are issues with using the autoincrement ID
> column, as Konstantin has pointed out, but we could continue to use
> these for shortlinks until such a time as we come up with something
> better.
>
>> I also haven't attempted to do anything meaningful with series.
>
> I think this needs to be addressed too. At a minimum, we should do
> cover letters.
>> +# message-id searches
>> +url(r'^project/(?P[^/]+)/message/(?P[^/]+)$',
>> +patch_views.patch_by_project_msgid,
>> +name='msgid-project-redirect'),
>
> Given my above comments, I wonder if we should split this into 'patch'
> and 'cover' again? I realize it's more work, but I really do think
> these should become the canonical links.
I'm not 100% sure what you had in mind here; I'm guessing you mean that
you want the ultimate URL scheme to be:
For patches:
The canonical URL - /project/N/patch/
Accepted, redirects you to canonical URL - /project/N/patch/NNN
Accepted, guesses a project and redirects - /patch/
For covers:
The canonical URL - /project/N/cover/
Accepted, redirects you to canonical URL - /project/N/cover/NNN
Accepted, guesses a project and redirects - /cover/
(And I suppose if you get patch vs cover wrong it'll just redirect you
to the right one.)
For series, I'm even less clear on what you would like. My guess would
be that we could have /project/N/series/ just show you an
abbreviated version of the entire series that msg_id is a part of. I
think that would be super useful but would require some UI work
(designing a template to show you a whole series) and ideally for the
1:N series change to land. It might be better for a follow up.
Does that line up with what you had in mind?
Regards,
Daniel
>
> Stephen
>
>> +url(r'^message/(?P[^/]+)$', patch_views.patch_by_msgid,
>> +name='msgid-redirect'),
>> +
>> # logged-in user stuff
>> url(r'^user/$', user_views.profile, name='user-profile'),
>> url(r'^user/todo/$', user_views.todo_lists,
>> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
>> index 6921882e71d1..53441b39cc57 100644
>> --- a/patchwork/views/patch.py
>> +++ b/patchwork/views/patch.py
>> @@ -28,6 +28,7 @@ from django.urls import reverse
>>
>> from patchwork.forms import CreateBundleForm
>> from patchwork.forms import PatchForm
>> +from patchwork.models import Comment
>> from patchwork.models import Bundle
>> from patchwork.models import Patch
>> from patchwork.models import Project
>> @@ -150,3 +151,51 @@ def patch_mbox(request, patch_id):
>> patch.filename)
>>
>> return response
>> +
>> +
>> +def patch_by_msgid(request, msg_id):
>> +msgid = ('<%s>' % msg_id)
>> +
>> +patches = Patch.objects.filter(msgid=msgid)
>> +if patches:
>> +return HttpResponseRedirect(
>> +reverse('patch-detail', kwargs={'patch_id':
>> patches.first().id}))
>> +
>> +subs = Submission.objects.filter(msgid=msgid)
>> +if subs:
>> +return HttpResponseRedirect(
>> +reverse('cover-detail', kwargs={'cover_id':
>> subs.first().id}))
>> +
>> +comments = Comment.objects.filter(msgid=msgid)
>> +if comments:
>> +return HttpResponseRedirect(comments.first().get_absolute_url())
>> +
>> +raise Http404("No patch, cover letter of comment matching %s found." %
>> msg_id)
>> +
>> +
>> +def patch_by_project_msgid(request, project_id, msg_id):
>> +project = get_object_or_404(Project, linkname=project_id)
>> +project_id = project.id
>> +msgid = ('<%s>' % msg_id)
>> +
>> +try:
>> +patch = Patch.objects.get(patch_project_id=project_id, msgid=msgid)
>> +return HttpResponseRedirect(
>> +reverse('patch-detail', kwargs={'patch_id': patch.id}))
>> +except Patch.DoesNotExist:
>> +pass
>> +
>> +try:
>> +sub = Submission.objects.get(project_id=project_id, msgid=msgid)
>> +return HttpResponseRedirect(
>> +reverse('cover-detail', kwargs={'cover_id': sub.id}))
>> +except Submission.DoesNotExist:
>> +pass
>> +
>> +try:
>> +comment =
Re: [PATCH 00/11] Add labels support
Don Zickus writes: > On Tue, Sep 11, 2018 at 12:21:26PM -0600, Stephen Finucane wrote: >> > Personally I would *really* like labels to land. They unlock a lot of >> > potential improvements to workflow for maintainers, eg. automated >> > tagging, tagging based on test results etc. As well as finer grained >> > reporting of status to submitters, eg. instead of "new" -> "under >> > review" -> "accepted", I can mark a patch as "under review" and >> > "applied-to-some-branch", then "under review" and >> > "in-testing" etc. etc. >> > >> > Would it simplify (or not?) the implementation if states became a >> > special case of labels? >> >> Oh, that's a really good idea, actually. The model I had been following >> for the "Remove State" series was to add two new fields to the "Patch" >> model: 'is_open' and 'resolution', the former being a boolean >> open/closed value and the later being an enum of the default state >> fields. I'd be happy to move this entire thing to labels though. Does >> anyone else (Daniel, Don) have thoughts on this? > > I would like to see a little more detail on the format of this would look > like. Sorta like how you describe what the data input looks like for > 'checks'. > > This probably works for us, just want to be sure. > I've thought about this a bit more. Here is a brain dump. Lets suppose we're redesigning the entire idea of the metadata about a patch that Patchwork tracks. States, Checks, Tags, whatever. At one end of the spectrum of options is that we just allow you to attach arbitrary strings to patches, and give you a UI to filter on them. So some examples: Patch | Tags |-- Fix foo -> bar | state:new, check:checkpatch:passed:, check:tox:failed:, | "reviewer:Jane Smith", for-stable |-- [RFC] framisets | rfc, "state:Not Applicable", applied-tech-preview |-- [v2] framisets | v2, "tag:Acked-by: Daniel Axtens ", ... --- Then everything just becomes a matter of people standardising on string formats and making them meaningful to the UI. Another step along the spectrum would be to do that for some things, but keep other bits as separate tables/models. The strongest argument would be to say that Checks are good as they are and we should exclude them from being labels. The next step would be to say that for efficient filtering, States or some close analogue need to be separate fields. Stephen, I think this is closest to your approach where you add a field to determine whether or not a patch should be shown by default in the UI (is_open). Going all the way to the other end of the spectrum, everthing should get its own DB field and we should support labels by just adding an array of strings to the existing patch/series models. And we should clean up tag support. Each approach has its own strengths and weaknesses. In evaluating them, I'm particularly cognisant of: - not breaking people's workflows, so you have to be able to do the same things in the same ways and move towards new features at your own pace. We should avoid foisting backwards-incompatible changes on people. - performance. The first approach is I think conceptually quite nice. We can just define particular types of labels that get treated specially in the UI (state:, check:, tag:, etc.), migrate things, and people can go for their lives. However, it seems like it might be an implementation nightmare. For example, how you do keep track of every State used in a project if they're strings in a list of strings? And I don't know how you'd implement it in a performant way. Either you store the labels as lists of strings and have to do string searches on every patch in your project just to list them, or you have some join table that's probably even worse. (Or there's DB magic here that I am not aware of.) I also have a soft spot for the final approach of doing the simplest possible thing and just adding a list of strings that people can additionally search on. However, I do think it would be a shame to pass up a chance to simplify things. I think (or at least I currently think) that tags should become labels. I would accept a reasonable approach to making states into labels if you can overcome the implementation issues. However, I don't think checks should become labels - they have a really nice structure of their own. Does that all make sense? Stephen, I think that at least in a very broad sense that lines up with how you were thinking about things - does that sound right to you? Did you have something in mind for filtering by states if they become some sort of label? Regards, Daniel > Cheers, > Don ___ Patchwork mailing list Patchwork@lists.ozlabs.org
Re: [PATCH v2] requirements: Start using fixed versions
On Sat, 2018-09-22 at 00:19 +1000, Daniel Axtens wrote:
> Stephen Finucane writes:
>
> > Given that 'tox' doesn't actually read any of these, there's no reason
> > to use ranges of requirements. Instead, use the latest and greatest for
> > live instances and rely on tox to validate behavior with older versions.
>
> I don't really understand the motivation for this, but I am not
> primarily a python developer, so I'm going to assume it's standard
> practise in the Python world. (And I see the pyup bot constantly making
> PRs for this sort of stuff, so that makes me more confident that this is
> the case.) On that basis, given that v2 fixes Postgres, I have merged
> it.
I've only grasped this myself recently so I'll just dump my impression
of things here.
When it comes to deciding how to manage requirements, you've got to
assess the two types of software you might encounter.
* libraries
* applications
Libraries are consumed by other projects and should therefore aim to
support as wide a range of requirements as possible/practical. This
allows the library itself to be as widely used as possible.
Applications, on the other hand, are consumers of libraries and are
rarely consumed themselves. For that reason, they don't need this
flexibility.
Given that Patchwork is an application, we fall into the latter camp.
In an ideal world, this means we wouldn't need to support more than a
single version of each of our requirements, allowing us to test against
one set of dependencies and require those. However, we don't get to
dictate how our users install their dependencies, meaning we need to
support a broader range of dependencies to allow people to install from
PyPI, apt, dnf/yum, etc. That said, given that we validate this by way
of tox, we can use 'requirements.txt' as the source of "optimal" (read:
latest and greatest) requirements, while the 'deps' section in tox
defines the requirements for various other configurations.
Does that make sense?
Stephen
> Regards,
> Daniel
>
> >
> > The selenium dependency, which is no longer required since commit
> > bab2895f, is removed. The psycopg2 dependency is updated to use
> > psycopg2-binary, as this avoids the need for the libpg library and
> > removes a deprecation warning.
> >
> > Signed-off-by: Stephen Finucane
> > ---
> > v2:
> > - Include psycopg2-binary in requirements-test.txt
> > - Include various django* requirements in requirements-dev.txt instead
> > of simply including requirements-prod.txt, to prevent conflicts with
> > psycopg2-binary dependencies
> > - Update dependencies to latest PATCH versions
> > ---
> > requirements-dev.txt | 8
> > requirements-prod.txt | 10 +-
> > requirements-test.txt | 7 +++
> > tox.ini | 1 -
> > 4 files changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/requirements-dev.txt b/requirements-dev.txt
> > index f4ad751a..70b747f5 100644
> > --- a/requirements-dev.txt
> > +++ b/requirements-dev.txt
> > @@ -1,5 +1,5 @@
> > -Django>=1.11,<2.1; python_version >= '3.4'
> > -Django>=1.11,<2.0; python_version < '3.0'
> > -djangorestframework>=3.4,<3.9
> > -django-filter>=1.0,<1.2
> > +Django==2.0.8; python_version >= '3.4'
> > +Django==1.11.15; python_version < '3.0'
> > +djangorestframework==3.8.2
> > +django-filter==1.1.0
> > -r requirements-test.txt
> > diff --git a/requirements-prod.txt b/requirements-prod.txt
> > index e7a75354..efe6743c 100644
> > --- a/requirements-prod.txt
> > +++ b/requirements-prod.txt
> > @@ -1,6 +1,6 @@
> > -Django>=1.11,<2.1; python_version >= '3.4'
> > -Django>=1.11,<2.0; python_version < '3.0'
> > -djangorestframework>=3.4,<3.9
> > -django-filter>=1.0,<1.2
> > -psycopg2>=2.7,<2.8
> > +Django==2.0.8; python_version >= '3.4'
> > +Django==1.11.15; python_version < '3.0'
> > +djangorestframework==3.8.2
> > +django-filter==1.1.0
> > +psycopg2-binary==2.7.5
> > sqlparse==0.2.4
> > diff --git a/requirements-test.txt b/requirements-test.txt
> > index 94dc3db7..295cceff 100644
> > --- a/requirements-test.txt
> > +++ b/requirements-test.txt
> > @@ -1,5 +1,4 @@
> > -mysqlclient>=1.3,<1.4
> > -psycopg2-binary>=2.7,<2.8
> > +mysqlclient==1.3.13
> > +psycopg2-binary==2.7.5
> > django-debug-toolbar==1.9.1
> > -python-dateutil>2.0,<3.0
> > -selenium>=3.0,<4.0
> > +python-dateutil==2.7.3
> > diff --git a/tox.ini b/tox.ini
> > index 3684d716..3a783d26 100644
> > --- a/tox.ini
> > +++ b/tox.ini
> > @@ -19,7 +19,6 @@ setenv =
> > py27: PYTHONWARNINGS = once
> > py{34,36}:PYTHONWARNINGS = once,ignore::ImportWarning:backports
> > py35:PYTHONWARNINGS =
> > once,ignore::ResourceWarning:unittest.suite,ignore::ImportWarning:backports
> > -
> > passenv =
> > http_proxy HTTP_PROXY https_proxy HTTPS_PROXY no_proxy NO_PROXY
> > PW_TEST_DB_TYPE PW_TEST_DB_USER PW_TEST_DB_PASS PW_TEST_DB_HOST
> > --
> > 2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
Re: [PATCH v2 1/2] parsearchive: Fix logging
On Sat, 2018-09-22 at 00:22 +1000, Daniel Axtens wrote: > Hi Stephen, > > > I've rebased a couple of things onto these two patches and am starting > > to rely on it locally, so I'm just going to apply both. It's easy to > > revert either of the if necessary. > > They're breaking the build: > > https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 > > I don't think it's worth reverting them because of this, but please > could you do a fixup? Yup, will take a look at this this evening. I saw it last night but it was too late to investigate it. This didn't happen locally so I'm guessing the issue is something to do with how Travis runs things. Worst case scenario, I will revert and can retry later. Stephen > Regards, > Daniel > > > > Stephen ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 1/2] parsearchive: Fix logging
Hi Stephen, > I've rebased a couple of things onto these two patches and am starting > to rely on it locally, so I'm just going to apply both. It's easy to > revert either of the if necessary. They're breaking the build: https://travis-ci.org/getpatchwork/patchwork/jobs/431231311#L486 I don't think it's worth reverting them because of this, but please could you do a fixup? Regards, Daniel > > 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 v2] requirements: Start using fixed versions
Stephen Finucane writes:
> Given that 'tox' doesn't actually read any of these, there's no reason
> to use ranges of requirements. Instead, use the latest and greatest for
> live instances and rely on tox to validate behavior with older versions.
I don't really understand the motivation for this, but I am not
primarily a python developer, so I'm going to assume it's standard
practise in the Python world. (And I see the pyup bot constantly making
PRs for this sort of stuff, so that makes me more confident that this is
the case.) On that basis, given that v2 fixes Postgres, I have merged
it.
Regards,
Daniel
>
> The selenium dependency, which is no longer required since commit
> bab2895f, is removed. The psycopg2 dependency is updated to use
> psycopg2-binary, as this avoids the need for the libpg library and
> removes a deprecation warning.
>
> Signed-off-by: Stephen Finucane
> ---
> v2:
> - Include psycopg2-binary in requirements-test.txt
> - Include various django* requirements in requirements-dev.txt instead
> of simply including requirements-prod.txt, to prevent conflicts with
> psycopg2-binary dependencies
> - Update dependencies to latest PATCH versions
> ---
> requirements-dev.txt | 8
> requirements-prod.txt | 10 +-
> requirements-test.txt | 7 +++
> tox.ini | 1 -
> 4 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/requirements-dev.txt b/requirements-dev.txt
> index f4ad751a..70b747f5 100644
> --- a/requirements-dev.txt
> +++ b/requirements-dev.txt
> @@ -1,5 +1,5 @@
> -Django>=1.11,<2.1; python_version >= '3.4'
> -Django>=1.11,<2.0; python_version < '3.0'
> -djangorestframework>=3.4,<3.9
> -django-filter>=1.0,<1.2
> +Django==2.0.8; python_version >= '3.4'
> +Django==1.11.15; python_version < '3.0'
> +djangorestframework==3.8.2
> +django-filter==1.1.0
> -r requirements-test.txt
> diff --git a/requirements-prod.txt b/requirements-prod.txt
> index e7a75354..efe6743c 100644
> --- a/requirements-prod.txt
> +++ b/requirements-prod.txt
> @@ -1,6 +1,6 @@
> -Django>=1.11,<2.1; python_version >= '3.4'
> -Django>=1.11,<2.0; python_version < '3.0'
> -djangorestframework>=3.4,<3.9
> -django-filter>=1.0,<1.2
> -psycopg2>=2.7,<2.8
> +Django==2.0.8; python_version >= '3.4'
> +Django==1.11.15; python_version < '3.0'
> +djangorestframework==3.8.2
> +django-filter==1.1.0
> +psycopg2-binary==2.7.5
> sqlparse==0.2.4
> diff --git a/requirements-test.txt b/requirements-test.txt
> index 94dc3db7..295cceff 100644
> --- a/requirements-test.txt
> +++ b/requirements-test.txt
> @@ -1,5 +1,4 @@
> -mysqlclient>=1.3,<1.4
> -psycopg2-binary>=2.7,<2.8
> +mysqlclient==1.3.13
> +psycopg2-binary==2.7.5
> django-debug-toolbar==1.9.1
> -python-dateutil>2.0,<3.0
> -selenium>=3.0,<4.0
> +python-dateutil==2.7.3
> diff --git a/tox.ini b/tox.ini
> index 3684d716..3a783d26 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -19,7 +19,6 @@ setenv =
> py27: PYTHONWARNINGS = once
> py{34,36}:PYTHONWARNINGS = once,ignore::ImportWarning:backports
> py35:PYTHONWARNINGS =
> once,ignore::ResourceWarning:unittest.suite,ignore::ImportWarning:backports
> -
> passenv =
> http_proxy HTTP_PROXY https_proxy HTTPS_PROXY no_proxy NO_PROXY
> PW_TEST_DB_TYPE PW_TEST_DB_USER PW_TEST_DB_PASS PW_TEST_DB_HOST
> --
> 2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
