I think I agree with both of you! In the end, it's a social problem, of actually getting round to doing the reviews: but better tools can help to encourage that. For example, on github, the code review is integrated into the source control: every change is a pull request, and someone has to press the big green button, which acts as a sign-off. Whereas, Review Board may be slightly klunky and adding friction into the process, thus deterring people from doing code review. But even given the best tools in the world, someone still has to do the work...
-- Stephen Turner -----Original Message----- From: Trippie [mailto:trip...@gmail.com] On Behalf Of Hugo Trippaers Sent: 11 June 2014 10:52 To: dev@cloudstack.apache.org Subject: Re: [DISCUSS] Introducing Gerrit for quality? was: [PROPOSAL] Using continuous integration to maintain our code quality... Sheng, In the end we both want the same thing, which is a higher quality codebase. However i find the technology first approach the wrong approach in this case. Of course supporting tools are needed to make the process more efficient. My main worry here is that the "experts" are not reviewing commits now, why should they start when we have a system like Gerrit in place. As long as all our committers don't take the time to review commits coming into CloudStack, no amount of tooling is going to help improve the code base. It is my opinion that such tooling will not be beneficial to our community unless there is a great willingness to review commits, which there isn't at the moment. Most of the bugs are currently found during the QA phase, which means that they were not found by our committers who have the responsibility to review every incoming commit. (Don't get me wrong, i'm blaming myself as much as anybody else) The next point is the concept of experts. We ask people to become committers because we trust them to act responsibly with regards to the code base. That means they know what they should or shouldn't commit. We should not introduce another level of committers without very carefully thinking about what the criteria are for that particular level and what the responsibilities are. In short, lets show the community that our "experts" are actively reviewing incoming commits and start improving the code base today so we can support them with additional tooling tomorrow. These are some things that we can do without having gerrit and/or PR. * Discus commits on the dev list openly so other people can learn from commonly made mistakes. * Review changes and branches proactively and engage with the contributors * Document intricacies in the codebase that will likely stump new committers. * Refactor code that is not easily maintainable. If we get there, i'm all for setting up whatever tooling we need to facilitate this, but don't let the absence of such tooling be an excuse for not doing what we should be doing. Cheers, Hugo P.S. This is an entirely different discussion than replacing RB with another tool for contributors. Completely +1 to using github PRs or Gerrit for new contributions. On 11 jun. 2014, at 00:56, Sheng Yang <sh...@yasker.org> wrote: > Hi Hugo, > > The main point I want to bring up an automation tool here is, enforce > mandatory review process and test(regression test probably) to happen > before commit. That would slow down the process of course, but it > should greatly help on code quality. > > Even committers would make mistakes from time to time. By adding and > enforce the review process, we would able to a. reduce the obvious > bugs or potential impact on issues author didn't know about, b. > transfer the knowledge for the project. I really think CloudStack > would going to be a more mature project in the future, but our current > develop process is too random and fail to take the whole control of > the quality. By specifying people in the area to review we should able > to spot error better in the early stage. > > I know integrating the tool would change how people working on > project, since review and testing would be a necessary part of it. The > problem now is a. committers can still push bad commits without > reviewing by experts in the subsystem, b. current reviewboard is too > primitive and overhead for uploading, updating and applying the patch > is too big, and c. reviewboard is not mandatory. I would like to have > devs spend more time on reviewing, by this way at least we can gain a > kind of control over the code base we're working on. > > And correct tool would be important to cut the overhead of our > development process, which can make people more focus on real work. > > In conclusion, I'd like to advocate the enforced(by tool) mandatory > review(and testing integration of course) for every commits, and I > think the correct tool is important in this case. > > --Sheng > > > On Mon, Jun 9, 2014 at 11:22 PM, Hugo Trippaers <h...@trippaers.nl> wrote: > >> Hey, >> >> I'm all for automated solutions. I'm a happy gerrit user on some >> other projects and quite fond of working with Github pull requests as well. >> However there is one important point that makes working with those >> tools work and that is a willingness by the committers to review >> requests. Both systems rely on either a well functioning and fast CI >> system or committers that consistently and rapidly review requests. >> Where the latter is actually the most important one. >> >> Both gerrit and pull requests do not improve quality. They are just >> tools to facilitate a certain way of working. If we want to improve >> quality we have to do it ourselves, no amount of automated tooling is >> going to solve it for us. As committers it is our job to review >> commits and make sure that quality is maintained. It is also our job >> to make sure that automated tests exist that will catch problems. >> >> At the moment we have a 433,412 line codebase with on average 3.91 >> potential defects per line of code (according to coverity). We have a >> very small amount of unit test coverage on our core code and no real >> idea how much or what code is covered by functional testing. If we >> want to improve quality i think that is the place to start. >> >> Of course it is also wise to see if we can improve the quality of the >> incoming commits, but that is easily done by taking a few moments >> during the day to review everything that was pushed to master and >> fix, revert and add unit tests where required. Coach >> committers/contributors that consistently have trouble with adding >> testing cases on how to do it. That part is the responsibility of >> being a committer, not just the bit that allows access to the repo. >> >> If we are able to get this bit going, i'll happily jump on any >> barricade and start a revolution to get whatever automated tooling we >> need to support this process. >> >> Cheers, >> >> Hugo >> >> >> >> On 10 jun. 2014, at 06:15, Rajani Karuturi >> <rajani.karut...@citrix.com> >> wrote: >> >>> +1 for github pull requests. They are much better and cleaner than >> review board. >>> >>> ~Rajani >>> >>> >>> >>> On 09-Jun-2014, at 9:17 pm, David Nalley <da...@gnsa.us<mailto: >> da...@gnsa.us>> wrote: >>> >>> On Fri, Jun 6, 2014 at 7:26 PM, Sheng Yang <sh...@yasker.org<mailto: >> sh...@yasker.org>> wrote: >>> Hi all, >>> >>> Seems it's a good timing to bring back the discussion about the gerrit. >>> >>> We want to do CI, and improve our code quality. One obvious way of >>> doing and reduce the workload of devs is introduce a tool to enforce >>> the >> process. >>> >>> I've checked out quite a few projects using gerrit, which would >>> force you to ask for review, and validation before the code can be >>> committed to the repo. Looks it's really a easier way for devs according >>> what I've heard. >>> >>> Even our competitor laid out a very detail workflow based on the use >>> of gerrit( https://wiki.openstack.org/wiki/Gerrit_Workflow ). I >>> guess it >> can >>> make a good reference. >>> >>> Well, gerrit has been brought up a few times before. And now the new >>> process we want to enforce just fits what gerrit(or other automation >>> review/test/commit software) is for. >>> >>> Maybe it's the time for us to review the possibility of using a tool >>> to enforce our commits and improve our code quality(as well as >>> transfer >>> knowledge) again? >>> >>> --Sheng >>> >>> >>> ASF Infra has a very dour view on Gerrit. Don't read that as >>> impossible; there are many projects at the ASF who are interested in >>> Gerrit. >>> That said; what about moving to using github pull requests instead >>> of RB, and from their, having the jenkins pull request builder >>> automatically process every pull request and list information. >>> >>> Here's an example: >>> https://github.com/jclouds/jclouds-labs/pull/61 >>> You'll see that every time the patch changes, the jenkins plugin >>> pulled the patch - ran tests against it and reported back. >>> >>> That said; it almost seems like we have the cart before the horse; >>> we need to finish figuring out the CI Infrastructure first. >>> >>> --David >>> >> >>
