Code review required for commits - formal Team vote.

[Elia Pinto]

Sorry for the top posting.

This thread is very intesting to me, a stranger, but a very old samba
user. Iirc in the past samba have a release manager, probably this
role exists today, i am sure. Using git as a dvcs, don't have the
samba project also a  figure similar a integrator, or someone that
tell, well this is ok to merge ? Sorry for the naive, hope not silly,
question.

Best

2012/10/13, Ira Cooper <ira at samba.org>:
> On Sat, Oct 13, 2012 at 12:05 PM, simo <idra at samba.org> wrote:
>> On Sat, 2012-10-13 at 11:34 -0400, Ira Cooper wrote:
>>> On Sat, Oct 13, 2012 at 7:28 AM, Christian Ambach <ambi at samba.org>
>>> wrote:
>>> > On 10/12/2012 07:48 PM, Jeremy Allison wrote:
>>> >
>>> >> --------------------------------------------------------
>>> >> No team member commits their own code.  All code will be "signed off"
>>> >> by two team members, as a team member you may sign off your own code.
>>> >> The "non-author" team member will be responsible for pushing the
>>> >> code.
>>> >> If there are two they can agree among themselves.
>>> >>
>>> >> Tools for code review to be decided amongst reviewers, but any
>>> >> patch posted to samba-technical is an explicit request for review.
>>> >>
>>> >> Release branches work as they do today, though the + should be
>>> >> treated
>>> >> as a sign-off in bugzilla. All release branch bug commits must
>>> >> contain
>>> >> their bug number in them, so we can track back what happened.
>>> >> --------------------------------------------------------
>>> >
>>> >
>>> > In principle, I would vote +1 for this proposal.
>>> >
>>> > But I can also understand some of the reservations that others have,
>>> > especially if it comes to patches to areas of the code that are not
>>> > under
>>> > regular development near to being un-maintained at all.
>>> > It will be difficult to find a second reviewer for this.
>>> >
>>> > Other areas might be special VFS modules that are made for certain
>>> > client
>>> > applications (like catia, mediaharmony) or uncommon platforms (e.g.
>>> > OpenAFS).
>>> >
>>> > So I'll put my vote on hold until the concerns have been resolved.
>>>
>>> Well put sir.
>>>
>>> I agree with your views, and share your concerns, and vote "I haven't
>>> voted yet."
>>>
>>> My concern follows, and may help some of yours and others:
>>>
>>> I'm also concerned that patches will fall through the cracks.
>>>
>>> There must be a way that we can know what reviews need to be done
>>> easily, and monitor the situation.  I don't know how to do this, but I
>>> think we should.  Just so we actually make sure the obscure patches
>>> get reviewed.
>>>
>>> I know at work, I have a website I goto, and it lists my reviews I
>>> have to get to.  That's handy, but it is not the requirement.  A
>>> command tool, a radio station, samba branded carrier pigeons, anything
>>> will work.  It merely needs to exist.  (I believe this echoes some of
>>> Matthieu's concerns.)  Given that I think this requirement can be
>>> solved about 18,000 ways.  It is not a "show stopper."  But it is a
>>> "release blocker" shall we say.
>>
>> Ok this could be easily done by having a 'review' git branch for each
>> team member where patches are pushed.
>> Should be easy then to fetch trees in the morning and see what's
>> pending, unless people leave the branches unmaintained.
>>
>> The other option could be bugzilla, but that is a bit more heavy handed.
>
> I 100% left how to do it off intentionally, it is polarizing.  I
> merely want agreement that we WILL do it.  How can be voted later.
> (Given that I suspect someone will just jump on the sword short term.)
>
> Interestingly, you didn't mention 2 approaches I thought of:
>
> 1. Use a review tool.  (Gerrit, ReviewBoard, whatever... I'm not that
> picky.)
> 2. Have a simple set of scripts on sn-devel that know the topics of
> the outstanding e-mail to be reviewed.  (I'm sure you can see that set
> of scripts isn't that hard to write for us if we want it.)
>
> The big thing I've seen so far is: A central repository for the
> information, and a common way to query it.
>
> But if you can show me it can be done w/o that.  All the better!
>
> The big thing: If we can't measure it.  It doesn't exist.
>
> And that goes for stale patches, and other such things.  Also I
> suspect many of us would run multiple review branches, etc at times.
> I don't expect people to have 1 review at a time up.  Most will be 0,
> some may be 5.
>
> -Ira
>

-- 
Inviato dal mio dispositivo mobile