More than thoughts, I have a question: what criteria are selected to order the merges under review? Length of the diff/priority of the blueprint etc?
Thanks, Armando > -----Original Message----- > From: openstack-bounces+armando.migliaccio=eu.citrix....@lists.launchpad.net > [mailto:openstack- > bounces+armando.migliaccio=eu.citrix....@lists.launchpad.net] On Behalf Of Jay > Pipes > Sent: 21 December 2010 17:48 > To: Rick Clark > Cc: openstack@lists.launchpad.net > Subject: Re: [Openstack] Merge proposals and criteria for approval > > FWIW, I agree with all your points on this, Rick. > > On Tue, Dec 21, 2010 at 11:43 AM, Rick Clark <r...@openstack.org> wrote: > > As most of you know, the Nova merge queue is starting to back up. In an > > effort to help clear the log jam, I would like to propose some criteria > > for how we do code reviews and what we approve. > > > > Statement of Fact: All code has bugs. Bugs are normal and can have no > > impact, or can be critical. We have all types in our code > > > > Statement of Fact: Regressions, bugs that break previously working > > features, are bad, and should be avoided. > > > > I think that we should focus on finding regressions at review time not bugs. > > > > I propose that the following criteria be used for approving code reviews: > > * Architectural soundness > > * regression free > > * Code cleanliness (Pep8 compliance) and style > > * Complete test coverage > > * Documentation > > > > > > Any obvious non-regressing bugs should be filed in Launchpad at review > > time by the reviewer. > > > > Basically I think this will solve a couple problems. We spend quite a > > bit of time in an iterative process with a patches author fixing issues > > in a patch. Some of these need to be fixed by the original author prior > > to merge, but many do not. As soon as a patch is merged it is owned by > > everyone and anyone can fix the issues. This also will allow new > > developers to grab relatively simple bugs from merges to cut their teeth > > on the code. > > > > I would also say that breaking a test is not a regression it is a bug. > > If a merge breaks a test, but does not break the actual feature it is a > > bug in the test. But I think this is more controversial, so I'm just > > bringing it up for discussion and don't propose any changes. > > > > > > Thoughts? > > > > Rick > > > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~openstack > > Post to : openstack@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~openstack > > More help : https://help.launchpad.net/ListHelp > > > > > > _______________________________________________ > Mailing list: https://launchpad.net/~openstack > Post to : openstack@lists.launchpad.net > Unsubscribe : https://launchpad.net/~openstack > More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
