-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: rohityada...@gmail.com [rohityada...@gmail.com] on behalf of Rohit Yadav [bhais...@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 <da...@gnsa.us> wrote: > On Fri, Feb 8, 2013 at 2:06 PM, Alex Huang <alex.hu...@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
