incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <HTrippa...@schubergphilis.com>
Subject RE: [DISCUSS][INFRA] Setting up gerrit
Date Sat, 09 Feb 2013 17:32:45 GMT
-1

I'm all for peer review, let me get that straight. Part of the responsibility of being a committer
is to ensure the quality of the code that goes in, however i don't think that have a formal
process like gerrit is necessary. We are all looking at the code and the commits that are
coming in which is our responsibility as a community. 

Of course at the moment it's really hard to see what is going on in the code. The committers
committing code is not the problem, that lack of testing is. If we work hard to ensure that
code it properly tested both on a unit level and on a functional level i don't feel i have
to look at it before hand. The committer status is granted based on the trust we put in a
certain individuals to take care of the CloudStack project, for me that included taking the
responsibility that any contributions are up to spec. I want to trust my fellow committers
that they know what they are doing and i don't feel the need to second guess that by wanting
to look over their code before they can commit it.

Next to that is a very practical problem that there are not they many developers in my timezone,
meaning that i would regularly have to wait for a day to get the two +2 votes. Also a large
majority of the developers work closely together and a natural tendency would be to trust
developer and there is a very real risk of blindly accepting changes because they come from
a trusted source. It might be hard to objectively judge the contributions from the guy sitting
next to you. Another angle here is that people will more likely commit less tested code, because
somebody is going to look at it anyway, it's being tested for me by somebody else.

Considering these two factors i think it would be a very bad idea to do this now. It would
reduce the momentum we have in our community now and potentially discourage people from contributing.
For me part of being a committer is that i know i have to personal responsibility to my fellows
in the community to deliver good quality code.

My last point is that people would have to free up time to actually go through the commits
on a regular basis. Nothing more frustrating than committing something and then having to
wait until somebody finds the time to look it over and press a button. I would rather have
everybody working on writing test cases and unit tests than manually checking commits in a
review tool.

Cheers,

Hugo


________________________________________
From: rohityadav89@gmail.com [rohityadav89@gmail.com] on behalf of Rohit Yadav [bhaisaab@apache.org]
Sent: Saturday, February 09, 2013 5:23 AM
To: David Nalley
Cc: Alex Huang; cloudstack-dev@incubator.apache.org
Subject: Re: [DISCUSS][INFRA] Setting up gerrit

On Sat, Feb 9, 2013 at 1:21 AM, David Nalley <david@gnsa.us> wrote:
> On Fri, Feb 8, 2013 at 2:06 PM, Alex Huang <Alex.Huang@citrix.com> wrote:
>> Hi everyone,
>>
>> I like to propose that we setup gerrit as the review mechanism.  Here are my reasons
>>
>> - Committer status in Apache is a reflection of one's commitment to the community,
not a reflection of understanding of code.  So to me just because you have committer status
shouldn't mean code does not need review.  Chip's been doing a great job monitoring the merges
and commits but one person handling all that just means things will slip through.
>> - This also has the side effect of contributors' code contribution to be treated
as a second class citizen with delays in reviews because review is not common place within
our community.
>> - Direct commits have to be reverted if there are -1 votes, directly impacting the
time and effort of the code contributor.
>>
>> It makes a lot of sense to make code commit and review a normal process in the CloudStack
Community.
>>
>
> So while I love Gerrit, and would love to see it implemented, most of
> what you've described above are social/cultural problems, not
> technical ones, and thus a technical solution is unlikely to fix those
> things. Gerrit also requires that there actually be tests in place to
> test, othewise it's just another hoop to jump through. With a whopping
> 3% of classes covered, it doesn't really mean much at this point,
> unless we spend significant amounts of time building tests.
>
> Speaking from a technical perspective - there is no ability to stop a
> committer from committing to the repo. And yes, committer status is a
> reflection of ones commitment to the project, and also of the trust we
> have in a person. That doesn't mean a committer is perfect.
>
> This is also a _dramatic_ change in culture - moving from a commit
> then review to a review then commit - this will dramatically slow down
> project development. I'll note that we have past guidance from our
> mentors that encouraged CTR as opposed to RTC.
>
> So let me toss out an example (and I am assuming you'll be requiring
> two committers/reviews to approve changes, because if we are just
> talking about publishing changes based on automated test infra, that's
> not much improvement over our current method due to our low test
> coverage ):
>
> If Hugo makes some packaging changes (he's made ~30 distinct commits
> around packaging within the past week - on two different branches (so
> 60 total commits)) he then needs to get two reviews for each of the
> those commits - which means he either has to work in isolation on his
> own to land massive changes that get reviewed by two people, or he has
> to get 120 reviews for his changes. Then lets talk about who is
> qualified to really review those commits. That's a dramatic subset of
> committers. And then I'd ask - what has the 120 reviews given us?
> Massive improvement?

While I in general agree, this means our committers would have to
inculcate habit of reviewing others code. There are teams and projects
who do this, while it may seem like it would slow us down, over time
our committers will be more cautious with the changes they are trying
to commit and besides, they can commit directly but if any review gets
-1 that commit gets reverted as Alex mentioned in 3rd point.

So, we can have best of both the worlds, if committer knows what s/he
is doing they can commit directly (which can be bugfix, buildfix, or
packaging work done by Hugo and whatnot) which may be reverted if that
commit gets -1, else in general committers should go through gerrit.
In many cases, I can think that a commit may only be evaluated by the
committer him/herself in those cases s/he can go forward and commit
directly. Using gerrit instead of reviewboard in present workflow
(commit first review later that we see for committers and review first
on RB for non-committers) would be much better anyway.

Regards.

>
> So in short, what we are really talking about is dramatically ramping
> up the number of reviews required to get something into ACS, and we've
> already demonstrated that we aren't capable of timely getting things
> reviewed in reviewboard, what makes us think that dramatically
> increasing the number of reviews needed to see progress is a good
> thing?
>
> -1 from me, though I am open to being persuaded, because I really do
> like Gerrit.
>
> --David

Mime
View raw message