My Thoughts On Pull Request Reviews

They are *the* place to capture context around code changes.

Author

Matt Gowie
January 28, 2025

Hey folks,

I’ve written before on pull requests (PRs) and how they interact with IaC, but I wanted to share some thoughts about PRs themselves.

This is a modified version of a post I shared internally at Masterpoint.

  1. All code should be PR’d before it is merged to the default branch (main or master). No exceptions.

  2. PR titles should should follow conventional commits.

  3. PRs should be small. If you’re working on code for more than 3 days then you should try to find a way to merge your changes as soon as possible. Or, start to do multi-part PRs so that they can be more easily reviewed.

  4. Don’t share random commit links, compare links, or branch files with another team member or client. Put your work up as a PR and have the discussion there. PRs are the best place to discuss code. They have a consistent URL, they have tooling for comments, and they are permanent, which helps when a decision needs to be understood months or years from now.

  5. Reviews should never be rubber stamps of approval. Take the time to review the PR, ensure that it is clear, and that it follows conventions and best practices. There are no dumb questions and being nit picky is not a bad thing.

  6. Prefer squash merges, which consolidate commits and include a reference to the PR. The PR will provide the full history of the commits, discussion, and the why/how reasoning that went into that merge. This setting should be enforced across all repositories. If it is not enforced for a repo, speak up and get that fixed.

  7. If for some reason you cannot do squash merges, such as client policies, then write better commit messages. Learn how to use --amend and git rebase -i before pushing changes to a remote.

  8. Do not do PR reviews in meetings if you can help it. If you must do a PR review in a meeting, write comments in the PR as you go along. This captures context for the future. Record the meeting and include the link in the PR.

All of these are hills I will die on.

May you never share a commit link,

Matt @ Masterpoint

PS I spoke at OpenTofu Day during Kubecon in the fall, discussing how we migrated a client on to OpenTofu. Enjoy the talk and the q&a session after!