On 02/20/2014 10:32 PM, Lukas Slebodnik wrote: > On (20/02/14 15:09), Martin Kosek wrote: >> On 02/20/2014 02:54 PM, Petr Spacek wrote: >>> On 20.2.2014 14:47, Martin Kosek wrote: >>>> On 02/20/2014 02:31 PM, Jan Cholasta wrote: >>>>> On 20.2.2014 13:14, 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. >>>>> >>>>> Is it possible to add new ticket states in Trac? I'm thinking extending >>>>> it so >>>>> that instead of "new -> assigned -> closed:fixed" we have "new -> >>>>> assigned:work >>>>> in progress -> assigned:patch available -> assigned:patch under review -> >>>>> closed:fixed", or something like that. >>>> >>>> It is possible to change the workflow, yes - this is something I was also >>>> considering. >>>> >>>> It can be done with this plugin: >>>> >>>> https://trac-hacks.org/wiki/AdvancedTicketWorkflowPlugin >>>> >>>> Unfortunately, the plugin that's in Fedorahosted Trac does not work >>>> properly, >>>> it gave me some Internal Errors. I filed a ticket for that: >>>> https://fedorahosted.org/fedora-infrastructure/ticket/4237 >>>> >>>> When it is fixed, we can try to think about adjusting the workflow. Maybe >>>> we >>>> can indeed add new states "submitted" and "onreview" to the workflow. But >>>> even >>>> then I think we could use the "Patch Review by" field so that we know who >>>> is >>>> reviewing, if anybody. >>> >>> Thinking a bit more the e-mail notifications ... >>> >>> We can reassign ticket to reviewer if we have state "onreview". IMHO ticket >>> owner always get e-mails, right? >>> >>> Nice side-effect is that report "{4} Assigned, Active Tickets by Owner" will >>> give you exact list of open tickets (state != new && state != closed) and >>> you >>> will see who is in charge for each ticket right in the report. >>> >>> Ticket owner is not set in stone and everything is still in Trac history >>> (and >>> Git, of course :-). >> >> Maybe. But that would mean that when you NACK a patch, you would need to move >> the ticket back to previous state to reset the owner. As I know how much you >> like the bureaucracy, are you sure you want this change? :) >> >> When the ticket is closed I personally like that owner is set to the >> implementer. As that way I quickly see who I need to yell when this ticket >> breaks something. >> >> IMO ideal state would be to have a workflow: >> "Accept as reviewer" - adds your name to reviewer field, moves to onreview >> "Assign reviewer: ________" - adds some login to reviewer field, moves to >> onreview >> >> We would just need to find out how to do this with some Workflow plugin when >> it >> is ready. >> > I could not resist.
That's fine, but you are still not the first SSSD dev who could not resist with this question :) > > This effort with the trac looks like reinventing wheel (gerrit or similar > tool) > > LS I am not saying it doesn't - we are partially reinventing a wheel. However, there were several discussions already about using a tool like Gerrit or Review Board and there was even a bigger push back against it. So moving one step forward to a home grown review system currently seems as our best option. As I said, if we see that it does not work and we start using other review tool, we can always delete this field in Trac. When ticket is closed, the real reviewer information is stored in the git commit anyway. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
