[Freeipa-devel] Reviewer in Trac

[Simo Sorce]

On Thu, 2014-02-20 at 16:34 +0100, Petr Viktorin wrote:
> On 02/20/2014 04:15 PM, Simo Sorce wrote:
> > On Thu, 2014-02-20 at 16:13 +0100, Martin Kosek wrote:
> >> On 02/20/2014 04:09 PM, Simo Sorce wrote:
> >>> On Thu, 2014-02-20 at 15:59 +0100, Martin Kosek wrote:
> >>>> On 02/20/2014 03:52 PM, Jakub Hrozek wrote:
> >>>>> On Thu, Feb 20, 2014 at 01:22:56PM +0100, Petr Viktorin wrote:
> >>>>>> On 02/20/2014 01:14 PM, Martin Kosek wrote:
> >>>>>>> We had a discussion with other developers how better track who is reviewing
> >>>>>>> which patch. Recently, we introduced the Reviewed-By tag in a commit message,
> >>>>>>> but that is a post-review tag which is not useful for someone who wants to know
> >>>>>>> which patches are already reviewed and which are not reviewed.
> >>>>>>>
> >>>>>>> We were testing Patch Work [1] in last months to contain this information, but
> >>>>>>> I personally think that it is suboptimal - it introduces 2 tracking tools that
> >>>>>>> needs to be maintained (Trac and Patch Work) and the Patch Work still requires
> >>>>>>> lot of manual actions when maintaining it's state.
> >>>>>>>
> >>>>>>> I think it would be better to hold this information rather in a single tracking
> >>>>>>> tool - Trac. There are 2 options:
> >>>>>>>
> >>>>>>> 1) "Patch on review" flag, similar to "Patch posted for review" flag which
> >>>>>>> would hold 1 bit information if the patch is just lying there or has somebody
> >>>>>>> assigned.
> >>>>>>>
> >>>>>>> 2) "Reviewed by" text field which would hold a login of a person who is
> >>>>>>> reviewing it. It would be filled either by a person starting the review or by a
> >>>>>>> supervisor like me to forcefully assign a reviewer ;-)
> >>>>>>>
> >>>>>>> With that information in Trac, we could run using a single tracking tool for
> >>>>>>> all patches that have a ticket (which is 95% of patches). It would be then
> >>>>>>> fairly easy to see which patches are sent for review but are reviewer-less.
> >>>>>>>
> >>>>>>> It would also have a benefit for Petr's sendpatches.py script which could pull
> >>>>>>> the reviewer from a ticket and one would not have to use the "-r" option to
> >>>>>>> hard code a reviewer.
> >>>>>>>
> >>>>>>> Any objections to using "Reviewed by" field?
> >>>>>>
> >>>>>> +1, this is the only thing I used Patchwork for, and keeping
> >>>>>> Patchwork up to date just for this took a lot of unnecessary
> >>>>>> mindless clicking.
> >>>>>>
> >>>>>> Just a nitpick: name it "Patch Reviewer"
> >>>>>> - there's more to a ticket than a patch
> >>>>>> - the review is not done yet when the field is filled out
> >>>>>
> >>>>> The only use-case I use patchwork for right now is a 'dashboard' to see
> >>>>> which patches need attention. If we could get this dashboard-like view
> >>>>> from Trac with some custom query, then I'm fine with deprecating
> >>>>> Patchwork.
> >>>>
> >>>> +1. I would like to add the reviewer to default report 3 + prepare a new view
> >>>> "My Active Reviews by Milestone" to see the reviews which one should track.
> >>>>
> >>>>>
> >>>>> However, one feature of patchwork was that each re-submission of a
> >>>>> patch triggered a new thread so as a reviewer you could easily see there
> >>>>> is a new instance of the patch that you need to look at. I suspect Trac
> >>>>> wouldn't give us anything like that?
> >>>>
> >>>> When I get a review, I like to get the response to inbox - then I always know.
> >>>> When replies are only being sent to the list, we would have to use the advanced
> >>>> Trac workflow and cautiously change states between accepted - submitted - onreview.
> >>>
> >>> I think this means we'll be back to have to carefully track the mailing
> >>> list because stuff will be missed. This is something patchwork "fixed".
> 
> No. The only thing that happened automatically in Patchwork was that 
> entries got created. Patchwork doesn't even have threads

This is not true, patchwork keeps together all answers to a patch (the
review) until you send a new one.

>  - each version 
> of a patch needed to be individually marked as superseded. Very much 
> mindless clicking is needed to keep Patchwork up to date.

It is not perfect for sure, not claiming that.

> >>> I wonder if we can build some automatism to not loose the good things
> >>> here.
> >>>
> >>> Simo.
> >>
> >> Majority of patches going to freeipa-devel are tied to some Trac ticket. These
> >> are tracked and watched by the on_review flag and the new reviewer field.
> >
> > If people remember to do it every time they just send an email, very
> > process heavy.
> 
> How is this different from Patchwork?

You get a new thread with every submission, so you know there is
something new autoatically by just looking at the list of patches, all
replys are also inline, so you know if a specific version of a patch has
got replies (there are caveats I know).

> >> Those that are not covered by any Trac ticket need to be tracked and cared of
> >> manually by the submitter IMO.
> >
> > Not very friendly to external submitters.
> 
> External submitters can easily open tickets.

Very process heavy, we need to make things easier not more complicated.

> > I guess I'll keep the patchwork instance up for now ...
> 
> I was basically the only one who used the IPA Patchwork any more. I have 
> stopped using it and I'm waiting for the Patch Reviewer field (in any 
> form!).
> Without someone to *manually* mark all the patches as On review, then 
> Changes Requested, then Pushed... Patchwork is quite useless.

I will eventually retire it for freeipa, but I am not satisfied by using
a field in trac.

> You can keep it running but no one from the FreeIPA team will use it. Sorry.

This is fine, nobody is forced to. I still keep it on for the SSSD
people which uses it so far.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York