Code review in GovPress

GovPress follows the same process for software development as the Tech Team. However, developers in the Tech Team work on a small number of long-running projects, with relatively large teams. In GovPress we work on a very large number of small projects, with small teams, so we have a slightly different emphasis on how we write code.

In particular, if you are requesting review, you should be aware that reviewers will be reviewing multiple changesets over a large number of very different projects each day, and will not necessarily be aware of the full context of each story. This makes it especially important that we follow a consistent and efficient process.

This document should therefore be read in addition to the Tech Team documentation and applies to all PRs that are not automatically generated by services such as Dependabot or GovPress Tools.

Creating PRs #

When you create a PR the Description section of the PR should include, as a minimum:

A full description of the story that the PR addresses. For example:
Links to any relevant documentation, which might be internal (Zendesk, Trello, etc.) or external. For example:
Details of any concerns that might not be obvious from the code (these can usually be copied from commit messages). For example:
- dalmatian/543
Full instructions on how the reviewer can test the PR locally (or in some other appropriate way), and what results the reviewer can expect. This section should never be skipped when making changes to source code. If the PR will change how a site looks, please include screenshots to help to illustrate the expected changes. For example:
- lambethtogeher/160
- govpress-product-theme/167

If your PR interacts with the database or contains any front-end code, special consideration should be given to security concerns. If you are not sure what security considerations you need to be aware of, please ask a Tech Lead before pushing your changes.

Reviewing PRs #

All developers are expected to undertake code review. Unless a PR only includes documentation, or similar, you should always test the changeset locally. If it is not clear how to do this, ask the PR author using the Request changes feature in GitHub, or similar.

PRs should not be held up unnecessarily, so if you have questions about the changeset, or small requests, be clear to tell the author if your comments are non-blocking. Your main role is to prevent bugs appearing on live systems, and to minimise the amount of technical debt in our repositories. If your suggested changes do not achieve either of those aims, they should not block a merge.

Identifying technical debt is a somewhat subjective process, and most developers will get better at this with experience. However, there are a few ways to think about code changes that will help. In particular, try to imagine the next engineer who will be reading these changes or the commit messages for them. They might be:

A developer from another unit who is debugging an outage in the early hours of the morning, with no context at all for the project.
A developer on Support who has never worked on the repository but needs to respond to an urgent client request.
An engineer debugging a critical error in production, during an incident, either by manual testing or git bisect.
In all of these cases, the context around the change you are proposing, the reason for making it, and links to Zendesk tickets or external documentation will be crucial to help the next developer.
If you are not sure how easy your commit history will be to understand, run something like git log --color --graph --oneline --decorate before you push to a Git server, and consider whether the subject lines of your commit tell a clear and consistent story. You might also find tools like tig or gitui useful.

After merging #

Deploying your changes #

Once a PR has been merged, it will be automatically deployed to a live environment. Initially, this will be a staging environment. Usually, the status of that deploy can be checked with dalmatian service deploy-status -i <infra> -e <env> -s <service>.

If you are the PR author, it is your responsibility to check the status of your deploy and to smoke-test your changes on the live site. If a change unexpectedly causes a bug, then you should raise a new PR to fix that bug, since you have the most context for the work.

Creating production releases #

Once your changes are ready for release (usually when they have been through client review). You should raise a production release PR, which will usually be requesting a merge of commits from develop to main.

The description for a production PR should contain a list of the changes that are included in the release. You are not expected to summarise all included PRs in detail, but you should point reviewers to anything that is unusual or unexpected in the PR, to speed up the reviewing process. It is OK to point to documentation in previous PR descriptions or commit messages for this.

There may also be maintenance changes (documentation, dependency updates, etc.) in the same release, but usually the developer with the largest number of meaningful commits should raise a release PR and smoke-test the deployment.

Production releases should happen as soon as possible after work has been tested on staging, never longer than a sprint, and should not be passed on to other colleagues unless you are out of office.

Leaving production releases for a long time risks losing context for the changes, and also increases the likelihood that security patches will need to be applied by hot-fixing, which we should avoid unless absolutely necessary.

If you find a production release has been unexpectedly held up for a technical reason, please speak to a Tech Lead about how this should be handled. If you experience a delay in client sign-off, please speak to your Delivery Lead.

Reviewing production releases #

If you are reviewing a production release, you should ensure that it has been tested carefully before merging. For most of our live systems, it is fine to use a staging server to manually test a production release, rather than your local development environment.

If a production release contains bugs #

If a deploy to a production site breaks a core piece of functionality, such as site navigation or site search, a major incident should be declared, usually by running /inc in Slack and following the on-screen instructions. Incidents take priority over all other work, and should be attended by the developer(s) who created the commits in the production release.

Whether or not a bug in a production release constitutes an incident, we aim to fix bugs that appear in production as quickly as possible.