Code review required for commits - formal Team vote.

[Ira Cooper]

That's a fine question:

There is no "integrator" on master.  Only on the release branches,
Today what we send to master is what goes in.  Thus this discussion
:).

Cheers,

-Ira

On Sat, Oct 13, 2012 at 4:05 PM, Elia Pinto <gitter.spiros at gmail.com> wrote:
> 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