On Thu, Feb 20, 2014 at 10:15:23AM -0500, 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".
> > > 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.
> 
> > 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.
> 
> I guess I'll keep the patchwork instance up for now ...
> 
> Simo.
> 

Oh, another thing we lose from Patchwork is the "Changes Requested"
state of the review. In other words, with Trac you only know who is the
reviewer, but you don't know who is supposed to act now.

btw all this bending of Trac makes me wonder if we shouldn't make the
leap to some completely different review tool rather than trying to
force Trac into something it's not?

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to