[Freeipa-devel] Reviewer in Trac

[Martin Kosek]

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