[PATCH 00/11] Add labels support

[Michael Ellerman]

Daniel Axtens <dja at axtens.net> writes:

> 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.)

If you store tags as (id, key, value) then I think you can avoid most of
the string searching problems.

ie. when someone tags a patch with "state:fubar" that looks in the tag
table and finds no match and so creates an entry (1, "state", "fubar").

That gives you a table where you can see all the tags, and if you filter
by key == "state" you can see all the states.


And I think it would be a reasonable compromise for the Patch model to
still have a "state" field, but it would just point to an entry in the
tag table.

That allows people to create arbitrary states but still (hopefully)
makes it efficient to filter patches by state.

It does enforce that a patch can only have one state, but that seems
reasonable.

cheers