[PATCH 00/11] Add labels support

[Daniel Axtens]

Don Zickus <dzickus at redhat.com> 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:<URL>, check:tox:failed:<URL>,
                | "reviewer:Jane Smith", for-stable
----------------|------------------------------------------------------
[RFC] framisets | rfc, "state:Not Applicable", applied-tech-preview
----------------|------------------------------------------------------
[v2] framisets  | v2, "tag:Acked-by: Daniel Axtens <dja at axtens.net>", ...
-----------------------------------------------------------------------

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