openstack team mailing list archive

[Jay Pipes <jaypipes@xxxxxxxxx>]

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
>
>