[PATCH 00/11] Add labels support

[Don Zickus]

On Sat, Sep 22, 2018 at 12:53:46AM +1000, Daniel Axtens wrote:
> 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.

We sorta did this internally, except a little simpler all tags were just

key : value

acked-by: <email>
build: <url>
test: <url>
superseded-by: <patch id>
bugzilla: <url>

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

I don't see why 'states' need to be labels, but I don't think it matters to
us if you can handle the performance issue.  Checks maybe ok for our needs
to.

I am trying to think of your definition of tag vs. label and I am guessing
it means

tag - key:value
label - arbitrary string

??

If so, searching does look challenging, though we keep things relative to
patches so the volume is manageable 20 or so items.  But if we wanted to
search across patches, that becomes interesting...

Thanks for the brain dump!

Cheers,
Don

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