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. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
