Hi Laca,
Thanks for the reply. Here is how I interprete the review/approval
process:
Just spell these out in pseudo code to be clear (for myself) :)
2 possible scenarios:
==============
(1) Simple patch (One that the submitter has confidence in)
- If simple patch then
submit patch for review and commit patch at the same time
if questions/remarks then
answer them
if changes requires or rejected by category owner then
undo commit and redo patch and resubmit patch
(2) Complex patch (One that submitter looking for input from reviewers)
If complex patch then
submit patch for review
wait for approval from category owner or questions/remarks
if approved by category owner then
commit patch
else if rejected by category owner then
resubmit patch
if questions/remarks then
answer them
if changes requires then
redo patch and resubmit patch
else if timeout (3 working days)
commit patch
So if seems to me in scenario (1), category owner approval is a
formality.
If these are your intended interpretation, then I am pretty happy :)
-Ghee
Laszlo (Laca) Peter wrote:
> Hi Ghee,
>
> Thanks for the comments, although I would have appreciated the comments
> coming earlier, when I posted it for discussion. That said, it's
> never too late to ask questions.
>
> Responses inline.
>
> On Mon, 2006-10-23 at 11:49 +0100, Ghee Teo wrote:
>
>> Hi Laca,
>>
>> Thank you Laca for putting together this nice and simple code review
>> process
>> http://www.opensolaris.org/os/project/jds/documents/code_review/
>>
>> I need some clarifications on these statements when taken together:
>>
>> * bug category owners must review all patches in their category and
>> respond. If the patch looks good and you have no comments, just
>> reply with "approve"
>> * reviews are non-blocking: you can go ahead and commit if you are
>> confident enough that the fix it good, but be aware that you may
>> need to revert or amend your changes if your peers or the upstream
>> community have issues with it
>> * you must address all questions or concerns
>>
>> Q1. Who is the final 'authoritive' approver? (e.g. bug category owner
>> approve a patch, but non category owner opposes it or raised questions
>> and concerns so is the patch approve or not?)
>>
>
> I'd really like a consensus, that's why all questions need to be
> answered/dealt with. If the category owners approves but someone
> else notices problems with the change, it still needs to be answered
> and/or fixed.
> If there is no agreement on the list (although code changes should
> not be political), the responsible manager decides (but I would really
> prefer to avoid that situation).
>
>
>> Q2. If the submitter is going ahead to submit the patch anyway, what is
>> the point of addressing all questions and concerns?
>> (Okay, I am over-stressing here a bit, but this is one possible
>> interpretation of the statements).
>>
>
> If the patch turns out to be bad, the submitted has to undo his/her
> changes.
>
>
>> With regard to this statement:
>>
>> * your changes are considered approved if no issues are raised
>> within 3 working days or if all issues raised are addressed
>>
>> This implies the lead time for a patch approval is 3 days. This is too
>> long.. I think.
>>
>
> This means, if you submit a code review request and noone reponds within
> 3 days, you can go ahead and commit and shame on the reviewers.
>
>
>> I think if the category owner has approved it then it is considered
>> approved, however, others can still raise concerns and questions
>> (regardless of time) that still need to be addressed but do not let the
>> process stopped.
>>
>
> Correct. The process is not stopped anyway. You don't even need to
> wait for the category owner's approval.
>
>
>> Ultimately any concerns raised on this list will most
>> likely to be raised by the community.
>>
>
> Right, that's why patches submitted to the community need not be
> reviewed on this list.
>
> Thanks,
> Laca
>
>
>> Laszlo (Laca) Peter wrote:
>>
>>> The JDS Core review process posted earlier is now in action.
>>> See http://www.opensolaris.org/os/project/jds/documents/code_review/
>>> for the description of the process.
>>>
>>> A new mailing list called jds-review was created and all Sun
>>> contributors were subscribed. Anyone is welcome to join at
>>> http://mail.opensolaris.org/mailman/listinfo/jds-review
>>>
>>> Everyone, please start submitting your svn diffs to jds-review
>>> prior to committing.
>>>
>>> Thanks,
>>> Laca
>>>
>>>
>>>
>>>
>
>
