Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify that PR can combine multiple changes, especially for dependencies upgrade #164

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Specify that PR can combine multiple changes, especially for dependencies upgrade #164

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7915c1
Specify that PR can combine multiple changes, especially for dependen…
mchv Apr 11, 2024
5f40846
Update pull-requests.md
mchv Apr 15, 2024
af81509
Wording improvements
mchv Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions pull-requests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,18 @@ Read over your code before opening the PR for review. The main point of code rev
Tip: Github allows you to open a PR in `draft` state, which can often be helpful if you want to review your own code, or have remote CI checks run, before others review it. (Remember to [change the status of your PR](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review) to `ready for review` later on though!)

## Small
A PR should be clear, and address a single concern.

There should be a manageable number of changes in a PR. Try to make your PR as small as possible while still advancing the functionality of the product.
A PR should be clear, address a single concern, and composed of a manageable number of changes.

If a PR is too complex, it is much easier for issues to be missed. In addition if trying to understand an aspect of the code change in the future, it becomes harder to understand if buried in a very large and complex PR.

When upgrading dependencies, if not [automated](https://github.com/guardian/recommendations/blob/main/scala.md#continuous-dependency-management), try to update several of them at the same time:
- For Scala, upgrade `scala` runtime, `sbt`, `sbt-plugins` and optionally `play-framework` all in one PR.
- For Node, upgrade `typescript` runtime, package manager, code formatter, linter, bundler, all in one PR.

Aim to deploy a PR before submitting the next one. Frequently small deployments of functionality are easier to review and easier to understand in PROD.
You are more likely to encounters incompatibilities or issues otherwise.
mchv marked this conversation as resolved.
Show resolved Hide resolved

In general you don't need to make your PR as small as possible, and favour pace and frequency of delivery over atomicity of changes. Contrary to commits which can be [bisected to find root cause of an issue](https://www.metaltoad.com/blog/beginners-guide-git-bisect-process-elimination), every deployment will integrate additional changes in the system to the ones in the PR (for example baked AMI may be a new one, EC2 bootstrap script may install a newer version of system library, etc.) so you will more likely identify reason of an issue through tests and monitoring than size of of the PR.
mchv marked this conversation as resolved.
Show resolved Hide resolved

## Releasable
A PR should be releasable once its review is complete.
Expand Down