wiki:CodeReviewProcedure

Overview

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

Normal Reviews

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. There is a ticket associated with the branch, which is usually named "tracNNNN", where NNNN is the ticket number. If this is not the case, then the branch name needs to be mentioned in the ticket.

Starting the Review Process

When a coder has a piece of code that is "done", then they must set the ticket "Action" for "review to" to UnAssigned in the drop-down list (or assign it to a developer who has agreed to review it).

During a sprint, the developers will review the tickets to see what review tickets are unassigned.

The coder can ask someone to become the reviewer, or ask the manager to pick someone to review the code. When the manager does this, the ticket will be assigned and an explanation added of what is expected; this will cause an e-mail from Trac to be sent. It is not necessary to copy the manager on any follow-up mails between the coder and reviewer.

IF YOU ARE THE OWNER OF A TICKET, IT IS WAITING ON YOU TO DO SOMETHING.

Reviewing the Code

When the reviewer is ready for the code, the ticket is accepted via Trac site, which changes the ownership.

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

A short checklist:

  • General
    1. Building and testing the code should work ("make distcheck" will perform this check). (Also run lettuce or other system tests as applicable.)
    2. The code must have Documentation (for the coder or user as appropriate)
    3. The CodingGuidelines must be followed
    4. There must be automated unit tests
    5. Tests should avoid introducing delays, such as sleep(), as much as possible.
    6. Installation should work
    7. In test cases and examples in documentation, documentation IPv4/IPv6 prefixes (192.0.2.0/24 and 2001:db8::/32) should be used.
    8. If the change would require a ChangeLog entry, the entry must be prepared before the review and it must get reviewed, too. See ChangeLogDetails for the ChangeLog entry format. (see also the relevant discussion on the dev list: https://lists.isc.org/pipermail/bind10-dev/2010-June/000984.html)
  • C++ Code
    1. Is the code exception safe? The reviewer should be careful especially about allocating resources by operator new or low-level system interfaces such as (socket(2)).
  • For API's
    1. Does the API make sense?
    2. Are there functions missing?
    3. Are there functions unnecessary?
    4. Does the documentation specify exceptions and memory allocations?
  • For code, read through the source code and try to find anything that might 'not work' or is not understandable. Some hints:
    1. Are there any internally undocumented weird steps performed?
    2. Are there hard-coded constants that should have a defined name?
    3. Are design choices written down?
    4. Are branches, switch statements and loop conditions correct?
    5. Do a git grep <ticket-number> in the codebase at toplevel and see if any results show up. Developers usually leave comments for things to be done after a particular ticket XXXX has been fixed. These may be small enough to do as part of the current ticket itself, or a new ticket may have to be created for it.
  • Documentation
    1. When you add/delete/modify a configuration/command option to a program, update the corresponding XML manual (for example kea-dhcp6.xml) with an example of how to specifically configure it.

Anything about the code is fair game, as long as it fits in the ISC 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.

Comments about the review should be added to the ticket.

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. When the review is complete, the developer may choose to copy-and-paste the above checklist in the reply and note "YES" for each item checked or explain why not checked or about any issues.

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.

The code is then ready for merge. Branching and merging procedures can be found in the GitWorkflow and GitGuidelines pages. 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.

Deadlines

The reviewer should provide feedback within 5 working days of receiving the review. For a complex review, the review may take longer than 5 days, but the coder needs to know that someone is working on it.

Review of Minor Changes

Some changes are relatively minor, for example:

  • A missing #include statement that only affects some systems, discovered by the build farm
  • Typos in comments or documentation

In these cases, it is more work to create a ticket and a git branch on the repository than it is to make the fix. The developer who makes the change may prefer to ask someone via the ISC dhcp jabber room, direct jabber chat, or via phone call.

Once the reviewer has looked at the change, it can be committed to the master, with a note explaining how the review was done.

About this Document

Created by Shane 2009-12-18
Reviewed on 2014-01-29
Review scheduled 2015-01-29

Last modified 3 years ago Last modified on Nov 12, 2014, 3:46:08 AM