Revision as of 01:30, 9 August 2013

This page includes a list of things that reviewers should keep in mind when reviewing patches to OpenStack projects. There are some items that are common across all projects and others that are specific to a project.

Common Review Checklist

1. The code should comply with everything in HACKING.
2. The code should be 'pythonic' and look like the code around it, to make the code more uniform and easier to read
3. When adding a new file:
   1. If it's not installed by setup.py, but should be included in the tarball, be sure to add it to `MANIFEST.in`.
4. Commit Message and Change break-up
   1. Follow the advice of GitCommitMessages.
   2. Use the "DocImpact" tag on changes that affect documentation.
   3. Use the "SecurityImpact" tag on changes that should get the attention of the OpenStack Security Group (OSSG) for additional review.
   4. If the patch fixes a bug, it should reference a bug report.
   5. If the patch implements a feature, it should reference a blueprint. The blueprint should be approved before the patch is merged.

Oslo Syncing Checklist

1. When syncing from oslo:
   1. Ensure the proposed change has actually merged in oslo.
   2. It should provide some benefit, don't sync just to sync. Maybe it fixes a bug, or provides new functionality to be used. If providing new functionality it should have a dependent patch lined up to use that functionality.
   3. The commit message should list which oslo changes are being synced in.

Nova Review Checklist

1. For ReST API changes:
   1. If an API sample test template changes, ensure the doc samples are also updated.
   2. The change should conform to APIChangeGuidelines
2. When changing an RPC interface
   1. Make sure the version numbers are updated appropriately and that backwards compatibility is maintained. More info here.
   2. If an existing method is being updated, make sure support for sending the old version of the method is still supported. Search for existing uses of "can_send_version()" for examples.
3. Criteria for new compute drivers
   1. Should support everything in the core OpenStack compute API
   2. Should have public CI doing functional testing (http://lists.openstack.org/pipermail/openstack-dev/2013-July/011280.html)
4. When to approve
   1. Every patch needs two +2s before being approved.
   2. Its ok to hold off on an approval until a subject matter export reviews it, such as for neutron, xen, or glance related changes.
   3. If a patch has already been approved but requires a trivial rebase to merge, you do not have to wait for a second +2, since the patch has already had two +2s.

Notes for Non-Core Developers

• When you are reviewing, you may notice a review that has several +1's from other reviewers, passes the functional tests, etc. but the code still has not been merged. As only core developers can approve code for merging, you can help things along by getting a core developer's attention and letting them know there is a review with lots of positive reviews and needs final approval.