Cybersecurity, Adopting Code Review

Adopting Code Review

The purpose of this document is to help development teams associated with the University of Illinois fulfill their responsibility to comply with Illinois Cybersecurity standards, including IT-7, IT08, and IT13.

Use this document to guide discussions on your development team of adopting effective code review practices. Teams are encouraged to use this document as a starting point to draft a more detailed document internal to their team, committed to by all team members.

TL;DR:

• Request lots of reviewers
• Use the bot
• Expect fast feedback

• Submit pull requests often
• Try to keep each pull request to 20 lines of new non-boilerplate code or less
• Link relevant GitHub issues to the pull request
• We typically link issues using GitHub commit keywords so issue numbers are present in our Git log
• Request lots of reviewers: No one sees a pull request until reviewers are assigned
• Everyone listed in a Code Owners file will be automatically requested for new pull requests
• Feel free to request reviewers from other teams
• Teams commit to providing additional reviewers when requested by pull request
• Add the GitHub Teams reminder bot to all repositories
• You are encouraged to remind colleagues of your open pull request
• Be available in chat to discuss the code
• During your daily stand-up, always mention any ready-for-review pull request that is not reviewed
• Teams should commit to reviewing code that is not reviewed daily
• Some feedback merits opening a new issue to keep the current pull request simple

• We prioritize reviewing each other's code over writing new code
• We encourage having a look at any open pull requests assigned to you each time you return from a coffee break
• One work day is the longest we expect any non-draft pull request to sit without feedback
• Please do review code you do not fully understand
• Your manager will make time for you to train in languages for code you are requested to review
• Pair and ensemble programming count as code review
• Feel free to approve pull requests for work that you are satisfied with after any collaborative programming session
• Review does not result in automatic approval, even when the work was done in an ensemble session

• Merging a pull request requires 2 approvals
• We require a linear history
  • We have disabled the default create a merge commit
  • This saves headaches for everyone when merging
• When merging to main
  • We prefer squash and merge as the typical method
  • This results in one commit per pull request

The following GitHub repository settings are recommended for fast effective code-review:

• Require pull request reviews before merging: two approvals is a recommended starting setting
• Restrict who can push to matching branches: use this setting to ensure only pull requests update the default branch
• Require status checks before merging: use this to ensure that tests, static code analysis and secret detection pass before merging each pull request
• Require linear history: linear history simplifies merging
• Require conversation resolution before merging
• Use a pre-commit hook to prevent checking in secrets

These resources are tailored for use with the University of Illinois Shared GitHub Service, but most of these principles can be applied to any source control solution.

• Linking a GitHub pull request to an issue using a keyword
• What's the Difference Between the 3 Github Merge Methods?
• Use a pre-commit hook to prevent checking in secrets