Code Review and Approval

"Code review is just the last line of defense to make sure people don't merge things that will create problems for everyone else. Good engineers figure out the important stuff before anyone writes a line of code. The APIs, the architecture, high-level component names. It's all been talked over. When a good engineer reviews another good engineer's pull request, they're just checking that what is delivered is an implementation of what is expected.

Code review is a road block for people who don't do the above. It causes them to bump into someone who knows what's up.

Everything else that gets argued about: local variable names, formatting, equivalent ways to express the same thing. It's not important. I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom?"

Source (opens in a new tab)

Code Ownership

At Pivot, we practice weak code ownership on selective parts of the codebase. The concept of code ownership here simply refers to a person or team who has domain expertise. This person/team has neither the authority to approve nor to disapprove the overall future of the 'owned' code, but as the technical expert they need to approve changes made and (more importantly, per the quote at the top of this article) they should be conferred with prior to even writing code if appropriate.

pivot and pivot-internal have a CODEOWNERS file where ownership rules are defined. Branch protection rules require approval from a code owner, so generally teams are preferred as code owners versus individuals.

Code should not have an owner defined unless one of the following is true:

1. The code defines production infrastructure (e.g., Terraform) or build tooling (dockerfiles, for example).
2. The code is part of a sensitive and complex system that is both mission critical and highly domain-specific (e.g., our Wallstreet service that handles payments and billing).
3. The code is responsible for a fundamental security layer (e.g. JWTs).

You'll note that 'features' do not qualify for code ownership rules — everyone is empowered to contribute and should not feel blocked by silos of control.

Approval Rules

In the absence of automatic approval requests via CODEOWNERS, developers should invite two applicable engineers to approve their PRs.

It's also important not to send out mass approval requests. Just ask the right two people, based on the scope of the PR. At Pivot, the expection is that when someone asks for review from you, you do so quicky or decline (for reasons of expertise or availability) quickly.

Because we practice continious deployment of the main branch and PR directly to that branch, PRs require two reviewers - the stakes are high. The two approving reviewers are taking collective responsibility with the PR author that they understand the changes made and know how to rollback if needed.

If you don't understand enough about AWS, Cloudflare, EAS Update, and the general deployment flow via GitHub Actions and Terraform, don't approve PRs.

Keep in mind the quote from the top of this article: if you are having long conversations via the GitHub approval flow, you skipped a step before you began writing code to begin with.

Specific Review Guidance

• Migrations: When reviewing database migrations, ensure the guidelines are followed.
• Service Boundaries: Services should only use each other's APIs, never each other's data stores.
• Rollback ability: Will this deployment mutate some value that then prevents a rollback from succeeding? This is related to database migrations, but also applicable to other aspects of a service like a DNS change, for example. Is Flipt being used for continious configuration to gate new functionality behind a flag? If not, why?
• Breaking changes: If a breaking change is being made to a service's API, there should be a very good reason and a clearly documented deployment strategy.
• Test coverage: For specific services we maintain ~100% code coverage (e.g. Auth). In those areas of the codebase, this coverage needs to be maintained and of course tests need to be passing. In areas of the codebase that aren't required to have such coverage, tests are entirely optional and should be the subject of team discussion.
• Scope: PRs should be small. If they are cross-service or otherwise complex, there should be a very good reason as well as a deployment strategy which considers the fact that old code will run alongside new code, so backwards compatability is required.

Self-Review via the PR Checklist

A pull request template (opens in a new tab) is automatically added to the description of all PRs to the pivot and pivot-internal repos. The intent of the template is to define a shared checklist and understanding of accountabilities between authors and reviewers. Always fill it out and make any needed changes before requesting reviews.