[Freeipa-devel] [RANT] Patchwork process

[Petr Viktorin]

On 11/02/2012 10:46 AM, Martin Kosek wrote:
> On 11/01/2012 07:28 PM, Simo Sorce wrote:
>> On Thu, 2012-11-01 at 10:59 -0400, Rob Crittenden wrote:
>>> Rob Crittenden wrote:
>>>> Simo Sorce wrote:
>>>>> From: Simo Sorce <ssorce at redhat.com>
>>>>>
>>>>> We check (possibly different) data from LDAP only at (re)start.
>>>>> This way we always shutdown exactly the services we started even if
>>>>> the list
>>>>> changed in the meanwhile (we avoid leaving a service running even if
>>>>> it was
>>>>> removed from LDAP as the admin decided it should not be started in
>>>>> future).
>>>>>
>>>>> This should also fix a problematic deadlock with systemd when we try
>>>>> to read
>>>>> the list of service from LDAP at shutdown.
>>>>
>>>> This fixes things for me but ipactl start isn't working reliably and
>>>> I've yet to determine if it is related or not (I'm thinking not).
>>>>
>>>> What I see is that it considers pki-tomcatd to not have started. What is
>>>> happening is the request to the getStatus URI is timing out and that
>>>> exception is being considered a "didn't start."
>>>>
>>>> I modified the code to treat a timeout as a 503 and it is still failing
>>>> because the timeout is so longer that it eats up all our normal timeout
>>>> for waiting for the service at all.
>>>>
>>>> I don't see a way to pass a timeout to the http request method, I'll
>>>> keep looking.
>>>>
>>>> I'm testing in F-18 btw.
>>>
>>> I can't reproduce the startup issues this morning, I still don't think
>>> they are related to this patch, so ACK.
>>>
>>> pushed all 3 to master and ipa-3-0
>>
>> You pushed an older revision for patch 2, I reverted it on both trees
>> and pushed the right one.
>>
>> Simo.
>
> While trying to follow this thread, I must throw my 2 conservative cents in.
> This thread is a very good example why processing our patches via patchwork is
> IMHO rather fuzzy and can cause more confusion than clarity. These are my main
> points:
>
> I cannot see which patches are the recent ones. Each patch is sent separately
> in mail body and I cannot quickly see by looking in thread which mail contains
> a revised patch and which is just a comment.
>
> When patches are attached directly to mail as files, I can quickly distinguish
> which mail has new patch set as it has "the paperclip icon". It also enabled us
> to attach a whole set of valid patches to one mail and thus make it easier for
> reviewer to pull a whole set of valid patches in one click.
 >
>
> Patchwork style patch sending also generates a lot of mails which may confuse
> not only me but other contributors...

+1

This particular thread is extremely confusing. Each individual patch 
generated its own subthread, and then the revised patches were sent as a 
reply the original, spawning new threads to continue the old ones. The 
discussion is broken up and impossible to navigate. You can't see where 
the patches are, let alone where to look for latest versions.
Thanks to Rob for showing us that even a person following the thread is 
easily confused.

Patchwork adds a view that is incomplete and not cross-linked.
To make a comment on a patch, you need to do it by e-mail; finding a 
patch in Patchwork is useless for this. Same for assigning a reviewer: 
you need to find it "manually" in Patchwork even if you have the mail in 
front of you.


I don't really see Patchwork's advantage. For a list of "active" 
patches, we have the "Patch submitted" bit in Trac; to claim a larger 
review we could have a policy to announce this on the list. And the 
claim that Patchwork doesn't change our current practices is simply false.

-- 
Petr³