On 02/20/2014 04:55 PM, Simo Sorce wrote:
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
Yeah, one part is automatic. That's not enough.
Patchwork:
patch arrives: nothing
mark self as reviewer: use web interface
send review: reply, find patch in Patchwork, mark status
send fixed patch: send the mail, find patch in Patchwork, mark status,
find old patch in Patchwork, mark old patch as superseded
patch pushed/superseded: find patch in Patchwork, mark as pushed
(where "find patch in Patchwork" is frustrating when done so often)
Mail+Trac:
patch arrives: tag message TODO when it comes in (1 keystroke)
mark self as reviewer: use web interface (or API)
send review: just reply ("changes requested" status is not kept though)
send fixed patch: send the mail
patch pushed: move from the push messsage to the tagged message (~1
keystroke), untag message (1 keystroke)
replys are also inline, so you know if a specific version of a patch has
got replies (there are caveats I know).
That assumes I read the replies in patchwork.
Obviously Patchwork can't hope to be a better mail client than my
favorite mail client.
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.
They don't have to. But if they forget their patches *and* we manage let
it slip, all that means is that nobody cares.
Anyway, with the amount of outside contributions we're getting, this is
premature optimization.
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.
Not a silver bullet, but better than Patchwork I'm afraid.
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.
Sounds good. Thanks for the experiment, it just didn't work out for us.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
