cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rohit Yadav <bhais...@apache.org>
Subject Re: [DISCUSS][INFRA] Setting up gerrit
Date Sat, 09 Feb 2013 04:23:13 GMT
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