One of the most important parts of any development process is probably its least-beloved: the code-review. Any project that entertains a notion of one day being stable and production-ready knows that it needs to do at least periodic reviews of the source.
There are many different approaches to the code review process. Some projects allow open commits during the first phase of development, then lock the tree and do in-depth code reviews, followed by a “hard freeze” wherein only reviewed and approved changes are further permitted. On the other end of the spectrum, many open-source projects follow the “hard freeze” party for their entire development cycle. For these projects (among which the SSSD is one), every patch is reviewed by at least one gatekeeper.
The former approach can serve a young project quite well. At the start of development, there are usually many small commits, few of which are functional or testable. By the time the code hits its first stable (or at least public) release, I suggest that it’s time to switch to the gatekeeper approach. The team should examine its membership for a very short list of gatekeepers. These will be the only members from there on that will have privilege to commit to the master repository. These individuals are the final say on what patches are accepted into a particular development branch.
This may sound a bit authoritarian. Perhaps it even sounds contrary to an open development process. I contend that stability should be the ultimate goal of any project, above and beyond “shiny features”. To ensure that this remains an open process, it becomes necessary for the code review process to be designed to be completely transparent. For example, it is usually not necessary for the gatekeepers to be the only code-reviewers on a project. In most cases, the reviews can and should be performed by any member of the team. The gatekeepers need be only a sanity check to ensure that the code-review was performed by a qualified individual.
Ok, so that’s all well and good, but how does one design a transparent and effective code-review system? There are two common approaches, which I will discuss briefly, and then I’d like to discuss a less-common approach that is gaining traction.
All of the common code-review processes involve somehow getting a patch file in the hands of a reviewer. One process favored by very well-established projects is the ticket-driven review process. For old and stable projects, it is common for the process to require that all changes to the source correspond to an issue-tracker ticket (such as a Bugzilla or Trac ticket). The code-review process will often be designed around this ticketing system. When a developer has a patch ready for review, it will be attached to the ticket and the appropriate flags/status will be set to indicate that it needs a reviewer. Potential reviewers will then be informed that the patch is ready (perhaps because a saved search will now reveal that the ticket is awaiting review). Code review comments and resubmissions will then be handled within ticket comments. Once the review is passed, a gatekeeper will take the patch and apply it to the master repository.
The second (and probably most popular) code-review process for open-source development is by email. When a patch is ready, a developer will email the patch as an attachment to an email sent to the developer mailing list for the project. Another developer will then take up the review, responding with an ack or nack (indicating corrections that need to be made). The gatekeeper will then take the patch and apply it to the master repository.
Both of these approaches have their merits and disadvantages. The ticket approach works well for well-established projects, providing easy history and a link to every issue or feature that the patches are meant to address. On the other hand, for young projects, the additional overhead of filing a ticket for every change can be unnecessarily heavy. Young projects tend to have a much more rapid development pace and filing a ticket for every change will usually serve only to train engineers to write short, worthless ticket descriptions.
The email approach is much more agile, but it is rather less transparent, as it requires all participants to be subscribed to the relevant list. It adds a barrier to entry for new contributors that may have only a single small patch to submit (and may not want to deal with a heavy-traffic development list). Further, the email approach adds additional effort on the part of the gatekeepers. It is necessary for gatekeepers to develop for themselves a system to ensure that patches are reviewed in a timely manner, as well as not forgotten.
To address these issues, some projects have begun looking to new code-review tools. As an example, I recently contributed to the packaging effort for the ReviewBoard tool. Tools such as this provide many benefits over the above approaches. It has built-in functionality to keep track of pending reviews and their status (approved, awaiting review, needs update etc.) Furthermore, they provide additional user interface enhancements, such as the ability to make review comments attached to the specific code that is being discussed. Such tools provide much greater visibility than the email approach, while retaining the same agility (especially if the tool is configured to announce updates to reviews on the developer mailing list).
As mentioned above, I’ve been working on packaging ReviewBoard for Fedora and EPEL, with the goal of making ReviewBoard available for projects hosted by the Fedora Hosted project. I think that the advantages provided by such a code-review system would contribute greatly to the security and stability of open-source software.