[Freeipa-devel] Reviewer in Trac

Simo Sorce simo at redhat.com
Thu Feb 20 15:09:58 UTC 2014


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.

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




More information about the Freeipa-devel mailing list