OIT Code Review Standard¶

Scope: OIT
Type: Standard
Version: 2023-initial

Goal¶

Establish a standard that code is reviewed prior to use.

Ownership¶

Direct questions to the Owner: Seth A. Roby email redacted

Resources to comply with this standard should be directed to your supervisor.

Timeline & Enforcement¶

All teams should be compliant with this standard by the close of 2023.

Exception Process¶

Exceptions that do not meet the standards by the deadline MAY petition the ARB for an extension. A majority vote will extend the deadline for three months. Additional extensions MAY be requested for up to one year after the initial deadline.

Terminology¶

• Code always includes actual source code, but also include scripts, configuration files, infrastructure-as-code, etc. Any artifact that is used to create an eventual product MAY be considered code for the purposes of this document.
• A developer is anyone who writes code.
• A reviewer is anyone who reviews code. They are often a developer themselves, but it can be helpful to distinguish the roles.
• The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL NOT”, “SHOULD”, “SHOULD NOT”, “RECOMMENDED”, “MAY”, and “OPTIONAL” in this document are to be interpreted as described in RFC 2119

Requirements¶

Scope¶

1. Projects MUST perform code review if they meet any of the following criteria:
   1. The project is part of an application that has an availability level of 3 or higher
   2. The project is part of an application that handles data with a protection level of 3 or higher
   3. The application is considered mission-critical
   4. The project team contains five or more developers
2. Projects SHOULD perform code review even if they do not meet the criteria above

Tooling¶

1. Teams SHOULD select a single tool to use for code review
   1. Teams should ensure the tool is accessible to all developers and reviewers
   2. ARB recommends using GitHub Pull Requests, which are native to our GitHub Enterprise deployment, document the process, collect and aggregate discussion and signoffs, and integrate with the source code repository in a natural way.
   3. Most code can be reviewed in GitHub or Git. If your code cannot be housed there, you SHOULD review it in another tool that provides an audit log. If no tool exists, you SHOULD keep an audit log in a shared space that your team can access.

Process¶

1. At its core, a code review process looks like this:
   1. A developer writes code
   2. One or more reviewers review the code
      1. The developer MUST NOT be a reviewer of their own code
   3. The developer works with the reviewers to get their approval
   4. When approvals are granted, the code is put into use
2. Teams SHOULD document their code review process, including…
   1. Where to put your code to facilitate review
      1. ARB recommends new git branches to facilitate using pull requests
   2. How and when to request review
      1. ARB recommends using your team’s existing communications channels to request reviews
   3. How quickly reviewers are expected to respond to requests
      1. ARB recommends that this be no more than 24 hours
   4. What reviewers are expected to look for and provide feedback on
      1. ARB recommends that reviewers SHOULD seek to understand the code and check for quality, consistency, and fitness to requirements. Other items can be included in the review at the team’s option– see the “Reasons for code review” section for some ideas on what your team might want to include.
   5. How reviewers are supposed to leave feedback
      1. ARB recommends using comments in a pull request
   6. When and how reviewers should withhold approval
      1. ARB recommends using the “Request changes” review type
   7. How many approving reviewers are required
      1. ARB recommends that small teams require one reviewer, and teams with four or more developers require two reviewers
   8. What to do when some reviewers approve and some do not
      1. ARB recommends that code should not be used until all approver’s requested changes have been addressed
   9. Who takes action to put the reviewed code into use
      1. ARB recommends that the original developer merges the pull request and deletes the old branch
      2. If the change will not be approved, ARB recommends that the original developer closes the pull request

Background¶

Code review is a process where code written by one developer is examined by another developer before it is put into use. By ensuring that multiple people see the code before it hits production, you have greater confidence that the code does what it is supposed to in a way that the team can support going forward.

Reasons for code review¶

1. Regulatory
   1. Separation of duties
   2. Possible requirements for deployment into production or merging branches
2. Accuracy and Security
   1. Validating that requirements are fulfilled
   2. Regression check
   3. Catching errors and issues before they are put into production and become a challenge for more people
   4. Spotting attempted insertion of malware and backdoors
3. Standards
   1. Consistency in application development: standardizing patterns and methods across a team or larger group
   2. Naming and formatting conventions and standards
   3. Code quality and readability
   4. Maintainability
4. Improvements and discussion
   1. Sharing new ideas, tools, and techniques
   2. Suggestions for improvements or optimization
   3. Bug and logical error catching
5. Continuity
   1. Understanding of details and components by other team members
   2. Accurate and effective integration of components built by different team members
   3. Mentorship of newer developers
   4. Continuity and idea sharing across different teams, simple continuity for a solo developer