From b7915c1d4d7b5711140f1710dfd17758d85cd961 Mon Sep 17 00:00:00 2001 From: Mariot Chauvin Date: Thu, 11 Apr 2024 18:46:43 +0100 Subject: [PATCH 1/3] Specify that PR can combine multiple changes, especially for dependencies upgrade --- pull-requests.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pull-requests.md b/pull-requests.md index f9a7b9d..a3d918b 100644 --- a/pull-requests.md +++ b/pull-requests.md @@ -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. + +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. ## Releasable A PR should be releasable once its review is complete. From 5f408468fec21637b44555be59c65a1c931bccb3 Mon Sep 17 00:00:00 2001 From: Mariot Chauvin Date: Mon, 15 Apr 2024 15:04:01 +0100 Subject: [PATCH 2/3] Update pull-requests.md Co-authored-by: Kelvin Chappell --- pull-requests.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pull-requests.md b/pull-requests.md index a3d918b..8c9c963 100644 --- a/pull-requests.md +++ b/pull-requests.md @@ -33,7 +33,7 @@ When upgrading dependencies, if not [automated](https://github.com/guardian/reco - 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. -You are more likely to encounters incompatibilities or issues otherwise. +You are more likely to encounter incompatibilities or issues otherwise. 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. From af81509388aad4175817f56898562af693f84667 Mon Sep 17 00:00:00 2001 From: Mariot Chauvin Date: Mon, 15 Apr 2024 15:04:32 +0100 Subject: [PATCH 3/3] Wording improvements Co-authored-by: Kelvin Chappell --- pull-requests.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pull-requests.md b/pull-requests.md index 8c9c963..005bf6a 100644 --- a/pull-requests.md +++ b/pull-requests.md @@ -35,7 +35,7 @@ When upgrading dependencies, if not [automated](https://github.com/guardian/reco You are more likely to encounter incompatibilities or issues otherwise. -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. +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 be more likely to identify the reason for an issue through tests and monitoring than size of the PR. ## Releasable A PR should be releasable once its review is complete.