wiki:CodeReviewProcedure

Version 3 (modified by shane, 8 years ago) (diff)

--

Overview

All BIND 10 code must be reviewed by another coder on the BIND 10 team before it may be merged into the main trunk.

Typically a piece of code will be created in a branch, and worked on until it is thought to be ready by the coder working on it. When this happens it is reviewed, as described below.

Starting the Review Process

When a coder has a piece of code that is "done", then they must:

  1. Create a ticket on the Trac site describing the code that is to be reviewed
  2. Announce that the code is ready for review.

The announcement can be done either by sending something to the development mailing list, bind10-dev@…, or by mailing the Programme Manager, shane@….

The Programme Manager will pick someone to review the code, and send an introduction e-mail including both the coder and the reviewer. It is not necessary to copy the Programme Manager on any follow-up mails between the coder and reviewer.

Reviewing the Code

When the reviewer is ready for the code, he or she takes ownership of the ticket on the Trac site.

The code should be reviewed to make sure it is clear, correct, and follows BIND 10 coding guidelines.

A short checklist:

  1. The code must have Documentation (for the coder or user as appropriate)
  2. The CodingGuidelines must be followed
  3. There must be automated unit tests

Anything about the code is fair game, as long as it fits in the BIND 10 style and is intended to make the software better.

Any questions or concerns should be added to the ticket, and then ownership passed back to the coder.

Iterating

The ticket may pass back and forth between the coder and the reviewer several times as it is discussed. This is quite normal, especially for large and/or complicated tickets.

Ticket ownership matters! A reviewer may make several comments, especially in a long review process. The signal to the coder that a given review pass is complete is getting ownership of the ticket back. Likewise when the coder thinks that all of the issues have been addressed, the signal to the reviewer is getting ownership of the ticket back.

If problems arise in the process - for example due to unclear guidelines about how things should work, or disagreements about what the best way forward in a given case is - then the issue can be brought to the mailing list or to the Programme Manager.

Completing the Review Process

When all issues have been resolved, the reviewer adds an entry to the ticket noting that, and passes ownership back to the coder. 1 The code is then ready for merge. This is normally done by using Subversion to get a diff, which is then applied as a patch to the main trunk. The ChangeLog file is updated. After this merge is complete, the ticket may be closed. Depending on the scope of the change, either a congratulatory cup of caffeinated liquid or a bubbly alcoholic beverage may be imbibed.

About this Document

Created by Shane 2009-12-18
Review scheduled 2010-06-18