openstack team mailing list archive

[Rick Clark <rick@xxxxxxxxxxxxx>]

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

Attachment: signature.asc
Description: OpenPGP digital signature