openstack team mailing list archive

[Armando Migliaccio <Armando.Migliaccio@xxxxxxxxxxxxx>]

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.com@xxxxxxxxxxxxxxxxxxx
> [mailto:openstack-
> bounces+armando.migliaccio=eu.citrix.com@xxxxxxxxxxxxxxxxxxx] On Behalf Of Jay
> Pipes
> Sent: 21 December 2010 17:48
> To: Rick Clark
> Cc: openstack@xxxxxxxxxxxxxxxxxxx
> 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 <rick@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~openstack
> > More help   : https://help.launchpad.net/ListHelp
> >
> >
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp