WorkingWithReviews

Not logged in - Log In / Register

Revision 8 as of 2010-01-28 18:59:27

Clear message

This page covers the process for effectively dealing with code and design reviews.

Overview

During the development of a feature from concept to rollout there are a number of reviews that occur. The first review process is the SpecificationProcess where the concept is examined and refined into a plan. This plan is eventually signed off by the appropriate manager. Some time later, a coder will start to implement it. When they are about to do this a DesignPhoneCall takes place, which is a verbal review of the design of the planned implementation. Finally, once the code is complete the PreMergeReviews process takes place.

Specification reviews

Please see SpecificationProcess for the specification workflow.

Design reviews

Design reviews are conducted by a DesignPhoneCall between a coder and a reviewer. The review aims to ensure that the resulting code will have few or no structural problems, which makes review of the code easier and faster.

Code reviews

Once your code is complete, it must go through the PreMergeReviews process to be accepted into the central trunk. See https://help.launchpad.net/Code/Review to learn about Launchpad Merge Proposals.

Handling review responses

The story starts with you making a Launchpad Merge Proposal into launchpad-devel or launchpad-db-devel. From there the branch is queued for review at https://code.launchpad.net/launchpad/+activereviews . You may ask an on-call reviewer in #launchpad-reviews to review the branch outside of the queue.

Once your reviewer has had time to go over the modifications, he will reply to the proposal with a description of areas or items that need to be fixed, modified or improved in your code. He may also write special recommendations, commend you for your code quality, or ask questions if unclear on domain specifics or obscure functionality.

You must respond to each individual suggestion and question made by your reviewer; one of the main dangers with reviews is wasting the reviewer's time by ignoring or forgetting to deal with some specific issue, and the reviewer can't be expected to go through and ensure each item was dealt with --- please make an effort to ensure you respond accurately to reviews.

One or more review cycles will be necessary per-modification; the number of cycles will depend on the amount of code changed, the increasing familiarity of the reviewer with your code, and of course the quality of the code produced.

You may only merge once the reviewer has approved the modification and you have made all changes that were requested. If you want to make a case why a specific suggestion should not be followed then you must bring it up with the reviewer and not merge until agreement has been reached and documented in the merge proposal.

Small branches are faster to review than large ones - a branch that is twice as big as another takes more than twice as long to review. Please try to keep branches small and focused.

Mentored reviews

When developers apply to become Launchpad reviewers they are assigned a mentor. We began referring to the person being mentored as a mentat, a made-up word you may see in reference to mentoring.

If your merge proposal is reviewed by a mentat his/her mentor will need to review the review. The mentat should add his mentor to the merge proposal. You must wait for both to approve the merge proposal before merging it.

UI reviews

If your branch changes the UI in any significant manner (even text rewordings) you'll need to seek a UI review. The UI reviewers are listed on ReviewerSchedule under the specialties column. If your UI reviewer is being mentored you'll need to follow the procedure outlined above to ensure the UI review is properly reviewed.

Dealing with Conflicts

If, after a successful review process, you submit your code to PQM only to find conflict you need to resolve the conflicts:

  1. Merge rocketfuel onto your reviewed branch,
  2. resolve the conflict,
  3. push, and
  4. submit again to PQM.

If, in the process of resolving conflicts, you make non-trivial changes to the code, you should consider having those changes reviewed before attempting to merge them.

The merge message

Each merge message must include an item indicating who the reviewer of the change was. This is the general form of a commit message.

Make it so

When you are ready to submit your branch to pqm for merging, use the following command (from RocketFuelSetup):

bzr pqm-submit -m '[r=reviewer] description of changes'

You can simplify the process of running tests on EC2 and landing your branch by doing the following:

  1. Get your merge proposal reviewed and in the 'Approved' state,
  2. Set the 'Commit Message' on the merge proposal to be the description of the branch, as above but without the reviewers and bug information. (Only the part above marked 'describe the change'.)
  3. % utilities/ec2 land -- voila, that's it!

  4. Enjoy a snack, a nap, or take in a movie while ec2 runs the test suite and then submits your branch to PQM.

Note: ec2 costs real money and only Canonical employees are currently allowed to submit to PQM. For those reasons community contributors must find a Canonical developer to test and land their changes. Usually you can arrange for your reviewer to do it but do not assume he will -- explicitly ask him to do so when the review is approved.