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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
