Simplify pull_request_template.md (#6985)

cloudflare · Oct 21, 2024 · 77a23ab · 77a23ab
1 parent c325e5c
commit 77a23ab
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 34 deletions.
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@ -1,8 +1,8 @@
-## What this PR solves / how to test
+Fixes #[insert GH or internal issue link(s)].
 
-Fixes #[insert GH or internal issue number(s)].
+_Describe your change..._
 
-## Author has addressed the following
+---
 
 - Tests
   - [ ] TODO (before merge)
@@ -12,10 +12,6 @@ Fixes #[insert GH or internal issue number(s)].
   - [ ] I don't know
   - [ ] Required
   - [ ] Not required because:
-- Changeset ([Changeset guidelines](https://github.com/cloudflare/workers-sdk/blob/main/CONTRIBUTING.md#changesets))
-  - [ ] TODO (before merge)
-  - [ ] Changeset included
-  - [ ] Changeset not necessary because:
 - Public documentation
   - [ ] TODO (before merge)
   - [ ] Cloudflare docs PR(s): <!--e.g. <https://github.com/cloudflare/cloudflare-docs/pull/>...-->
@@ -25,9 +21,3 @@ Fixes #[insert GH or internal issue number(s)].
 Have you read our [Contributing guide](https://github.com/cloudflare/workers-sdk/blob/main/CONTRIBUTING.md)?
 In particular, for non-trivial changes, please always engage on the issue or create a discussion or feature request issue first before writing your code.
 -->
-
-<!--
-**Note for PR author:**
-We want to celebrate and highlight awesome PR review!
-If you think this PR received a particularly high-caliber review, please assign it the label `highlight pr review` so future reviewers can take inspiration and learn from it.
--->
diff --git a/.github/workflows/validate-pr-description.yml b/.github/workflows/validate-pr-description.yml
@@ -26,6 +26,11 @@ jobs:
             everything_but_markdown:
               - '!**/*.md'
 
+      - uses: jitterbit/get-changed-files@v1
+        id: files
+        with:
+          format: "json"
+
       - name: Install Dependencies
         if: steps.changes.outputs.everything_but_markdown == 'true'
         uses: ./.github/actions/install-dependencies
@@ -41,3 +46,4 @@ jobs:
           TITLE: ${{ github.event.pull_request.title }}
           BODY: ${{ github.event.pull_request.body }}
           LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }}
+          FILES: ${{ steps.files.outputs.all }}
diff --git a/tools/deployments/__tests__/validate-pr-description.test.ts b/tools/deployments/__tests__/validate-pr-description.test.ts
@@ -4,7 +4,7 @@ import { validateDescription } from "../validate-pr-description";
 describe("validateDescription()", () => {
 	it("should skip validation with the `skip-pr-description-validation` label", () => {
 		expect(
-			validateDescription("", "", '["skip-pr-description-validation"]')
+			validateDescription("", "", '["skip-pr-description-validation"]', "[]")
 		).toHaveLength(0);
 	});
 
@@ -26,28 +26,54 @@ Fixes #[insert GH or internal issue number(s)].
   - [ ] I don't know
   - [ ] Required
   - [ ] Not required because:
-- Changeset ([Changeset guidelines](https://github.com/cloudflare/workers-sdk/blob/main/CONTRIBUTING.md#changesets))
-  - [ ] TODO (before merge)
-  - [ ] Changeset included
-  - [ ] Changeset not necessary because:
 - Public documentation
   - [x] TODO (before merge)
   - [ ] Cloudflare docs PR(s): <!--e.g. <https://github.com/cloudflare/cloudflare-docs/pull/>...-->
   - [ ] Documentation not necessary because:
 `,
+				"[]",
 				"[]"
 			)
 		).toMatchInlineSnapshot(`
 			[
 			  "All TODO checkboxes in your PR description must be unchecked before merging",
 			  "Your PR must include tests, or provide justification for why no tests are required",
 			  "Your PR must run E2E tests, or provide justification for why running them is not required",
-			  "Your PR must include a changeset, or provide justification for why no changesets are required",
+			  "Your PR doesn't include a changeset. Either include one (following the instructions in CONTRIBUTING.md) or add the 'no-changeset-required' label to bypass this check. Most PRs should have a changeset, so only bypass this check if you're sure that your change doesn't need one: see https://github.com/cloudflare/workers-sdk/blob/main/CONTRIBUTING.md#changesets for more details.",
 			  "Your PR must include documentation (in the form of a link to a Cloudflare Docs issue or PR), or provide justification for why no documentation is required",
 			]
 		`);
 	});
 
+	it("should bypass changesets check with label", () => {
+		expect(
+			validateDescription(
+				"",
+				`## What this PR solves / how to test
+
+Fixes #[insert GH or internal issue number(s)].
+
+## Author has addressed the following
+
+- Tests
+  - [ ] TODO (before merge)
+  - [x] Tests included
+  - [ ] Tests not necessary because:
+- E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
+  - [ ] I don't know
+  - [ ] Required
+  - [x] Not required because: test
+- Public documentation
+  - [ ] TODO (before merge)
+  - [ ] Cloudflare docs PR(s): <!--e.g. <https://github.com/cloudflare/cloudflare-docs/pull/>...-->
+  - [x] Documentation not necessary because: test
+`,
+				'["no-changeset-required"]',
+				"[]"
+			)
+		).toHaveLength(0);
+	});
+
 	it("should accept everything included", () => {
 		expect(
 			validateDescription(
@@ -75,7 +101,8 @@ Fixes [AA-000](https://jira.cfdata.org/browse/AA-000).
   - [x] Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/123
   - [ ] Documentation not necessary because:
 `,
-				"[]"
+				"[]",
+				'[".changeset/hello-world.md"]'
 			)
 		).toHaveLength(0);
 	});
@@ -107,7 +134,8 @@ Fixes [AA-000](https://jira.cfdata.org/browse/AA-000).
   - [x] Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/123
   - [ ] Documentation not necessary because:
 `,
-				"[]"
+				"[]",
+				'[".changeset/hello-world.md"]'
 			)
 		).toMatchInlineSnapshot(`
 			[
@@ -144,7 +172,8 @@ Fixes [AA-000](https://jira.cfdata.org/browse/AA-000).
   - [x] Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/123
   - [ ] Documentation not necessary because:
 `,
-				"[]"
+				"[]",
+				'[".changeset/hello-world.md"]'
 			)
 		).toMatchInlineSnapshot(`
 			[
@@ -180,7 +209,8 @@ Fixes [AA-000](https://jira.cfdata.org/browse/AA-000).
   - [x] Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/123
   - [ ] Documentation not necessary because:
 `,
-				'["e2e"]'
+				'["e2e"]',
+				'[".changeset/hello-world.md"]'
 			)
 		).toHaveLength(0);
 	});
@@ -212,7 +242,8 @@ Fixes [AA-000](https://jira.cfdata.org/browse/AA-000).
   - [X] Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/123
   - [ ] Documentation not necessary because:
 `,
-				'["e2e"]'
+				'["e2e"]',
+				'[".changeset/hello-world.md"]'
 			)
 		).toHaveLength(0);
 	});

diff --git a/tools/deployments/validate-pr-description.ts b/tools/deployments/validate-pr-description.ts
@@ -3,7 +3,8 @@ if (require.main === module) {
 	const errors = validateDescription(
 		process.env.TITLE as string,
 		process.env.BODY as string,
-		process.env.LABELS as string
+		process.env.LABELS as string,
+		process.env.FILES as string
 	);
 	if (errors.length > 0) {
 		console.error("Validation errors in PR description:");
@@ -17,12 +18,14 @@ if (require.main === module) {
 export function validateDescription(
 	title: string,
 	body: string,
-	labels: string
+	labels: string,
+	changedFilesJson: string
 ) {
 	const errors: string[] = [];
 
 	console.log("PR:", title);
-	const parsedLabels = JSON.parse(labels);
+
+	const parsedLabels = JSON.parse(labels) as string[];
 
 	if (parsedLabels.includes("skip-pr-description-validation")) {
 		console.log(
@@ -71,14 +74,14 @@ export function validateDescription(
 		);
 	}
 
-	if (
-		!(
-			/- \[x\] Changeset included/i.test(body) ||
-			/- \[x\] Changeset not necessary because: .+/i.test(body)
-		)
-	) {
+	const changedFiles = JSON.parse(changedFilesJson) as string[];
+	const changesetIncluded = changedFiles.some((f) =>
+		f.startsWith(".changeset/")
+	);
+
+	if (!changesetIncluded && !parsedLabels.includes("no-changeset-required")) {
 		errors.push(
-			"Your PR must include a changeset, or provide justification for why no changesets are required"
+			"Your PR doesn't include a changeset. Either include one (following the instructions in CONTRIBUTING.md) or add the 'no-changeset-required' label to bypass this check. Most PRs should have a changeset, so only bypass this check if you're sure that your change doesn't need one: see https://github.com/cloudflare/workers-sdk/blob/main/CONTRIBUTING.md#changesets for more details."
 		);
 	}