From b5278c1604f0fed31914a9a1c7506e74e435ffed Mon Sep 17 00:00:00 2001 From: epolon Date: Sun, 12 Jan 2025 23:33:10 +0200 Subject: [PATCH 01/16] mid work --- tools/@aws-cdk/prlint/lint.ts | 51 +++++++++++++++++++++++++ tools/@aws-cdk/prlint/test/lint.test.ts | 8 ++++ 2 files changed, 59 insertions(+) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 48c5a23819459..06f267be643dd 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -10,6 +10,16 @@ export type GitHubPr = Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}']['response']['data']; export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; +export const CODECOV_PREFIX = 'codecov/'; + +export const CODECOV_CHECKS = [ + 'patch', + 'patch/packages/aws-cdk', + 'patch/packages/aws-cdk-lib/core', + 'project', + 'project/packages/aws-cdk', + 'project/packages/aws-cdk-lib/core' +]; const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; @@ -24,6 +34,7 @@ enum Exemption { CLI_INTEG_TESTED = 'pr-linter/cli-integ-tested', REQUEST_CLARIFICATION = 'pr/reviewer-clarification-requested', REQUEST_EXEMPTION = 'pr-linter/exemption-requested', + CODECOV = "pr-linter/exempt-codecov", } export interface GithubStatusEvent { @@ -352,6 +363,24 @@ export class PullRequestLinter { } } + private async checkRunSucceeded(sha: string, checkName: string) { + const response = await this.client.paginate(this.client.checks.listForRef, { + owner: this.prParams.owner, + repo: this.prParams.repo, + ref: sha, + }); + + // grab the last check run that was started + const status = response + .filter(c => c.started_at !== undefined) + .sort((c1, c2) => c2.started_at!.localeCompare(c1.started_at!)) + .filter(c => c.name === checkName) + .map(s => s.conclusion)[0]; + + console.log(`${checkName} status: ${status}`) + return status === 'success'; + } + /** * Assess whether or not a PR is ready for review. * This is needed because some things that we need to evaluate are not filterable on @@ -575,6 +604,24 @@ export class PullRequestLinter { ], }); + const codecovTests: Test[] = []; + for (const c of CODECOV_CHECKS) { + const checkName = `${CODECOV_PREFIX}${c}`; + const succeeded = await this.checkRunSucceeded(sha, checkName); + codecovTests.push({ + test: () => { + const result = new TestResult(); + result.assessFailure(!succeeded, `${checkName} job is not succeeding`); + return result; + } + }) + } + + validationCollector.validateRuleSet({ + exemption: shouldExemptCodecov, + testRuleSet: codecovTests, + }); + console.log("Deleting PR Linter Comment now"); await this.deletePRLinterComment(); try { @@ -656,6 +703,10 @@ function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { return result; } +function shouldExemptCodecov(pr: GitHubPr): boolean { + return hasLabel(pr, Exemption.CODECOV); +} + function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); } diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 1de1f3c334ee0..3cc25584b3818 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1190,6 +1190,14 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ }, }; + const checksClient = { + listForRef() { + return { + data: linter.CODECOV_CHECKS.map(c => ({ name: `${linter.CODECOV_PREFIX}${c}`, conclusion: 'success' })), + } + } + } + const searchClient = { issuesAndPullRequests() {}, }; From d8d84277d0fa197725e9fc363875935160b4f735 Mon Sep 17 00:00:00 2001 From: epolon Date: Mon, 13 Jan 2025 09:09:28 +0200 Subject: [PATCH 02/16] mid work --- tools/@aws-cdk/prlint/lint.ts | 20 ++++++++++++-------- tools/@aws-cdk/prlint/test/lint.test.ts | 1 + 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 06f267be643dd..5ad0a97a0ac14 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import { Octokit } from '@octokit/rest'; import { Endpoints } from '@octokit/types'; import { StatusEvent } from '@octokit/webhooks-definitions/schema'; +import type { components } from '@octokit/openapi-types'; import { findModulePath, moduleStability } from './module'; import { breakingModules } from './parser'; @@ -21,6 +22,8 @@ export const CODECOV_CHECKS = [ 'project/packages/aws-cdk-lib/core' ]; +type CheckRunConclusion = components['schemas']['check-run']['conclusion'] + const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; /** @@ -363,7 +366,7 @@ export class PullRequestLinter { } } - private async checkRunSucceeded(sha: string, checkName: string) { + private async checkRunConclusion(sha: string, checkName: string): Promise { const response = await this.client.paginate(this.client.checks.listForRef, { owner: this.prParams.owner, repo: this.prParams.repo, @@ -371,14 +374,14 @@ export class PullRequestLinter { }); // grab the last check run that was started - const status = response - .filter(c => c.started_at !== undefined) - .sort((c1, c2) => c2.started_at!.localeCompare(c1.started_at!)) + const conclusion = response .filter(c => c.name === checkName) + .filter(c => c.started_at != null) + .sort((c1, c2) => c2.started_at!.localeCompare(c1.started_at!)) .map(s => s.conclusion)[0]; - console.log(`${checkName} status: ${status}`) - return status === 'success'; + console.log(`${checkName} conclusion: ${conclusion}`) + return conclusion; } /** @@ -607,11 +610,12 @@ export class PullRequestLinter { const codecovTests: Test[] = []; for (const c of CODECOV_CHECKS) { const checkName = `${CODECOV_PREFIX}${c}`; - const succeeded = await this.checkRunSucceeded(sha, checkName); + const status = await this.checkRunConclusion(sha, checkName); codecovTests.push({ test: () => { const result = new TestResult(); - result.assessFailure(!succeeded, `${checkName} job is not succeeding`); + const message = status == null ? `${checkName} has not started yet` : `${checkName} job is in status: ${status}`; + result.assessFailure(status !== 'success', message); return result; } }) diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 3cc25584b3818..1eb2fee558309 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1212,6 +1212,7 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ issues: issuesClient as any, search: searchClient as any, repos: reposClient as any, + checks: checksClient as any, paginate: (method: any, args: any) => { return method(args).data; }, } as any, }); From 493539a89280f183bc80de5fdf1bc0b6b5fe3d07 Mon Sep 17 00:00:00 2001 From: epolon Date: Mon, 13 Jan 2025 09:41:44 +0200 Subject: [PATCH 03/16] fix tests --- tools/@aws-cdk/prlint/lint.ts | 4 ++-- tools/@aws-cdk/prlint/test/lint.test.ts | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 5ad0a97a0ac14..39cd4db00919c 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -366,7 +366,7 @@ export class PullRequestLinter { } } - private async checkRunConclusion(sha: string, checkName: string): Promise { + private async checkRunConclusion(sha: string, checkName: string): Promise { const response = await this.client.paginate(this.client.checks.listForRef, { owner: this.prParams.owner, repo: this.prParams.repo, @@ -374,7 +374,7 @@ export class PullRequestLinter { }); // grab the last check run that was started - const conclusion = response + const conclusion = response.check_runs .filter(c => c.name === checkName) .filter(c => c.started_at != null) .sort((c1, c2) => c2.started_at!.localeCompare(c1.started_at!)) diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 1eb2fee558309..fe64c74d026f6 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1193,7 +1193,11 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ const checksClient = { listForRef() { return { - data: linter.CODECOV_CHECKS.map(c => ({ name: `${linter.CODECOV_PREFIX}${c}`, conclusion: 'success' })), + data: { check_runs: linter.CODECOV_CHECKS.map(c => ({ + name: `${linter.CODECOV_PREFIX}${c}`, + conclusion: 'success', + started_at: '1' + }))}, } } } From c5da0d3aedcb7e8a5cd361ed021f2a3684961b50 Mon Sep 17 00:00:00 2001 From: epolon Date: Wed, 15 Jan 2025 10:37:51 +0200 Subject: [PATCH 04/16] mid work --- tools/@aws-cdk/prlint/lint.ts | 47 +++++++++++++------------ tools/@aws-cdk/prlint/test/lint.test.ts | 8 ++--- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 39cd4db00919c..547184e722fb2 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -373,11 +373,10 @@ export class PullRequestLinter { ref: sha, }); - // grab the last check run that was started - const conclusion = response.check_runs - .filter(c => c.name === checkName) - .filter(c => c.started_at != null) - .sort((c1, c2) => c2.started_at!.localeCompare(c1.started_at!)) + // grab the last check run that was completed + const conclusion = response + .filter(c => c.name === checkName && c.completed_at != null) + .sort((c1, c2) => c2.completed_at!.localeCompare(c1.completed_at!)) .map(s => s.conclusion)[0]; console.log(`${checkName} conclusion: ${conclusion}`) @@ -555,6 +554,7 @@ export class PullRequestLinter { console.log(`⌛ Fetching PR number ${number}`); const pr = (await this.client.pulls.get(this.prParams)).data as GitHubPr; + console.log(`PR base ref is: ${pr.base.ref}`) console.log(`⌛ Fetching files for PR number ${number}`); const files = await this.client.paginate(this.client.pulls.listFiles, this.prParams); @@ -607,25 +607,28 @@ export class PullRequestLinter { ], }); - const codecovTests: Test[] = []; - for (const c of CODECOV_CHECKS) { - const checkName = `${CODECOV_PREFIX}${c}`; - const status = await this.checkRunConclusion(sha, checkName); - codecovTests.push({ - test: () => { - const result = new TestResult(); - const message = status == null ? `${checkName} has not started yet` : `${checkName} job is in status: ${status}`; - result.assessFailure(status !== 'success', message); - return result; - } - }) + if (pr.base.ref === 'main') { + // we don't enforce codecov on release branches + const codecovTests: Test[] = []; + for (const c of CODECOV_CHECKS) { + const checkName = `${CODECOV_PREFIX}${c}`; + const conclusion = await this.checkRunConclusion(sha, checkName); + codecovTests.push({ + test: () => { + const result = new TestResult(); + const message = conclusion == null ? `${checkName} has not reported a status yet` : `${checkName} job is in status: ${conclusion}`; + result.assessFailure(conclusion !== 'success', message); + return result; + } + }) + } + + validationCollector.validateRuleSet({ + exemption: shouldExemptCodecov, + testRuleSet: codecovTests, + }); } - validationCollector.validateRuleSet({ - exemption: shouldExemptCodecov, - testRuleSet: codecovTests, - }); - console.log("Deleting PR Linter Comment now"); await this.deletePRLinterComment(); try { diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index fe64c74d026f6..d287f27186c74 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1142,7 +1142,7 @@ describe('integration tests required on features', () => { function configureMock(pr: Subset, prFiles?: linter.GitHubFile[], existingComments?: string[]): linter.PullRequestLinter { const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) { - return { data: pr }; + return { data: { ...pr, base: { ref: 'main'}} }; }, listFiles(_props: { _owner: string, _repo: string, _pull_number: number }) { @@ -1193,11 +1193,11 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ const checksClient = { listForRef() { return { - data: { check_runs: linter.CODECOV_CHECKS.map(c => ({ + data: linter.CODECOV_CHECKS.map(c => ({ name: `${linter.CODECOV_PREFIX}${c}`, conclusion: 'success', - started_at: '1' - }))}, + completed_at: '1' + })), } } } From 50996675bfc626690c72ee50af1b2618714662d0 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 10:59:32 +0100 Subject: [PATCH 05/16] Big-ass refactor --- .github/workflows/pr-linter-trigger.yml | 4 + .github/workflows/pr-linter.yml | 45 +-- tools/@aws-cdk/prlint/constants.ts | 27 ++ tools/@aws-cdk/prlint/github.ts | 36 +++ tools/@aws-cdk/prlint/index.ts | 78 +++-- tools/@aws-cdk/prlint/lint.ts | 414 ++---------------------- tools/@aws-cdk/prlint/linter-base.ts | 226 +++++++++++++ tools/@aws-cdk/prlint/results.ts | 92 ++++++ tools/@aws-cdk/prlint/test/lint.test.ts | 60 ++-- 9 files changed, 508 insertions(+), 474 deletions(-) create mode 100644 tools/@aws-cdk/prlint/constants.ts create mode 100644 tools/@aws-cdk/prlint/github.ts create mode 100644 tools/@aws-cdk/prlint/linter-base.ts create mode 100644 tools/@aws-cdk/prlint/results.ts diff --git a/.github/workflows/pr-linter-trigger.yml b/.github/workflows/pr-linter-trigger.yml index fb13df957a1c9..6a6cb809f8d9f 100644 --- a/.github/workflows/pr-linter-trigger.yml +++ b/.github/workflows/pr-linter-trigger.yml @@ -1,3 +1,7 @@ +# Unprivileged workflow that runs in the context of the PR. +# +# Save the PR number, and download it again in the context of the PR Linter, +# which runs privileged. name: PR Linter Trigger on: diff --git a/.github/workflows/pr-linter.yml b/.github/workflows/pr-linter.yml index 89e5944c89ef0..6388fee91fe2e 100644 --- a/.github/workflows/pr-linter.yml +++ b/.github/workflows/pr-linter.yml @@ -26,39 +26,29 @@ jobs: # if conditions on all individual steps because subsequent jobs depend on this job # and we cannot skip it entirely steps: - - name: 'Download artifact' + - name: 'Download workflow_run artifact' if: github.event_name == 'workflow_run' - uses: actions/github-script@v7 + uses: dawidd6/action-download-artifact@v7 with: - script: | - let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({ - owner: context.repo.owner, - repo: context.repo.repo, - run_id: context.payload.workflow_run.id, - }); - let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => { - return artifact.name == "pr_info" - })[0]; - let download = await github.rest.actions.downloadArtifact({ - owner: context.repo.owner, - repo: context.repo.repo, - artifact_id: matchArtifact.id, - archive_format: 'zip', - }); - let fs = require('fs'); - fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/pr_info.zip`, Buffer.from(download.data)); - - name: 'Unzip artifact' - if: github.event_name == 'workflow_run' - run: unzip pr_info.zip + run_id: ${{ github.event.workflow_run.id }} + name: pr_info + path: pr/ + search_artifacts: true - - name: 'Make GitHub output' + - name: 'Determine PR info' + # PR info comes from the artifact if downloaded, or GitHub context if not. if: github.event_name == 'workflow_run' id: 'pr_output' run: | - echo "cat pr_number" - echo "pr_number=$(cat pr_number)" >> "$GITHUB_OUTPUT" - echo "cat pr_sha" - echo "pr_sha=$(cat pr_sha)" >> "$GITHUB_OUTPUT" + if [[ ! -f pr/pr_number ]]; then + echo "${{ github.event.pull_request.number }}" > pr/pr_number + fi + if [[ ! -f pr/pr_sha ]]; then + echo "${{ github.event.pull_request.head.sha }}" > pr/pr_sha + fi + cat pr/* + echo "pr_number=$(cat pr/pr_number)" >> "$GITHUB_OUTPUT" + echo "pr_sha=$(cat pr/pr_sha)" >> "$GITHUB_OUTPUT" validate-pr: # Necessary to have sufficient permissions to write to the PR @@ -80,7 +70,6 @@ jobs: uses: ./tools/@aws-cdk/prlint env: GITHUB_TOKEN: ${{ secrets.PROJEN_GITHUB_TOKEN }} - # PR_NUMBER and PR_SHA is empty if triggered by pull_request_target, since we already have that info PR_NUMBER: ${{ needs.download-if-workflow-run.outputs.pr_number }} PR_SHA: ${{ needs.download-if-workflow-run.outputs.pr_sha }} REPO_ROOT: ${{ github.workspace }} diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts new file mode 100644 index 0000000000000..227f75ece94dd --- /dev/null +++ b/tools/@aws-cdk/prlint/constants.ts @@ -0,0 +1,27 @@ + +export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; + +/** + * Types of exemption labels in aws-cdk project. + */ +export enum Exemption { + README = 'pr-linter/exempt-readme', + TEST = 'pr-linter/exempt-test', + INTEG_TEST = 'pr-linter/exempt-integ-test', + BREAKING_CHANGE = 'pr-linter/exempt-breaking-change', + CLI_INTEG_TESTED = 'pr-linter/cli-integ-tested', + REQUEST_CLARIFICATION = 'pr/reviewer-clarification-requested', + REQUEST_EXEMPTION = 'pr-linter/exemption-requested', + CODECOV = "pr-linter/exempt-codecov", +} + +export const CODECOV_PREFIX = 'codecov/'; + +export const CODECOV_CHECKS = [ + 'patch', + 'patch/packages/aws-cdk', + 'patch/packages/aws-cdk-lib/core', + 'project', + 'project/packages/aws-cdk', + 'project/packages/aws-cdk-lib/core' +]; diff --git a/tools/@aws-cdk/prlint/github.ts b/tools/@aws-cdk/prlint/github.ts new file mode 100644 index 0000000000000..30efa7135e812 --- /dev/null +++ b/tools/@aws-cdk/prlint/github.ts @@ -0,0 +1,36 @@ +import { Endpoints } from '@octokit/types'; +import { StatusEvent } from '@octokit/webhooks-definitions/schema'; +import type { components } from '@octokit/openapi-types'; + +export type GitHubPr = + Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}']['response']['data']; + + +export interface GitHubComment { + id: number; +} + +export interface Review { + id: number; + user: { + login: string; + }; + body: string; + state: string; +} + +export interface GithubStatusEvent { + readonly sha: string; + readonly state?: StatusEvent['state']; + readonly context?: string; +} + +export interface GitHubLabel { + readonly name: string; +} + +export interface GitHubFile { + readonly filename: string; +} + +export type CheckRunConclusion = components['schemas']['check-run']['conclusion'] diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 4512a28ad1ac9..6e66ec3e39c58 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -2,53 +2,69 @@ import * as core from '@actions/core'; import * as github from '@actions/github'; import { Octokit } from '@octokit/rest'; import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema'; -import * as linter from './lint'; +import { PullRequestLinter } from './lint'; async function run() { const token: string = process.env.GITHUB_TOKEN!; const client = new Octokit({ auth: token }); + const owner = github.context.repo.owner; + const repo = github.context.repo.repo; + + // Try to get a SHA from various places + let sha = process.env.PR_SHA; + if (!sha && github.context.eventName.startsWith('pull_request')) { + sha = (github.context.payload as PullRequestEvent)?.pull_request?.head?.sha; + } + if (!sha && github.context.eventName === 'status') { + sha = (github.context.payload as StatusEvent)?.sha; + } + if (!sha) { + throw new Error(`Could not determine a SHA from either \$PR_SHA or ${JSON.stringify(github.context.payload)}`); + } + + let number = process.env.PR_NUMBER ? Number(process.env.PR_NUMBER) : undefined; + if (number === undefined) { + const pr = await PullRequestLinter.getPRFromCommit(client, owner, repo, sha); + if (!pr && github.context.eventName === 'status') { + // For a "status" event, apparently it's fine to not be able to find the PR + console.error('Could not find appropriate PR for status event; exiting'); + process.exit(0); + } + + if (pr) { + number = pr.number; + } + } + + if (number === undefined) { + throw new Error(`Could not determine PR number from either \$PR_NUMBER or the SHA: ${sha}`); + } + try { + const prLinter = new PullRequestLinter({ + client, + owner, + repo, + number, + }); + switch (github.context.eventName) { case 'status': const statusPayload = github.context.payload as StatusEvent; - const pr = await linter.PullRequestLinter.getPRFromCommit(client, 'aws', 'aws-cdk', statusPayload.sha); + const pr = await PullRequestLinter.getPRFromCommit(client, owner, repo, statusPayload.sha); if (pr) { - const prLinter = new linter.PullRequestLinter({ - client, - owner: 'aws', - repo: 'aws-cdk', - number: pr.number, - }); console.log('validating status event'); - await prLinter.validateStatusEvent(pr, github.context.payload as StatusEvent); + await prLinter.validateStatusEvent(pr, statusPayload); } break; - case 'workflow_run': - const prNumber = process.env.PR_NUMBER; - const prSha = process.env.PR_SHA; - if (!prNumber || !prSha) { - throw new Error(`Cannot have undefined values in: ${prNumber} pr number and ${prSha} pr sha.`) - } - const workflowPrLinter = new linter.PullRequestLinter({ - client, - owner: github.context.repo.owner, - repo: github.context.repo.repo, - number: Number(prNumber), - }); - await workflowPrLinter.validatePullRequestTarget(prSha); - break; + default: - const payload = github.context.payload as PullRequestEvent; - const prLinter = new linter.PullRequestLinter({ - client, - owner: github.context.repo.owner, - repo: github.context.repo.repo, - number: github.context.issue.number, - }); - await prLinter.validatePullRequestTarget(payload.pull_request.head.sha); + await prLinter.validatePullRequestTarget(sha); + break; } } catch (error: any) { + // Fail the action core.setFailed(error.message); } } diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 547184e722fb2..c229136ac6bbc 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -1,349 +1,20 @@ import { execSync } from 'child_process'; import * as path from 'path'; -import { Octokit } from '@octokit/rest'; -import { Endpoints } from '@octokit/types'; -import { StatusEvent } from '@octokit/webhooks-definitions/schema'; -import type { components } from '@octokit/openapi-types'; import { findModulePath, moduleStability } from './module'; import { breakingModules } from './parser'; - -export type GitHubPr = - Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}']['response']['data']; - -export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; -export const CODECOV_PREFIX = 'codecov/'; - -export const CODECOV_CHECKS = [ - 'patch', - 'patch/packages/aws-cdk', - 'patch/packages/aws-cdk-lib/core', - 'project', - 'project/packages/aws-cdk', - 'project/packages/aws-cdk-lib/core' -]; - -type CheckRunConclusion = components['schemas']['check-run']['conclusion'] - -const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; - -/** - * Types of exemption labels in aws-cdk project. - */ -enum Exemption { - README = 'pr-linter/exempt-readme', - TEST = 'pr-linter/exempt-test', - INTEG_TEST = 'pr-linter/exempt-integ-test', - BREAKING_CHANGE = 'pr-linter/exempt-breaking-change', - CLI_INTEG_TESTED = 'pr-linter/cli-integ-tested', - REQUEST_CLARIFICATION = 'pr/reviewer-clarification-requested', - REQUEST_EXEMPTION = 'pr-linter/exemption-requested', - CODECOV = "pr-linter/exempt-codecov", -} - -export interface GithubStatusEvent { - readonly sha: string; - readonly state?: StatusEvent['state']; - readonly context?: string; -} - -export interface GitHubLabel { - readonly name: string; -} - -export interface GitHubFile { - readonly filename: string; -} - -export interface Review { - id: number; - user: { - login: string; - }; - body: string; - state: string; -} - -export interface Comment { - id: number; -} - -class LinterError extends Error { - constructor(message: string) { - super(message); - } -} - -/** - * Results of a single test. - * - * On a successful validation, no failures will be present. - * Some tests may return multiple failures. - */ -class TestResult { - /** - * Create a test result from a potential failure - */ - public static fromFailure(failureCondition: boolean, failureMessage: string): TestResult { - const ret = new TestResult(); - ret.assessFailure(failureCondition, failureMessage); - return ret; - } - - public errorMessages: string[] = []; - - /** - * Assesses the failure condition for the type of pull request being tested and adds the failure message - * to errorMessages if failures are present. - * @param failureCondition The conditions for this failure type. - * @param failureMessage The message to emit to the contributor. - */ - public assessFailure(failureCondition: boolean, failureMessage: string): void { - if (failureCondition) { - this.errorMessages.push(failureMessage); - } - } -} - -/** - * Represents a single test. - */ -interface Test { - test: (pr: GitHubPr, files: GitHubFile[]) => TestResult; -} - -/** - * Represents a set of tests and the conditions under which those rules exempt. - */ -interface ValidateRuleSetOptions { - /** - * The function to test for exemption from the rules in testRuleSet. - */ - exemption?: (pr: GitHubPr) => boolean; - - /** - * The log message printed if the exemption is granted. - */ - exemptionMessage?: string; - - /** - * The set of rules to test against if the pull request is not exempt. - */ - testRuleSet: Test[]; -} - -/** - * This class provides functionality for performing validation tests against each ruleset and - * collecting all the errors returned by those tests. - */ -class ValidationCollector { - public errors: string[] = []; - - constructor(private pr: GitHubPr, private files: GitHubFile[]) { } - - /** - * Checks for exemption criteria and then validates against the ruleset when not exempt to it. - * Any validation failures are collected by the ValidationCollector. - * @param validationOptions the options to validate against - */ - public validateRuleSet(validationOptions: ValidateRuleSetOptions): void { - if (validationOptions.exemption ? validationOptions.exemption(this.pr) : false) { - console.log(validationOptions.exemptionMessage); - } else { - this.errors = this.errors.concat(...validationOptions.testRuleSet.map(((test: Test) => test.test(this.pr, this.files).errorMessages))); - } - } - - /** - * Checks whether any validation errors have been collected. - * @returns boolean - */ - public isValid() { - return this.errors.length === 0; - } -} - -/** - * Props used to perform linting against the pull request. - */ -export interface PullRequestLinterProps { - /** - * GitHub client scoped to pull requests. Imported via @actions/github. - */ - readonly client: Octokit; - - /** - * Repository owner. - */ - readonly owner: string; - - /** - * Repository name. - */ - readonly repo: string; - - /** - * Pull request number. - */ - readonly number: number; -} +import { CheckRunConclusion, GitHubFile, GitHubPr, Review } from "./github"; +import { Test, TestResult, ValidationCollector } from './results'; +import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX, Exemption } from './constants'; +import { StatusEvent } from '@octokit/webhooks-definitions/schema'; +import { PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base'; /** * This class provides functionality to run lint checks against a pull request, request changes with the lint failures * in the body of the review, and dismiss any previous reviews upon changes to the pull request. */ -export class PullRequestLinter { - /** - * Find an open PR for the given commit. - * @param sha the commit sha to find the PR of - */ - public static async getPRFromCommit(client: Octokit, owner: string, repo: string, sha: string): Promise { - const prs = await client.search.issuesAndPullRequests({ - q: sha, - }); - console.log('Found PRs: ', prs); - const foundPr = prs.data.items.find(pr => pr.state === 'open'); - if (foundPr) { - // need to do this because the list PR response does not have - // all the necessary information - const pr = (await client.pulls.get({ - owner, - repo, - pull_number: foundPr.number, - })).data; - console.log(`PR: ${foundPr.number}: `, pr); - // only process latest commit - if (pr.head.sha === sha) { - return pr; - } - } - return; - } - - private readonly client: Octokit; - private readonly prParams: { owner: string, repo: string, pull_number: number }; - private readonly issueParams: { owner: string, repo: string, issue_number: number }; +export class PullRequestLinter extends PullRequestLinterBase { private readonly trustedCommunity: string[] = []; - constructor(private readonly props: PullRequestLinterProps) { - this.client = props.client; - this.prParams = { owner: props.owner, repo: props.repo, pull_number: props.number }; - this.issueParams = { owner: props.owner, repo: props.repo, issue_number: props.number }; - } - - /** - * Deletes the previous linter comment if it exists. - */ - private async deletePRLinterComment(): Promise { - // Since previous versions of this pr linter didn't add comments, we need to do this check first. - const comment = await this.findExistingPRLinterComment(); - if (comment) { - await this.client.issues.deleteComment({ - ...this.issueParams, - comment_id: comment.id, - }); - }; - }; - - /** - * Dismisses previous reviews by aws-cdk-automation when the pull request succeeds the linter. - * @param existingReview The review created by a previous run of the linter - */ - private async dismissPRLinterReview(existingReview?: Review): Promise { - if (existingReview) { - await this.client.pulls.dismissReview({ - ...this.prParams, - review_id: existingReview.id, - message: '✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.', - }); - } - } - - /** - * Creates a new review and comment for first run with failure or creates a new comment with new failures for existing reviews. - * @param failureMessages The failures received by the pr linter validation checks. - * @param existingReview The review created by a previous run of the linter. - */ - private async createOrUpdatePRLinterReview(failureMessages: string[], existingReview?: Review): Promise { - let body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` - + 'PRs must pass status checks before we can provide a meaningful review.\n\n' - + 'If you would like to request an exemption from the status checks or clarification on feedback,' - + ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.'; - if (!existingReview) { - await this.client.pulls.createReview({ - ...this.prParams, - body: 'The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons.' - + ' If you believe this pull request should receive an exemption, please comment and provide a justification.' - + '\n\n\nA comment requesting an exemption should contain the text `Exemption Request`.' - + ' Additionally, if clarification is needed add `Clarification Request` to a comment.', - event: 'REQUEST_CHANGES', - }); - } - - const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - if (comments.find(comment => comment.body?.toLowerCase().includes("exemption request"))) { - body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; - } - await this.client.issues.createComment({ - ...this.issueParams, - body, - }); - - // Closing the PR if it is opened from main branch of author's fork - if (failureMessages.includes(PR_FROM_MAIN_ERROR)) { - - const errorMessageBody = 'Your pull request must be based off of a branch in a personal account ' - + '(not an organization owned account, and not the main branch). You must also have the setting ' - + 'enabled that allows the CDK team to push changes to your branch ' - + '(this setting is enabled by default for personal accounts, and cannot be enabled for organization owned accounts). ' - + 'The reason for this is that our automation needs to synchronize your branch with our main after it has been approved, ' - + 'and we cannot do that if we cannot push to your branch.' - - await this.client.issues.createComment({ - ...this.issueParams, - body: errorMessageBody, - }); - - await this.client.pulls.update({ - ...this.prParams, - state: 'closed', - }); - } - - throw new LinterError(body); - } - - /** - * Finds existing review, if present - * @returns Existing review, if present - */ - private async findExistingPRLinterReview(): Promise { - const reviews = await this.client.paginate(this.client.pulls.listReviews, this.prParams); - return reviews.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review; - } - - /** - * Finds existing comment from previous review, if present - * @returns Existing comment, if present - */ - private async findExistingPRLinterComment(): Promise { - const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - return comments.find((comment) => comment.user?.login === 'aws-cdk-automation' && comment.body?.startsWith('The pull request linter fails with the following errors:')) as Comment; - } - - /** - * Creates a new review, requesting changes, with the reasons that the linter did not pass. - * @param result The result of the PR Linter run. - */ - private async communicateResult(result: ValidationCollector): Promise { - const existingReview = await this.findExistingPRLinterReview(); - if (result.isValid()) { - console.log('✅ Success'); - await this.dismissPRLinterReview(existingReview); - } else { - await this.createOrUpdatePRLinterReview(result.errors, existingReview); - } - } - /** * Whether or not the codebuild job for the given commit is successful * @@ -366,7 +37,7 @@ export class PullRequestLinter { } } - private async checkRunConclusion(sha: string, checkName: string): Promise { + private async checkRunConclusion(sha: string, checkName: string): Promise { const response = await this.client.paginate(this.client.checks.listForRef, { owner: this.prParams.owner, repo: this.prParams.repo, @@ -504,32 +175,6 @@ export class PullRequestLinter { } } - private addLabel(label: string, pr: Pick) { - // already has label, so no-op - if (pr.labels.some(l => l.name === label)) { return; } - console.log(`adding ${label} to pr ${pr.number}`); - this.client.issues.addLabels({ - issue_number: pr.number, - owner: this.prParams.owner, - repo: this.prParams.repo, - labels: [ - label, - ], - }); - } - - private removeLabel(label: string, pr: Pick) { - // does not have label, so no-op - if (!pr.labels.some(l => l.name === label)) { return; } - console.log(`removing ${label} to pr ${pr.number}`); - this.client.issues.removeLabel({ - issue_number: pr.number, - owner: this.prParams.owner, - repo: this.prParams.repo, - name: label, - }); - } - /** * Trusted community reviewers is derived from the source of truth at this wiki: * https://github.com/aws/aws-cdk/wiki/CDK-Community-PR-Reviews @@ -622,11 +267,11 @@ export class PullRequestLinter { } }) } - + validationCollector.validateRuleSet({ exemption: shouldExemptCodecov, testRuleSet: codecovTests, - }); + }); } console.log("Deleting PR Linter Comment now"); @@ -648,98 +293,95 @@ export class PullRequestLinter { } } } - - private formatErrors(errors: string[]) { - return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; - }; } -function isFeature(pr: GitHubPr): boolean { +export function isFeature(pr: GitHubPr): boolean { return pr.title.startsWith('feat'); } -function isFix(pr: GitHubPr): boolean { +export function isFix(pr: GitHubPr): boolean { return pr.title.startsWith('fix'); } -function testChanged(files: GitHubFile[]): boolean { +export function testChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('test')).length != 0; } -function integTestChanged(files: GitHubFile[]): boolean { +export function integTestChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().match(/integ.*.ts$/)).length != 0; } -function integTestSnapshotChanged(files: GitHubFile[]): boolean { +export function integTestSnapshotChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('.snapshot')).length != 0; } -function readmeChanged(files: GitHubFile[]): boolean { +export function readmeChanged(files: GitHubFile[]): boolean { return files.filter(f => path.basename(f.filename) == 'README.md').length != 0; } -function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { +export function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !readmeChanged(files), 'Features must contain a change to a README file.'); return result; } -function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +export function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !testChanged(files), 'Features must contain a change to a test file.'); return result; } -function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +export function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && !testChanged(files), 'Fixes must contain a change to a test file.'); return result; } -function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +export function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Features must contain a change to an integration test file and the resulting snapshot.'); return result; } -function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +export function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Fixes must contain a change to an integration test file and the resulting snapshot.'); return result; } -function shouldExemptCodecov(pr: GitHubPr): boolean { +export function shouldExemptCodecov(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.CODECOV); } -function shouldExemptReadme(pr: GitHubPr): boolean { +export function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); } -function shouldExemptTest(pr: GitHubPr): boolean { +export function shouldExemptTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.TEST); } -function shouldExemptIntegTest(pr: GitHubPr): boolean { +export function shouldExemptIntegTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.INTEG_TEST); } -function shouldExemptBreakingChange(pr: GitHubPr): boolean { +export function shouldExemptBreakingChange(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.BREAKING_CHANGE); } -function shouldExemptCliIntegTested(pr: GitHubPr): boolean { +export function shouldExemptCliIntegTested(pr: GitHubPr): boolean { return (hasLabel(pr, Exemption.CLI_INTEG_TESTED) || pr.user?.login === 'aws-cdk-automation'); } -function hasLabel(pr: GitHubPr, labelName: string): boolean { +export function hasLabel(pr: GitHubPr, labelName: string): boolean { return pr.labels.some(function (l: any) { return l.name === labelName; }); } + /** * Check that the 'BREAKING CHANGE:' note in the body is correct. * diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts new file mode 100644 index 0000000000000..0bf3cb75cbfcb --- /dev/null +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -0,0 +1,226 @@ +import { Octokit } from '@octokit/rest'; +import { GitHubComment, GitHubPr, Review } from "./github"; +import { ValidationCollector } from './results'; + +export const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; + +/** + * Props used to perform linting against the pull request. + */ +export interface PullRequestLinterBaseProps { + /** + * GitHub client scoped to pull requests. Imported via @actions/github. + */ + readonly client: Octokit; + + /** + * Repository owner. + */ + readonly owner: string; + + /** + * Repository name. + */ + readonly repo: string; + + /** + * Pull request number. + */ + readonly number: number; +} + + +/** + * Base "interacting with GitHub" functionality, devoid of any specific validation logic. + * + * Inheritance is not great, but this was the easiest way to factor this out for now. + */ +export class PullRequestLinterBase { + /** + * Find an open PR for the given commit. + * @param sha the commit sha to find the PR of + */ + public static async getPRFromCommit(client: Octokit, owner: string, repo: string, sha: string): Promise { + const prs = await client.search.issuesAndPullRequests({ + q: sha, + }); + console.log('Found PRs: ', prs); + const foundPr = prs.data.items.find(pr => pr.state === 'open'); + if (foundPr) { + // need to do this because the list PR response does not have + // all the necessary information + const pr = (await client.pulls.get({ + owner, + repo, + pull_number: foundPr.number, + })).data; + console.log(`PR: ${foundPr.number}: `, pr); + // only process latest commit + if (pr.head.sha === sha) { + return pr; + } + } + return; + } + + protected readonly client: Octokit; + protected readonly prParams: { owner: string, repo: string, pull_number: number }; + protected readonly issueParams: { owner: string, repo: string, issue_number: number }; + + constructor(readonly props: PullRequestLinterBaseProps) { + this.client = props.client; + this.prParams = { owner: props.owner, repo: props.repo, pull_number: props.number }; + this.issueParams = { owner: props.owner, repo: props.repo, issue_number: props.number }; + } + + /** + * Deletes the previous linter comment if it exists. + */ + protected async deletePRLinterComment(): Promise { + // Since previous versions of this pr linter didn't add comments, we need to do this check first. + const comment = await this.findExistingPRLinterComment(); + if (comment) { + await this.client.issues.deleteComment({ + ...this.issueParams, + comment_id: comment.id, + }); + }; + }; + + /** + * Dismisses previous reviews by aws-cdk-automation when the pull request succeeds the linter. + * @param existingReview The review created by a previous run of the linter + */ + protected async dismissPRLinterReview(existingReview?: Review): Promise { + if (existingReview) { + await this.client.pulls.dismissReview({ + ...this.prParams, + review_id: existingReview.id, + message: '✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.', + }); + } + } + + /** + * Creates a new review, requesting changes, with the reasons that the linter did not pass. + * @param result The result of the PR Linter run. + */ + protected async communicateResult(result: ValidationCollector): Promise { + const existingReview = await this.findExistingPRLinterReview(); + if (result.isValid()) { + console.log('✅ Success'); + await this.dismissPRLinterReview(existingReview); + } else { + await this.createOrUpdatePRLinterReview(result.errors, existingReview); + } + } + + /** + * Finds existing review, if present + * @returns Existing review, if present + */ + private async findExistingPRLinterReview(): Promise { + const reviews = await this.client.paginate(this.client.pulls.listReviews, this.prParams); + return reviews.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review; + } + + /** + * Finds existing comment from previous review, if present + * @returns Existing comment, if present + */ + private async findExistingPRLinterComment(): Promise { + const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); + return comments.find((comment) => comment.user?.login === 'aws-cdk-automation' && comment.body?.startsWith('The pull request linter fails with the following errors:')) as GitHubComment; + } + + /** + * Creates a new review and comment for first run with failure or creates a new comment with new failures for existing reviews. + * @param failureMessages The failures received by the pr linter validation checks. + * @param existingReview The review created by a previous run of the linter. + */ + private async createOrUpdatePRLinterReview(failureMessages: string[], existingReview?: Review): Promise { + let body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` + + 'PRs must pass status checks before we can provide a meaningful review.\n\n' + + 'If you would like to request an exemption from the status checks or clarification on feedback,' + + ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.'; + if (!existingReview) { + await this.client.pulls.createReview({ + ...this.prParams, + body: 'The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons.' + + ' If you believe this pull request should receive an exemption, please comment and provide a justification.' + + '\n\n\nA comment requesting an exemption should contain the text `Exemption Request`.' + + ' Additionally, if clarification is needed add `Clarification Request` to a comment.', + event: 'REQUEST_CHANGES', + }); + } + + const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); + if (comments.find(comment => comment.body?.toLowerCase().includes("exemption request"))) { + body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; + } + await this.client.issues.createComment({ + ...this.issueParams, + body, + }); + + // Closing the PR if it is opened from main branch of author's fork + if (failureMessages.includes(PR_FROM_MAIN_ERROR)) { + + const errorMessageBody = 'Your pull request must be based off of a branch in a personal account ' + + '(not an organization owned account, and not the main branch). You must also have the setting ' + + 'enabled that allows the CDK team to push changes to your branch ' + + '(this setting is enabled by default for personal accounts, and cannot be enabled for organization owned accounts). ' + + 'The reason for this is that our automation needs to synchronize your branch with our main after it has been approved, ' + + 'and we cannot do that if we cannot push to your branch.' + + await this.client.issues.createComment({ + ...this.issueParams, + body: errorMessageBody, + }); + + await this.client.pulls.update({ + ...this.prParams, + state: 'closed', + }); + } + + throw new LinterError(body); + } + + private formatErrors(errors: string[]) { + return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; + } + + protected addLabel(label: string, pr: Pick) { + // already has label, so no-op + if (pr.labels.some(l => l.name === label)) { return; } + console.log(`adding ${label} to pr ${pr.number}`); + this.client.issues.addLabels({ + issue_number: pr.number, + owner: this.prParams.owner, + repo: this.prParams.repo, + labels: [ + label, + ], + }); + } + + protected removeLabel(label: string, pr: Pick) { + // does not have label, so no-op + if (!pr.labels.some(l => l.name === label)) { return; } + console.log(`removing ${label} to pr ${pr.number}`); + this.client.issues.removeLabel({ + issue_number: pr.number, + owner: this.prParams.owner, + repo: this.prParams.repo, + name: label, + }); + } + +} + +export class LinterError extends Error { + constructor(message: string) { + super(message); + } +} \ No newline at end of file diff --git a/tools/@aws-cdk/prlint/results.ts b/tools/@aws-cdk/prlint/results.ts new file mode 100644 index 0000000000000..7784637a2f4a4 --- /dev/null +++ b/tools/@aws-cdk/prlint/results.ts @@ -0,0 +1,92 @@ +import { GitHubFile, GitHubPr } from "./github"; + +/** + * Results of a single test. + * + * On a successful validation, no failures will be present. + * Some tests may return multiple failures. + */ +export class TestResult { + /** + * Create a test result from a potential failure + */ + public static fromFailure(failureCondition: boolean, failureMessage: string): TestResult { + const ret = new TestResult(); + ret.assessFailure(failureCondition, failureMessage); + return ret; + } + + public errorMessages: string[] = []; + + /** + * Assesses the failure condition for the type of pull request being tested and adds the failure message + * to errorMessages if failures are present. + * @param failureCondition The conditions for this failure type. + * @param failureMessage The message to emit to the contributor. + */ + public assessFailure(failureCondition: boolean, failureMessage: string): void { + if (failureCondition) { + this.errorMessages.push(failureMessage); + } + } +} + +/** + * Represents a set of tests and the conditions under which those rules exempt. + */ +export interface ValidateRuleSetOptions { + /** + * The function to test for exemption from the rules in testRuleSet. + */ + exemption?: (pr: GitHubPr) => boolean; + + /** + * The log message printed if the exemption is granted. + */ + exemptionMessage?: string; + + /** + * The set of rules to test against if the pull request is not exempt. + */ + testRuleSet: Test[]; +} + + + +/** + * This class provides functionality for performing validation tests against each ruleset and + * collecting all the errors returned by those tests. + */ +export class ValidationCollector { + public errors: string[] = []; + + constructor(private pr: GitHubPr, private files: GitHubFile[]) { } + + /** + * Checks for exemption criteria and then validates against the ruleset when not exempt to it. + * Any validation failures are collected by the ValidationCollector. + * @param validationOptions the options to validate against + */ + public validateRuleSet(validationOptions: ValidateRuleSetOptions): void { + if (validationOptions.exemption ? validationOptions.exemption(this.pr) : false) { + console.log(validationOptions.exemptionMessage); + } else { + this.errors = this.errors.concat(...validationOptions.testRuleSet.map(((test: Test) => test.test(this.pr, this.files).errorMessages))); + } + } + + /** + * Checks whether any validation errors have been collected. + * @returns boolean + */ + public isValid() { + return this.errors.length === 0; + } +} + +/** + * Represents a single test. + */ +export interface Test { + test: (pr: GitHubPr, files: GitHubFile[]) => TestResult; +} diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index d287f27186c74..8cf24d67d0d72 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1,5 +1,7 @@ import * as path from 'path'; -import * as linter from '../lint'; +import { GitHubFile, GitHubLabel, GitHubPr } from '../github'; +import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX } from '../constants'; +import { PullRequestLinter } from '../lint'; let mockRemoveLabel = jest.fn(); let mockAddLabel = jest.fn(); @@ -10,7 +12,7 @@ let mockListReviews = jest.fn().mockImplementation((_props: { _owner: string, _r beforeAll(() => { jest.spyOn(console, 'log').mockImplementation(); - jest.spyOn(linter.PullRequestLinter.prototype as any, 'getTrustedCommunityMembers').mockImplementation(() => ['trusted1', 'trusted2', 'trusted3']) + jest.spyOn(PullRequestLinter.prototype as any, 'getTrustedCommunityMembers').mockImplementation(() => ['trusted1', 'trusted2', 'trusted3']) process.env.REPO_ROOT = path.join(__dirname, '..', '..', '..', '..'); }); @@ -23,7 +25,7 @@ afterAll(() => { jest.resetAllMocks(); }); -let mockCreateReview: (errorMessage: string) => Promise; +let mockCreateReview: (errorMessage: string) => Promise = jest.fn(); const SHA = 'ABC'; type Subset = { @@ -38,7 +40,7 @@ type Subset = { describe('breaking changes format', () => { test('disallow variations to "BREAKING CHANGE:"', async () => { - const issue: Subset = { + const issue: Subset = { number: 1, title: 'chore: some title', body: 'BREAKING CHANGES:', @@ -96,7 +98,7 @@ describe('breaking changes format', () => { }); test('disallow PRs from main branch of fork', async () => { - const issue: Subset = { + const issue: Subset = { number: 1, title: 'chore: some title', body: 'making a pr from main of my fork', @@ -521,7 +523,7 @@ describe('integration tests required on features', () => { }); describe('CLI file changed', () => { - const labels: linter.GitHubLabel[] = []; + const labels: GitHubLabel[] = []; const issue = { number: 23, title: 'chore(cli): change the help or something', @@ -582,7 +584,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -602,7 +604,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -633,7 +635,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -664,7 +666,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -701,7 +703,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -734,7 +736,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -767,7 +769,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -807,7 +809,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -845,7 +847,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -875,7 +877,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -903,7 +905,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -932,7 +934,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -962,7 +964,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -995,7 +997,7 @@ describe('integration tests required on features', () => { const prLinter = configureMock(pr); await prLinter.validateStatusEvent(pr as any, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -1041,7 +1043,7 @@ describe('integration tests required on features', () => { describe('with existing Exemption Request comment', () => { test('valid exemption request comment', async () => { - const issue: Subset = { + const issue: Subset = { number: 1, title: 'fix: some title', body: '', @@ -1062,7 +1064,7 @@ describe('integration tests required on features', () => { }); test('valid exemption request with additional context', async () => { - const issue: Subset = { + const issue: Subset = { number: 1, title: 'fix: some title', body: '', @@ -1083,7 +1085,7 @@ describe('integration tests required on features', () => { }); test('valid exemption request with middle exemption request', async () => { - const issue: Subset = { + const issue: Subset = { number: 1, title: 'fix: some title', body: '', @@ -1105,7 +1107,7 @@ describe('integration tests required on features', () => { }); describe('metadata file changed', () => { - const files: linter.GitHubFile[] = [{ + const files: GitHubFile[] = [{ filename: 'packages/aws-cdk-lib/region-info/build-tools/metadata.ts', }]; @@ -1139,7 +1141,7 @@ describe('integration tests required on features', () => { }); }); -function configureMock(pr: Subset, prFiles?: linter.GitHubFile[], existingComments?: string[]): linter.PullRequestLinter { +function configureMock(pr: Subset, prFiles?: GitHubFile[], existingComments?: string[]): PullRequestLinter { const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) { return { data: { ...pr, base: { ref: 'main'}} }; @@ -1183,7 +1185,7 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ listCommitStatusesForRef() { return { data: [{ - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', }], }; @@ -1193,8 +1195,8 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ const checksClient = { listForRef() { return { - data: linter.CODECOV_CHECKS.map(c => ({ - name: `${linter.CODECOV_PREFIX}${c}`, + data: CODECOV_CHECKS.map(c => ({ + name: `${CODECOV_PREFIX}${c}`, conclusion: 'success', completed_at: '1' })), @@ -1205,7 +1207,7 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ const searchClient = { issuesAndPullRequests() {}, }; - return new linter.PullRequestLinter({ + return new PullRequestLinter({ owner: 'aws', repo: 'aws-cdk', number: 1000, From a425b37867740959f4ae2279c7638deb9a4ab979 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 11:43:18 +0100 Subject: [PATCH 06/16] Separate evaluation from execution --- ...igger.yml => pr-linter-review-trigger.yml} | 7 +- tools/@aws-cdk/prlint/index.ts | 24 +++- tools/@aws-cdk/prlint/lint.ts | 120 +++++++++++------- tools/@aws-cdk/prlint/linter-base.ts | 117 ++++++++++++++--- tools/@aws-cdk/prlint/test/lint.test.ts | 28 ++-- 5 files changed, 208 insertions(+), 88 deletions(-) rename .github/workflows/{pr-linter-trigger.yml => pr-linter-review-trigger.yml} (67%) diff --git a/.github/workflows/pr-linter-trigger.yml b/.github/workflows/pr-linter-review-trigger.yml similarity index 67% rename from .github/workflows/pr-linter-trigger.yml rename to .github/workflows/pr-linter-review-trigger.yml index 6a6cb809f8d9f..8408bf1b0fed4 100644 --- a/.github/workflows/pr-linter-trigger.yml +++ b/.github/workflows/pr-linter-review-trigger.yml @@ -1,7 +1,8 @@ -# Unprivileged workflow that runs in the context of the PR. +# Unprivileged workflow that runs in the context of the PR, when a review is changed. # -# Save the PR number, and download it again in the context of the PR Linter, -# which runs privileged. +# Save the PR number, and download it again in the PR Linter workflow which +# needs to run in privileged `workflow_run` context (but then must restore the +# PR context). name: PR Linter Trigger on: diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 6e66ec3e39c58..ca52ae92f7c92 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -3,6 +3,7 @@ import * as github from '@actions/github'; import { Octokit } from '@octokit/rest'; import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema'; import { PullRequestLinter } from './lint'; +import { LinterActions } from './linter-base'; async function run() { const token: string = process.env.GITHUB_TOKEN!; @@ -49,20 +50,31 @@ async function run() { number, }); + let actions: LinterActions | undefined; + switch (github.context.eventName) { case 'status': const statusPayload = github.context.payload as StatusEvent; - const pr = await PullRequestLinter.getPRFromCommit(client, owner, repo, statusPayload.sha); - if (pr) { - console.log('validating status event'); - await prLinter.validateStatusEvent(pr, statusPayload); - } + console.log('validating status event'); + actions = await prLinter.validateStatusEvent(statusPayload); break; default: - await prLinter.validatePullRequestTarget(sha); + actions = await prLinter.validatePullRequestTarget(sha); break; } + + if (actions) { + console.log(actions); + + if (github.context.eventName || process.env.FOR_REAL) { + console.log('Carrying out actions'); + + // Actually running in GitHub actions, do it (otherwise we're just testing) + await prLinter.executeActions(actions); + } + } + } catch (error: any) { // Fail the action core.setFailed(error.message); diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index c229136ac6bbc..ad682fb9a04c2 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -6,7 +6,7 @@ import { CheckRunConclusion, GitHubFile, GitHubPr, Review } from "./github"; import { Test, TestResult, ValidationCollector } from './results'; import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX, Exemption } from './constants'; import { StatusEvent } from '@octokit/webhooks-definitions/schema'; -import { PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base'; +import { LinterActions, mergeLinterActions, PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base'; /** * This class provides functionality to run lint checks against a pull request, request changes with the lint failures @@ -31,10 +31,11 @@ export class PullRequestLinter extends PullRequestLinterBase { return statuses.data.some(status => status.context === CODE_BUILD_CONTEXT && status.state === 'success'); } - public async validateStatusEvent(pr: GitHubPr, status: StatusEvent): Promise { + public async validateStatusEvent(status: StatusEvent): Promise { if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') { - await this.assessNeedsReview(pr); + return await this.assessNeedsReview(); } + return {}; } private async checkRunConclusion(sha: string, checkName: string): Promise { @@ -72,9 +73,9 @@ export class PullRequestLinter extends PullRequestLinterBase { * 6. It links to a p1 issue * 7. It links to a p2 issue and has an approved community review */ - private async assessNeedsReview( - pr: Pick, - ): Promise { + private async assessNeedsReview(): Promise { + const pr = await this.pr(); + const reviewsData = await this.client.paginate(this.client.pulls.listReviews, this.prParams); console.log(JSON.stringify(reviewsData)); @@ -164,14 +165,22 @@ export class PullRequestLinter extends PullRequestLinterBase { // 2) is already community approved // 3) is authored by a core team member if (readyForReview && (fixesP1 || communityApproved || pr.labels.some(label => label.name === 'contribution/core'))) { - this.addLabel('pr/needs-maintainer-review', pr); - this.removeLabel('pr/needs-community-review', pr); + return { + addLabels: ['pr/needs-maintainer-review'], + removeLabels: ['pr/needs-community-review'], + }; } else if (readyForReview && !fixesP1) { - this.removeLabel('pr/needs-maintainer-review', pr); - this.addLabel('pr/needs-community-review', pr); + return { + addLabels: ['pr/needs-community-review'], + removeLabels: ['pr/needs-maintainer-review'], + }; } else { - this.removeLabel('pr/needs-community-review', pr); - this.removeLabel('pr/needs-maintainer-review', pr); + return { + removeLabels: [ + 'pr/needs-community-review', + 'pr/needs-maintainer-review', + ], + }; } } @@ -194,11 +203,13 @@ export class PullRequestLinter extends PullRequestLinterBase { * Performs validations and communicates results via pull request comments, upon failure. * This also dismisses previous reviews so they do not remain in REQUEST_CHANGES upon fix of failures. */ - public async validatePullRequestTarget(sha: string): Promise { + public async validatePullRequestTarget(sha: string): Promise { + let ret: LinterActions = {}; + const number = this.props.number; console.log(`⌛ Fetching PR number ${number}`); - const pr = (await this.client.pulls.get(this.prParams)).data as GitHubPr; + const pr = await this.pr(); console.log(`PR base ref is: ${pr.base.ref}`) console.log(`⌛ Fetching files for PR number ${number}`); @@ -275,107 +286,124 @@ export class PullRequestLinter extends PullRequestLinterBase { } console.log("Deleting PR Linter Comment now"); - await this.deletePRLinterComment(); + ret.deleteSingletonPrLinterComment = true; + + ret = mergeLinterActions(ret, await this.communicateResult(validationCollector)); + + // also assess whether the PR needs review or not try { - await this.communicateResult(validationCollector); - // always assess the review, even if the linter fails - } finally { - // also assess whether the PR needs review or not - try { - const state = await this.codeBuildJobSucceeded(sha); - console.log(`PR code build job ${state ? "SUCCESSFUL" : "not yet successful"}`); - if (state) { - console.log('Assessing if the PR needs a review now'); - await this.assessNeedsReview(pr); - } - } catch (e) { - console.log(`assessing review failed for sha ${sha}: `, e); + const state = await this.codeBuildJobSucceeded(sha); + console.log(`PR code build job ${state ? "SUCCESSFUL" : "not yet successful"}`); + if (state) { + console.log('Assessing if the PR needs a review now'); + ret = mergeLinterActions(ret, await this.assessNeedsReview()); } + } catch (e) { + console.log(`assessing review failed for sha ${sha}: `, e); + } + + return ret; + } + + /** + * Creates a new review, requesting changes, with the reasons that the linter did not pass. + * @param result The result of the PR Linter run. + */ + private async communicateResult(result: ValidationCollector): Promise { + if (result.isValid()) { + console.log('✅ Success'); + return { + dismissPreviousReview: true, + }; + } else { + return { + requestChanges: { failures: result.errors }, + }; } } } -export function isFeature(pr: GitHubPr): boolean { +function isFeature(pr: GitHubPr): boolean { return pr.title.startsWith('feat'); } -export function isFix(pr: GitHubPr): boolean { +function isFix(pr: GitHubPr): boolean { return pr.title.startsWith('fix'); } -export function testChanged(files: GitHubFile[]): boolean { +function testChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('test')).length != 0; } -export function integTestChanged(files: GitHubFile[]): boolean { +function integTestChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().match(/integ.*.ts$/)).length != 0; } -export function integTestSnapshotChanged(files: GitHubFile[]): boolean { +function integTestSnapshotChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('.snapshot')).length != 0; } -export function readmeChanged(files: GitHubFile[]): boolean { +function readmeChanged(files: GitHubFile[]): boolean { return files.filter(f => path.basename(f.filename) == 'README.md').length != 0; } -export function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { +function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !readmeChanged(files), 'Features must contain a change to a README file.'); return result; } -export function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !testChanged(files), 'Features must contain a change to a test file.'); return result; } -export function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && !testChanged(files), 'Fixes must contain a change to a test file.'); return result; } -export function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Features must contain a change to an integration test file and the resulting snapshot.'); return result; } -export function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { +function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Fixes must contain a change to an integration test file and the resulting snapshot.'); return result; } -export function shouldExemptCodecov(pr: GitHubPr): boolean { +function shouldExemptCodecov(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.CODECOV); } -export function shouldExemptReadme(pr: GitHubPr): boolean { +function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); } -export function shouldExemptTest(pr: GitHubPr): boolean { +function shouldExemptTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.TEST); } -export function shouldExemptIntegTest(pr: GitHubPr): boolean { +function shouldExemptIntegTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.INTEG_TEST); } -export function shouldExemptBreakingChange(pr: GitHubPr): boolean { +function shouldExemptBreakingChange(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.BREAKING_CHANGE); } -export function shouldExemptCliIntegTested(pr: GitHubPr): boolean { +function shouldExemptCliIntegTested(pr: GitHubPr): boolean { return (hasLabel(pr, Exemption.CLI_INTEG_TESTED) || pr.user?.login === 'aws-cdk-automation'); } -export function hasLabel(pr: GitHubPr, labelName: string): boolean { +function hasLabel(pr: GitHubPr, labelName: string): boolean { return pr.labels.some(function (l: any) { return l.name === labelName; }); diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index 0bf3cb75cbfcb..d2b31cf581056 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -1,6 +1,5 @@ import { Octokit } from '@octokit/rest'; import { GitHubComment, GitHubPr, Review } from "./github"; -import { ValidationCollector } from './results'; export const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; @@ -67,16 +66,60 @@ export class PullRequestLinterBase { protected readonly prParams: { owner: string, repo: string, pull_number: number }; protected readonly issueParams: { owner: string, repo: string, issue_number: number }; + private _pr: GitHubPr | undefined; + constructor(readonly props: PullRequestLinterBaseProps) { this.client = props.client; this.prParams = { owner: props.owner, repo: props.repo, pull_number: props.number }; this.issueParams = { owner: props.owner, repo: props.repo, issue_number: props.number }; } + public async pr(): Promise { + if (!this._pr) { + const r = await this.client.pulls.get(this.prParams); + this._pr = r.data; + } + return this._pr; + } + + /** + * Execute the given set of actions + */ + public async executeActions(actions: LinterActions) { + const pr = await this.pr(); + + for (const label of actions.removeLabels ?? []) { + this.removeLabel(label, pr); + } + + for (const label of actions.addLabels ?? []) { + this.addLabel(label, pr); + } + + if (actions.deleteSingletonPrLinterComment) { + this.deletePRLinterComment(); + } + + if (actions.dismissPreviousReview || actions.requestChanges) { + if (actions.dismissPreviousReview && actions.requestChanges) { + throw new Error(`It does not make sense to supply both dismissPreviousReview and requestChanges: ${JSON.stringify(actions)}`); + } + + const existingReview = await this.findExistingPRLinterReview(); + + if (actions.dismissPreviousReview) { + this.dismissPRLinterReview(existingReview); + } + if (actions.requestChanges) { + this.createOrUpdatePRLinterReview(actions.requestChanges.failures, existingReview); + } + } + } + /** * Deletes the previous linter comment if it exists. */ - protected async deletePRLinterComment(): Promise { + private async deletePRLinterComment(): Promise { // Since previous versions of this pr linter didn't add comments, we need to do this check first. const comment = await this.findExistingPRLinterComment(); if (comment) { @@ -91,7 +134,7 @@ export class PullRequestLinterBase { * Dismisses previous reviews by aws-cdk-automation when the pull request succeeds the linter. * @param existingReview The review created by a previous run of the linter */ - protected async dismissPRLinterReview(existingReview?: Review): Promise { + private async dismissPRLinterReview(existingReview?: Review): Promise { if (existingReview) { await this.client.pulls.dismissReview({ ...this.prParams, @@ -101,20 +144,6 @@ export class PullRequestLinterBase { } } - /** - * Creates a new review, requesting changes, with the reasons that the linter did not pass. - * @param result The result of the PR Linter run. - */ - protected async communicateResult(result: ValidationCollector): Promise { - const existingReview = await this.findExistingPRLinterReview(); - if (result.isValid()) { - console.log('✅ Success'); - await this.dismissPRLinterReview(existingReview); - } else { - await this.createOrUpdatePRLinterReview(result.errors, existingReview); - } - } - /** * Finds existing review, if present * @returns Existing review, if present @@ -135,10 +164,14 @@ export class PullRequestLinterBase { /** * Creates a new review and comment for first run with failure or creates a new comment with new failures for existing reviews. + * * @param failureMessages The failures received by the pr linter validation checks. * @param existingReview The review created by a previous run of the linter. */ private async createOrUpdatePRLinterReview(failureMessages: string[], existingReview?: Review): Promise { + // FIXME: this function is doing too much at once. We should split this out into separate + // actions on `LinterActions`. + let body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` + 'PRs must pass status checks before we can provide a meaningful review.\n\n' + 'If you would like to request an exemption from the status checks or clarification on feedback,' @@ -191,7 +224,7 @@ export class PullRequestLinterBase { return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; } - protected addLabel(label: string, pr: Pick) { + private addLabel(label: string, pr: Pick) { // already has label, so no-op if (pr.labels.some(l => l.name === label)) { return; } console.log(`adding ${label} to pr ${pr.number}`); @@ -205,7 +238,7 @@ export class PullRequestLinterBase { }); } - protected removeLabel(label: string, pr: Pick) { + private removeLabel(label: string, pr: Pick) { // does not have label, so no-op if (!pr.labels.some(l => l.name === label)) { return; } console.log(`removing ${label} to pr ${pr.number}`); @@ -223,4 +256,50 @@ export class LinterError extends Error { constructor(message: string) { super(message); } +} + +/** + * Actions that the PR linter should carry out + */ +export interface LinterActions { + /** + * Add labels to the PR + */ + addLabels?: string[]; + + /** + * Remove labels from the PR + */ + removeLabels?: string[]; + + /** + * Delete the singleton PR linter comment + */ + deleteSingletonPrLinterComment?: boolean; + + /** + * Post a "request changes" review + */ + requestChanges?: { + failures: string[]; + }; + + /** + * Dismiss the PR linter's previous review. + */ + dismissPreviousReview?: boolean; +} + +export function mergeLinterActions(a: LinterActions, b: LinterActions): LinterActions { + return { + addLabels: nonEmpty([...(a.addLabels ?? []), ...(b.addLabels ?? [])]), + removeLabels: nonEmpty([...(a.removeLabels ?? []), ...(b.removeLabels ?? [])]), + deleteSingletonPrLinterComment: b.deleteSingletonPrLinterComment ?? a.deleteSingletonPrLinterComment, + requestChanges: b.requestChanges ?? a.requestChanges, + dismissPreviousReview: b.dismissPreviousReview ?? a.dismissPreviousReview, + }; +} + +function nonEmpty(xs: A[]): A[] | undefined { + return xs.length > 0 ? xs : undefined; } \ No newline at end of file diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 8cf24d67d0d72..17c5b824216b9 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -582,7 +582,7 @@ describe('integration tests required on features', () => { test('needs a review', async () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -602,7 +602,7 @@ describe('integration tests required on features', () => { // WHEN pr.labels = [{ name: 'p1' }]; const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -633,7 +633,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -664,7 +664,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -701,7 +701,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -734,7 +734,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -767,7 +767,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -807,7 +807,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -845,7 +845,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -875,7 +875,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -903,7 +903,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -932,7 +932,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -962,7 +962,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -995,7 +995,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await prLinter.validateStatusEvent({ sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', From 4a6c6f10487f316f5d022e1eda7826619db8ae0c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 11:51:07 +0100 Subject: [PATCH 07/16] Refactor tests to new API --- tools/@aws-cdk/prlint/index.ts | 2 + tools/@aws-cdk/prlint/lint.ts | 13 +- tools/@aws-cdk/prlint/linter-base.ts | 24 +++- tools/@aws-cdk/prlint/test/lint.test.ts | 173 ++++++++++++------------ 4 files changed, 118 insertions(+), 94 deletions(-) diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index ca52ae92f7c92..f3a3e112e3eeb 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -73,6 +73,8 @@ async function run() { // Actually running in GitHub actions, do it (otherwise we're just testing) await prLinter.executeActions(actions); } + + await prLinter.actionsToException(actions); } } catch (error: any) { diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index ad682fb9a04c2..2134ac3c01044 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -288,7 +288,7 @@ export class PullRequestLinter extends PullRequestLinterBase { console.log("Deleting PR Linter Comment now"); ret.deleteSingletonPrLinterComment = true; - ret = mergeLinterActions(ret, await this.communicateResult(validationCollector)); + ret = mergeLinterActions(ret, await this.validationToActions(validationCollector)); // also assess whether the PR needs review or not try { @@ -309,15 +309,22 @@ export class PullRequestLinter extends PullRequestLinterBase { * Creates a new review, requesting changes, with the reasons that the linter did not pass. * @param result The result of the PR Linter run. */ - private async communicateResult(result: ValidationCollector): Promise { + private async validationToActions(result: ValidationCollector): Promise { if (result.isValid()) { console.log('✅ Success'); return { dismissPreviousReview: true, }; } else { + // Not the best place to put this, but this is ~where it was before the refactor. + const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); + const exemptionRequest = comments.some(comment => comment.body?.toLowerCase().includes("exemption request")); + return { - requestChanges: { failures: result.errors }, + requestChanges: { + failures: result.errors, + exemptionRequest, + }, }; } } diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index d2b31cf581056..f70a8a565017a 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -111,11 +111,25 @@ export class PullRequestLinterBase { this.dismissPRLinterReview(existingReview); } if (actions.requestChanges) { - this.createOrUpdatePRLinterReview(actions.requestChanges.failures, existingReview); + this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReview); } } } + /** + * For code that requires an exception + */ + public actionsToException(actions: LinterActions) { + if (actions.requestChanges) { + // The tests check for exactly these messages + const messages = [...actions.requestChanges.failures]; + if (actions.requestChanges.exemptionRequest) { + messages.push('A exemption request has been requested. Please wait for a maintainer\'s review.'); + } + throw new LinterError(messages.join('\n')); + } + } + /** * Deletes the previous linter comment if it exists. */ @@ -168,7 +182,7 @@ export class PullRequestLinterBase { * @param failureMessages The failures received by the pr linter validation checks. * @param existingReview The review created by a previous run of the linter. */ - private async createOrUpdatePRLinterReview(failureMessages: string[], existingReview?: Review): Promise { + private async createOrUpdatePRLinterReview(failureMessages: string[], exemptionRequest?: boolean, existingReview?: Review): Promise { // FIXME: this function is doing too much at once. We should split this out into separate // actions on `LinterActions`. @@ -187,8 +201,7 @@ export class PullRequestLinterBase { }); } - const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - if (comments.find(comment => comment.body?.toLowerCase().includes("exemption request"))) { + if (exemptionRequest) { body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; } await this.client.issues.createComment({ @@ -216,8 +229,6 @@ export class PullRequestLinterBase { state: 'closed', }); } - - throw new LinterError(body); } private formatErrors(errors: string[]) { @@ -282,6 +293,7 @@ export interface LinterActions { */ requestChanges?: { failures: string[]; + exemptionRequest?: boolean; }; /** diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 17c5b824216b9..0aa2081f30813 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -2,6 +2,7 @@ import * as path from 'path'; import { GitHubFile, GitHubLabel, GitHubPr } from '../github'; import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX } from '../constants'; import { PullRequestLinter } from '../lint'; +import { StatusEvent } from '@octokit/webhooks-definitions/schema'; let mockRemoveLabel = jest.fn(); let mockAddLabel = jest.fn(); @@ -50,7 +51,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/); }); test('the first breaking change should immediately follow "BREAKING CHANGE:"', async () => { @@ -65,7 +66,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/description of the first breaking change should immediately follow/); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/description of the first breaking change should immediately follow/); }); test('invalid title', async () => { @@ -79,7 +80,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/The title of this pull request must specify the module name that the first breaking change should be associated to./); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The title of this pull request must specify the module name that the first breaking change should be associated to./); }); test('valid title', async () => { @@ -93,7 +94,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; // not throw + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; // not throw }); }); @@ -112,7 +113,7 @@ test('disallow PRs from main branch of fork', async () => { } }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork./); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork./); }); describe('commit message format', () => { @@ -127,7 +128,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('invalid scope with aws- prefix', async () => { @@ -141,7 +142,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'./); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'./); }); test('valid scope with aws- in summary and body', async () => { @@ -155,7 +156,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('valid with missing scope', async () => { @@ -169,7 +170,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('valid with aws-cdk-lib as a scope', async () => { @@ -183,7 +184,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test.each(['core', 'prlint', 'awslint'])('valid scope for packages that dont use aws- prefix', async (scope) => { @@ -197,7 +198,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('invalid capitalized title', async () => { @@ -211,7 +212,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``"/); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``"/); }); test('valid capitalized title with backticks', async () => { @@ -225,7 +226,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); }); @@ -241,7 +242,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); test('breaking changes multiple in stable modules', async () => { @@ -259,7 +260,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow('Breaking changes in stable modules [lambda, ecs] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [lambda, ecs] is disallowed.'); }); test('unless exempt-breaking-change label added', async () => { @@ -276,7 +277,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; // not throw + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; // not throw }); test('with additional "closes" footer', async () => { @@ -296,7 +297,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); }); @@ -327,7 +328,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('integ files not changed in feat', async () => { @@ -356,12 +357,8 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Features must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -391,12 +388,8 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Features must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -426,12 +419,8 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -461,12 +450,8 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -493,7 +478,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; }); test('integ files not changed, not a feature', async () => { @@ -519,7 +504,7 @@ describe('integration tests required on features', () => { }, ]; const prlinter = configureMock(issue, files); - expect(await prlinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prlinter, SHA)).resolves; }); describe('CLI file changed', () => { @@ -540,14 +525,14 @@ describe('integration tests required on features', () => { test('no label throws error', async () => { const prLinter = configureMock(issue, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(/CLI code has changed. A maintainer must/); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/CLI code has changed. A maintainer must/); }); test('with label no error', async () => { labels.push({ name: 'pr-linter/cli-integ-tested' }); const prLinter = configureMock(issue, files); // THEN: no exception - expect(async () => await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(async () => await legacyValidatePullRequestTarget(prLinter, SHA)).resolves; }); test('with aws-cdk-automation author', async () => { @@ -559,7 +544,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(issue, files); - await prLinter.validatePullRequestTarget(SHA); +legacyValidatePullRequestTarget( await prLinter, SHA); // THEN: no exception }); }); @@ -582,7 +567,7 @@ describe('integration tests required on features', () => { test('needs a review', async () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -602,7 +587,7 @@ describe('integration tests required on features', () => { // WHEN pr.labels = [{ name: 'p1' }]; const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -633,7 +618,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -664,7 +649,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -701,7 +686,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -734,7 +719,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -767,7 +752,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -807,7 +792,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -845,7 +830,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -875,7 +860,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -903,7 +888,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -932,7 +917,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -962,7 +947,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -995,7 +980,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent({ + await legacyValidateStatusEvent(prLinter, { sha: SHA, context: CODE_BUILD_CONTEXT, state: 'success', @@ -1028,7 +1013,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -1053,13 +1038,8 @@ describe('integration tests required on features', () => { }, }; const prLinter = configureMock(issue, undefined, ['Exemption Request']); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' + - '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -1074,13 +1054,9 @@ describe('integration tests required on features', () => { }, }; const prLinter = configureMock(issue, undefined, ['Exemption Request: \nThe reason is blah blah blah.']); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' + - '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + + 'A exemption request has been requested. Please wait for a maintainer\'s review.', ); }); @@ -1095,13 +1071,9 @@ describe('integration tests required on features', () => { }, }; const prLinter = configureMock(issue, undefined, ['Random content - Exemption Request - hello world']); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow( - 'The pull request linter fails with the following errors:' + - '\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' + - '\n\nPRs must pass status checks before we can provide a meaningful review.\n\n' + - 'If you would like to request an exemption from the status checks or clarification on feedback,' + - ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' + - '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.', + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + + 'A exemption request has been requested. Please wait for a maintainer\'s review.', ); }); }); @@ -1122,7 +1094,7 @@ describe('integration tests required on features', () => { }; const prLinter = configureMock(pr, files); - await expect(prLinter.validatePullRequestTarget(SHA)).resolves; + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).resolves; }); test('with another author', async () => { @@ -1136,7 +1108,7 @@ describe('integration tests required on features', () => { }; const prLinter = configureMock(pr, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(); + await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(); }); }); }); @@ -1223,3 +1195,34 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom } as any, }); } + +/** + * Interface-compatible implementation of validatePullRequestTarget before the refactor + * + * Previously, one method did 3 things: + * + * - Evaluate rules + * - Apply changes to the PR + * - Throw an exception + * + * We pulled those things apart, but many tests are still expecting all things to happen together + * so it's easier to bundle them back up into a legacy compat functin. + * + * This is just so we can mass-replace code in the tests and move on with our + * lives. It's not recommended to write new code using this! + * + * @deprecated Assert on the contents of `LinterActions` instead. + */ +async function legacyValidatePullRequestTarget(prLinter: PullRequestLinter, sha: string) { + const actions = await prLinter.validatePullRequestTarget(sha); + await prLinter.executeActions(actions); + prLinter.actionsToException(actions); +} + +/** + * Same as for validatePullRequesTarget + */ +async function legacyValidateStatusEvent(prLinter: PullRequestLinter, statusEvent: StatusEvent) { + const actions = await prLinter.validateStatusEvent(statusEvent); + await prLinter.executeActions(actions); +} From aa423426fd7efa83a40d42fb81bd0a086484ce98 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 12:20:18 +0100 Subject: [PATCH 08/16] Remove SHA --- tools/@aws-cdk/prlint/index.ts | 77 ++++++++++++++++--------- tools/@aws-cdk/prlint/lint.ts | 3 +- tools/@aws-cdk/prlint/test/lint.test.ts | 74 ++++++++++++------------ 3 files changed, 89 insertions(+), 65 deletions(-) diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index f3a3e112e3eeb..cd0f698ac3fd1 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -5,6 +5,17 @@ import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/sch import { PullRequestLinter } from './lint'; import { LinterActions } from './linter-base'; +/** + * Entry point for PR linter + * + * This gets run as a GitHub action. + * + * To test locally, do the following: + * + * ``` + * env GITHUB_TOKEN=$(cat ~/.my-github-token) GITHUB_REPOSITORY=aws/aws-cdk PR_NUMBER=1234 npx ts-node ./index + * ``` + */ async function run() { const token: string = process.env.GITHUB_TOKEN!; const client = new Octokit({ auth: token }); @@ -12,34 +23,13 @@ async function run() { const owner = github.context.repo.owner; const repo = github.context.repo.repo; - // Try to get a SHA from various places - let sha = process.env.PR_SHA; - if (!sha && github.context.eventName.startsWith('pull_request')) { - sha = (github.context.payload as PullRequestEvent)?.pull_request?.head?.sha; - } - if (!sha && github.context.eventName === 'status') { - sha = (github.context.payload as StatusEvent)?.sha; - } - if (!sha) { - throw new Error(`Could not determine a SHA from either \$PR_SHA or ${JSON.stringify(github.context.payload)}`); - } - - let number = process.env.PR_NUMBER ? Number(process.env.PR_NUMBER) : undefined; - if (number === undefined) { - const pr = await PullRequestLinter.getPRFromCommit(client, owner, repo, sha); - if (!pr && github.context.eventName === 'status') { - // For a "status" event, apparently it's fine to not be able to find the PR - console.error('Could not find appropriate PR for status event; exiting'); + const number = await determinePrNumber(client); + if (!number) { + if (github.context.eventName === 'status') { + console.error(`Could not find PR belonging to status event, but that's not unusual. Skipping.`); process.exit(0); } - - if (pr) { - number = pr.number; - } - } - - if (number === undefined) { - throw new Error(`Could not determine PR number from either \$PR_NUMBER or the SHA: ${sha}`); + throw new Error(`Could not find PR number from context.`); } try { @@ -54,13 +44,17 @@ async function run() { switch (github.context.eventName) { case 'status': + // Triggering on a 'status' event is solely used to see if the CodeBuild + // job just turned green, and adding certain 'ready for review' labels + // if it does. const statusPayload = github.context.payload as StatusEvent; console.log('validating status event'); actions = await prLinter.validateStatusEvent(statusPayload); break; default: - actions = await prLinter.validatePullRequestTarget(sha); + // This is the main PR validator action. + actions = await prLinter.validatePullRequestTarget(); break; } @@ -83,4 +77,31 @@ async function run() { } } -void run(); +async function determinePrNumber(client: Octokit): Promise { + const owner = github.context.repo.owner; + const repo = github.context.repo.repo; + + if (process.env.PR_NUMBER) { + return Number(process.env.PR_NUMBER); + } + if (github.context.eventName.startsWith('pull_request')) { + return (github.context.payload as PullRequestEvent).pull_request.number; + } + + // If we don't have PR_NUMBER, try to find a SHA and convert that into a PR number + let sha = process.env.PR_SHA; + if (!sha && github.context.eventName === 'status') { + sha = (github.context.payload as StatusEvent)?.sha; + } + if (!sha) { + throw new Error(`Could not determine a SHA from either \$PR_SHA or ${JSON.stringify(github.context.payload)}`); + } + + const pr = await PullRequestLinter.getPRFromCommit(client, owner, repo, sha); + return pr?.number; +} + +run().catch(e => { + console.error(e); + process.exitCode = 1; +}); diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 2134ac3c01044..fd5cfb5ed5ebc 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -203,10 +203,11 @@ export class PullRequestLinter extends PullRequestLinterBase { * Performs validations and communicates results via pull request comments, upon failure. * This also dismisses previous reviews so they do not remain in REQUEST_CHANGES upon fix of failures. */ - public async validatePullRequestTarget(sha: string): Promise { + public async validatePullRequestTarget(): Promise { let ret: LinterActions = {}; const number = this.props.number; + const sha = (await this.pr()).head.sha; console.log(`⌛ Fetching PR number ${number}`); const pr = await this.pr(); diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 0aa2081f30813..0abf28314b980 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -51,7 +51,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/); }); test('the first breaking change should immediately follow "BREAKING CHANGE:"', async () => { @@ -66,7 +66,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/description of the first breaking change should immediately follow/); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/description of the first breaking change should immediately follow/); }); test('invalid title', async () => { @@ -80,7 +80,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The title of this pull request must specify the module name that the first breaking change should be associated to./); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/The title of this pull request must specify the module name that the first breaking change should be associated to./); }); test('valid title', async () => { @@ -94,7 +94,7 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; // not throw + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; // not throw }); }); @@ -113,7 +113,7 @@ test('disallow PRs from main branch of fork', async () => { } }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork./); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork./); }); describe('commit message format', () => { @@ -128,7 +128,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('invalid scope with aws- prefix', async () => { @@ -142,7 +142,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'./); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'./); }); test('valid scope with aws- in summary and body', async () => { @@ -156,7 +156,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('valid with missing scope', async () => { @@ -170,7 +170,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('valid with aws-cdk-lib as a scope', async () => { @@ -184,7 +184,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test.each(['core', 'prlint', 'awslint'])('valid scope for packages that dont use aws- prefix', async (scope) => { @@ -198,7 +198,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('invalid capitalized title', async () => { @@ -212,7 +212,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``"/); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``"/); }); test('valid capitalized title with backticks', async () => { @@ -226,7 +226,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); }); @@ -242,7 +242,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); test('breaking changes multiple in stable modules', async () => { @@ -260,7 +260,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [lambda, ecs] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow('Breaking changes in stable modules [lambda, ecs] is disallowed.'); }); test('unless exempt-breaking-change label added', async () => { @@ -277,7 +277,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; // not throw + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; // not throw }); test('with additional "closes" footer', async () => { @@ -297,7 +297,7 @@ describe('ban breaking changes in stable modules', () => { }, }; const prLinter = configureMock(issue, undefined); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); }); @@ -328,7 +328,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('integ files not changed in feat', async () => { @@ -357,7 +357,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -388,7 +388,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -419,7 +419,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -450,7 +450,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -478,7 +478,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(legacyValidatePullRequestTarget(await prLinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('integ files not changed, not a feature', async () => { @@ -504,7 +504,7 @@ describe('integration tests required on features', () => { }, ]; const prlinter = configureMock(issue, files); - expect(legacyValidatePullRequestTarget(await prlinter, SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prlinter)).resolves; }); describe('CLI file changed', () => { @@ -525,14 +525,14 @@ describe('integration tests required on features', () => { test('no label throws error', async () => { const prLinter = configureMock(issue, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(/CLI code has changed. A maintainer must/); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(/CLI code has changed. A maintainer must/); }); test('with label no error', async () => { labels.push({ name: 'pr-linter/cli-integ-tested' }); const prLinter = configureMock(issue, files); // THEN: no exception - expect(async () => await legacyValidatePullRequestTarget(prLinter, SHA)).resolves; + expect(async () => await legacyValidatePullRequestTarget(prLinter)).resolves; }); test('with aws-cdk-automation author', async () => { @@ -544,7 +544,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(issue, files); -legacyValidatePullRequestTarget( await prLinter, SHA); +legacyValidatePullRequestTarget( await prLinter); // THEN: no exception }); }); @@ -1013,7 +1013,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); // WHEN const prLinter = configureMock(pr); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -1038,7 +1038,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); }, }; const prLinter = configureMock(issue, undefined, ['Exemption Request']); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -1054,7 +1054,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); }, }; const prLinter = configureMock(issue, undefined, ['Exemption Request: \nThe reason is blah blah blah.']); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + 'A exemption request has been requested. Please wait for a maintainer\'s review.', ); @@ -1071,7 +1071,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); }, }; const prLinter = configureMock(issue, undefined, ['Random content - Exemption Request - hello world']); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow( + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + 'A exemption request has been requested. Please wait for a maintainer\'s review.', ); @@ -1094,7 +1094,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); }; const prLinter = configureMock(pr, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).resolves; + await expect(legacyValidatePullRequestTarget(prLinter)).resolves; }); test('with another author', async () => { @@ -1108,7 +1108,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); }; const prLinter = configureMock(pr, files); - await expect(legacyValidatePullRequestTarget(prLinter, SHA)).rejects.toThrow(); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(); }); }); }); @@ -1116,7 +1116,7 @@ legacyValidatePullRequestTarget( await prLinter, SHA); function configureMock(pr: Subset, prFiles?: GitHubFile[], existingComments?: string[]): PullRequestLinter { const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) { - return { data: { ...pr, base: { ref: 'main'}} }; + return { data: { ...pr, base: { ref: 'main'}, head: { sha: 'ABC', ...pr?.head }} }; }, listFiles(_props: { _owner: string, _repo: string, _pull_number: number }) { @@ -1213,14 +1213,16 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom * * @deprecated Assert on the contents of `LinterActions` instead. */ -async function legacyValidatePullRequestTarget(prLinter: PullRequestLinter, sha: string) { - const actions = await prLinter.validatePullRequestTarget(sha); +async function legacyValidatePullRequestTarget(prLinter: PullRequestLinter) { + const actions = await prLinter.validatePullRequestTarget(); await prLinter.executeActions(actions); prLinter.actionsToException(actions); } /** * Same as for validatePullRequesTarget + * + * @deprecated Assert on the contents of `LinterActions` instead. */ async function legacyValidateStatusEvent(prLinter: PullRequestLinter, statusEvent: StatusEvent) { const actions = await prLinter.validateStatusEvent(statusEvent); From 3f56cde78875e744716a736bcc092cee73c93d6c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 12:54:51 +0100 Subject: [PATCH 09/16] Remove codecov stuff --- tools/@aws-cdk/prlint/constants.ts | 11 ------ tools/@aws-cdk/prlint/lint.ts | 52 ++----------------------- tools/@aws-cdk/prlint/linter-base.ts | 11 +----- tools/@aws-cdk/prlint/test/lint.test.ts | 17 +------- 4 files changed, 6 insertions(+), 85 deletions(-) diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts index 227f75ece94dd..e68a3e9b5d616 100644 --- a/tools/@aws-cdk/prlint/constants.ts +++ b/tools/@aws-cdk/prlint/constants.ts @@ -14,14 +14,3 @@ export enum Exemption { REQUEST_EXEMPTION = 'pr-linter/exemption-requested', CODECOV = "pr-linter/exempt-codecov", } - -export const CODECOV_PREFIX = 'codecov/'; - -export const CODECOV_CHECKS = [ - 'patch', - 'patch/packages/aws-cdk', - 'patch/packages/aws-cdk-lib/core', - 'project', - 'project/packages/aws-cdk', - 'project/packages/aws-cdk-lib/core' -]; diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index fd5cfb5ed5ebc..f777cf047be32 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -2,9 +2,9 @@ import { execSync } from 'child_process'; import * as path from 'path'; import { findModulePath, moduleStability } from './module'; import { breakingModules } from './parser'; -import { CheckRunConclusion, GitHubFile, GitHubPr, Review } from "./github"; -import { Test, TestResult, ValidationCollector } from './results'; -import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX, Exemption } from './constants'; +import { GitHubFile, GitHubPr, Review } from "./github"; +import { TestResult, ValidationCollector } from './results'; +import { CODE_BUILD_CONTEXT, Exemption } from './constants'; import { StatusEvent } from '@octokit/webhooks-definitions/schema'; import { LinterActions, mergeLinterActions, PR_FROM_MAIN_ERROR, PullRequestLinterBase } from './linter-base'; @@ -38,23 +38,6 @@ export class PullRequestLinter extends PullRequestLinterBase { return {}; } - private async checkRunConclusion(sha: string, checkName: string): Promise { - const response = await this.client.paginate(this.client.checks.listForRef, { - owner: this.prParams.owner, - repo: this.prParams.repo, - ref: sha, - }); - - // grab the last check run that was completed - const conclusion = response - .filter(c => c.name === checkName && c.completed_at != null) - .sort((c1, c2) => c2.completed_at!.localeCompare(c1.completed_at!)) - .map(s => s.conclusion)[0]; - - console.log(`${checkName} conclusion: ${conclusion}`) - return conclusion; - } - /** * Assess whether or not a PR is ready for review. * This is needed because some things that we need to evaluate are not filterable on @@ -264,31 +247,6 @@ export class PullRequestLinter extends PullRequestLinterBase { ], }); - if (pr.base.ref === 'main') { - // we don't enforce codecov on release branches - const codecovTests: Test[] = []; - for (const c of CODECOV_CHECKS) { - const checkName = `${CODECOV_PREFIX}${c}`; - const conclusion = await this.checkRunConclusion(sha, checkName); - codecovTests.push({ - test: () => { - const result = new TestResult(); - const message = conclusion == null ? `${checkName} has not reported a status yet` : `${checkName} job is in status: ${conclusion}`; - result.assessFailure(conclusion !== 'success', message); - return result; - } - }) - } - - validationCollector.validateRuleSet({ - exemption: shouldExemptCodecov, - testRuleSet: codecovTests, - }); - } - - console.log("Deleting PR Linter Comment now"); - ret.deleteSingletonPrLinterComment = true; - ret = mergeLinterActions(ret, await this.validationToActions(validationCollector)); // also assess whether the PR needs review or not @@ -387,10 +345,6 @@ function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { return result; } -function shouldExemptCodecov(pr: GitHubPr): boolean { - return hasLabel(pr, Exemption.CODECOV); -} - function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); } diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index f70a8a565017a..d1586aef3af88 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -96,16 +96,13 @@ export class PullRequestLinterBase { this.addLabel(label, pr); } - if (actions.deleteSingletonPrLinterComment) { - this.deletePRLinterComment(); - } - if (actions.dismissPreviousReview || actions.requestChanges) { if (actions.dismissPreviousReview && actions.requestChanges) { throw new Error(`It does not make sense to supply both dismissPreviousReview and requestChanges: ${JSON.stringify(actions)}`); } const existingReview = await this.findExistingPRLinterReview(); + this.deletePRLinterComment(); if (actions.dismissPreviousReview) { this.dismissPRLinterReview(existingReview); @@ -283,11 +280,6 @@ export interface LinterActions { */ removeLabels?: string[]; - /** - * Delete the singleton PR linter comment - */ - deleteSingletonPrLinterComment?: boolean; - /** * Post a "request changes" review */ @@ -306,7 +298,6 @@ export function mergeLinterActions(a: LinterActions, b: LinterActions): LinterAc return { addLabels: nonEmpty([...(a.addLabels ?? []), ...(b.addLabels ?? [])]), removeLabels: nonEmpty([...(a.removeLabels ?? []), ...(b.removeLabels ?? [])]), - deleteSingletonPrLinterComment: b.deleteSingletonPrLinterComment ?? a.deleteSingletonPrLinterComment, requestChanges: b.requestChanges ?? a.requestChanges, dismissPreviousReview: b.dismissPreviousReview ?? a.dismissPreviousReview, }; diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 0abf28314b980..ea98fd01a0448 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1,6 +1,6 @@ import * as path from 'path'; import { GitHubFile, GitHubLabel, GitHubPr } from '../github'; -import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, CODECOV_PREFIX } from '../constants'; +import { CODE_BUILD_CONTEXT } from '../constants'; import { PullRequestLinter } from '../lint'; import { StatusEvent } from '@octokit/webhooks-definitions/schema'; @@ -1116,7 +1116,7 @@ legacyValidatePullRequestTarget( await prLinter); function configureMock(pr: Subset, prFiles?: GitHubFile[], existingComments?: string[]): PullRequestLinter { const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) { - return { data: { ...pr, base: { ref: 'main'}, head: { sha: 'ABC', ...pr?.head }} }; + return { data: { ...pr, base: { ref: 'main', ...pr?.base }, head: { sha: 'ABC', ...pr?.head }} }; }, listFiles(_props: { _owner: string, _repo: string, _pull_number: number }) { @@ -1164,18 +1164,6 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom }, }; - const checksClient = { - listForRef() { - return { - data: CODECOV_CHECKS.map(c => ({ - name: `${CODECOV_PREFIX}${c}`, - conclusion: 'success', - completed_at: '1' - })), - } - } - } - const searchClient = { issuesAndPullRequests() {}, }; @@ -1190,7 +1178,6 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom issues: issuesClient as any, search: searchClient as any, repos: reposClient as any, - checks: checksClient as any, paginate: (method: any, args: any) => { return method(args).data; }, } as any, }); From f274ee0960e72ff21c061cb8e677691e05920f42 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 13:16:57 +0100 Subject: [PATCH 10/16] Don't request exception if requested by someone else --- tools/@aws-cdk/prlint/lint.ts | 4 +- tools/@aws-cdk/prlint/test/lint.test.ts | 85 +++++++++++++++---------- 2 files changed, 56 insertions(+), 33 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index f777cf047be32..30e6c3f2b5708 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -276,8 +276,10 @@ export class PullRequestLinter extends PullRequestLinterBase { }; } else { // Not the best place to put this, but this is ~where it was before the refactor. + const prAuthor = (await this.pr()).user?.login; + const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - const exemptionRequest = comments.some(comment => comment.body?.toLowerCase().includes("exemption request")); + const exemptionRequest = comments.some(comment => comment.user?.login === prAuthor && comment.body?.toLowerCase().includes("exemption request")); return { requestChanges: { diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index ea98fd01a0448..d4dde4f1fac09 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1027,33 +1027,33 @@ legacyValidatePullRequestTarget( await prLinter); }); describe('with existing Exemption Request comment', () => { + const issue: Subset = { + number: 1, + title: 'fix: some title', + body: '', + labels: [{ name: 'pr-linter/exempt-test' }], + user: { + login: 'author', + }, + }; + test('valid exemption request comment', async () => { - const issue: Subset = { - number: 1, - title: 'fix: some title', - body: '', - labels: [{ name: 'pr-linter/exempt-test' }], - user: { - login: 'author', - }, - }; - const prLinter = configureMock(issue, undefined, ['Exemption Request']); + const comments = [ + { login: 'author', body: 'Exemption Request' } + ]; + + const prLinter = configureMock(issue, undefined, comments); await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); test('valid exemption request with additional context', async () => { - const issue: Subset = { - number: 1, - title: 'fix: some title', - body: '', - labels: [{ name: 'pr-linter/exempt-test' }], - user: { - login: 'author', - }, - }; - const prLinter = configureMock(issue, undefined, ['Exemption Request: \nThe reason is blah blah blah.']); + const comments = [ + { login: 'author', body: 'Exemption Request: \nThe reason is blah blah blah.' } + ]; + + const prLinter = configureMock(issue, undefined, comments); await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + 'A exemption request has been requested. Please wait for a maintainer\'s review.', @@ -1061,21 +1061,42 @@ legacyValidatePullRequestTarget( await prLinter); }); test('valid exemption request with middle exemption request', async () => { - const issue: Subset = { - number: 1, - title: 'fix: some title', - body: '', - labels: [{ name: 'pr-linter/exempt-test' }], - user: { - login: 'author', - }, - }; - const prLinter = configureMock(issue, undefined, ['Random content - Exemption Request - hello world']); + const comments = [ + { login: 'author', body: 'Random content - Exemption Request - hello world' } + ]; + + const prLinter = configureMock(issue, undefined, comments); await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow( 'Fixes must contain a change to an integration test file and the resulting snapshot.\n' + 'A exemption request has been requested. Please wait for a maintainer\'s review.', ); }); + + test('exemption only counts if requested by PR author', async () => { + const comments = [ + { login: 'bert', body: 'Random content - Exemption Request - hello world' } + ]; + + const prLinter = configureMock(issue, undefined, comments); + await expect(prLinter.validatePullRequestTarget()).resolves.toEqual(expect.objectContaining({ + requestChanges: expect.objectContaining({ + exemptionRequest: false, + }), + })); + }); + + test('bot does not trigger on its own exemption requests', async () => { + const comments = [ + { login: 'aws-cdk-automation', body: 'Random content - Exemption Request - hello world' } + ]; + + const prLinter = configureMock(issue, undefined, comments); + await expect(prLinter.validatePullRequestTarget()).resolves.toEqual(expect.objectContaining({ + requestChanges: expect.objectContaining({ + exemptionRequest: false, + }), + })); + }); }); describe('metadata file changed', () => { @@ -1113,7 +1134,7 @@ legacyValidatePullRequestTarget( await prLinter); }); }); -function configureMock(pr: Subset, prFiles?: GitHubFile[], existingComments?: string[]): PullRequestLinter { +function configureMock(pr: Subset, prFiles?: GitHubFile[], existingComments?: Array<{ login: string, body: string }>): PullRequestLinter { const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) { return { data: { ...pr, base: { ref: 'main', ...pr?.base }, head: { sha: 'ABC', ...pr?.head }} }; @@ -1144,7 +1165,7 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom listComments() { const data = [{ id: 1212121212, user: { login: 'aws-cdk-automation' }, body: 'The pull request linter fails with the following errors:' }]; if (existingComments) { - existingComments.forEach(comment => data.push({ id: 1212121211, user: { login: 'aws-cdk-automation' }, body: comment })); + existingComments.forEach(comment => data.push({ id: 1212121211, user: { login: comment.login }, body: comment.body })); } return { data }; }, From 0716640131cc936b53d6107af78ed61e9520cc79 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 14:42:01 +0100 Subject: [PATCH 11/16] Give the linter a notation of its own username --- .github/workflows/pr-linter.yml | 1 + tools/@aws-cdk/prlint/constants.ts | 2 ++ tools/@aws-cdk/prlint/index.ts | 5 ++++- tools/@aws-cdk/prlint/linter-base.ts | 11 +++++++++-- tools/@aws-cdk/prlint/test/lint.test.ts | 1 + 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pr-linter.yml b/.github/workflows/pr-linter.yml index 6388fee91fe2e..e45b6c7f95ece 100644 --- a/.github/workflows/pr-linter.yml +++ b/.github/workflows/pr-linter.yml @@ -72,4 +72,5 @@ jobs: GITHUB_TOKEN: ${{ secrets.PROJEN_GITHUB_TOKEN }} PR_NUMBER: ${{ needs.download-if-workflow-run.outputs.pr_number }} PR_SHA: ${{ needs.download-if-workflow-run.outputs.pr_sha }} + LINTER_LOGIN: ${{ vars.LINTER_LOGIN }} REPO_ROOT: ${{ github.workspace }} diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts index e68a3e9b5d616..7e6f18db49989 100644 --- a/tools/@aws-cdk/prlint/constants.ts +++ b/tools/@aws-cdk/prlint/constants.ts @@ -1,4 +1,6 @@ +export const LINTER_LOGIN = 'aws-cdk-automation'; + export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; /** diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index cd0f698ac3fd1..57b4625ce7091 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -4,6 +4,7 @@ import { Octokit } from '@octokit/rest'; import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema'; import { PullRequestLinter } from './lint'; import { LinterActions } from './linter-base'; +import { LINTER_LOGIN } from './constants'; /** * Entry point for PR linter @@ -13,7 +14,7 @@ import { LinterActions } from './linter-base'; * To test locally, do the following: * * ``` - * env GITHUB_TOKEN=$(cat ~/.my-github-token) GITHUB_REPOSITORY=aws/aws-cdk PR_NUMBER=1234 npx ts-node ./index + * env GITHUB_TOKEN=$(cat ~/.my-github-token) LINTER_LOGIN=my-gh-alias GITHUB_REPOSITORY=aws/aws-cdk PR_NUMBER=1234 npx ts-node ./index * ``` */ async function run() { @@ -38,6 +39,8 @@ async function run() { owner, repo, number, + // On purpose || instead of ??, also collapse empty string + linterLogin: process.env.LINTER_LOGIN || LINTER_LOGIN, }); let actions: LinterActions | undefined; diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index d1586aef3af88..44e050c550df2 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -26,6 +26,11 @@ export interface PullRequestLinterBaseProps { * Pull request number. */ readonly number: number; + + /** + * For cases where the linter needs to know its own username + */ + readonly linterLogin: string; } @@ -65,6 +70,7 @@ export class PullRequestLinterBase { protected readonly client: Octokit; protected readonly prParams: { owner: string, repo: string, pull_number: number }; protected readonly issueParams: { owner: string, repo: string, issue_number: number }; + protected readonly linterLogin: string; private _pr: GitHubPr | undefined; @@ -72,6 +78,7 @@ export class PullRequestLinterBase { this.client = props.client; this.prParams = { owner: props.owner, repo: props.repo, pull_number: props.number }; this.issueParams = { owner: props.owner, repo: props.repo, issue_number: props.number }; + this.linterLogin = props.linterLogin; } public async pr(): Promise { @@ -161,7 +168,7 @@ export class PullRequestLinterBase { */ private async findExistingPRLinterReview(): Promise { const reviews = await this.client.paginate(this.client.pulls.listReviews, this.prParams); - return reviews.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review; + return reviews.find((review) => review.user?.login === this.linterLogin && review.state !== 'DISMISSED') as Review; } /** @@ -170,7 +177,7 @@ export class PullRequestLinterBase { */ private async findExistingPRLinterComment(): Promise { const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - return comments.find((comment) => comment.user?.login === 'aws-cdk-automation' && comment.body?.startsWith('The pull request linter fails with the following errors:')) as GitHubComment; + return comments.find((comment) => comment.user?.login === this.linterLogin && comment.body?.startsWith('The pull request linter fails with the following errors:')) as GitHubComment; } /** diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index d4dde4f1fac09..fd78397f88846 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1192,6 +1192,7 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom owner: 'aws', repo: 'aws-cdk', number: 1000, + linterLogin: 'aws-cdk-automation', // hax hax client: { From 860469f6a49aac948f484eef5184d2e47e577957 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 14:54:08 +0100 Subject: [PATCH 12/16] Do not keep on reviewing if the review is already up-to-date --- tools/@aws-cdk/prlint/linter-base.ts | 34 +++++++++++++++++----------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index 44e050c550df2..f2d9f11340b87 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -3,6 +3,8 @@ import { GitHubComment, GitHubPr, Review } from "./github"; export const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; +const LINTER_MARKER = '\n'; + /** * Props used to perform linting against the pull request. */ @@ -177,7 +179,7 @@ export class PullRequestLinterBase { */ private async findExistingPRLinterComment(): Promise { const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - return comments.find((comment) => comment.user?.login === this.linterLogin && comment.body?.startsWith('The pull request linter fails with the following errors:')) as GitHubComment; + return comments.find((comment) => comment.user?.login === this.linterLogin && comment.body?.startsWith(LINTER_MARKER)) as GitHubComment; } /** @@ -190,28 +192,36 @@ export class PullRequestLinterBase { // FIXME: this function is doing too much at once. We should split this out into separate // actions on `LinterActions`. - let body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` + let body = LINTER_MARKER + + `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` + 'PRs must pass status checks before we can provide a meaningful review.\n\n' + 'If you would like to request an exemption from the status checks or clarification on feedback,' + ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.'; - if (!existingReview) { + if (!existingReview || existingReview.state !== 'REQUEST_CHANGES') { await this.client.pulls.createReview({ ...this.prParams, - body: 'The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons.' + body: LINTER_MARKER + + 'The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons.' + ' If you believe this pull request should receive an exemption, please comment and provide a justification.' + '\n\n\nA comment requesting an exemption should contain the text `Exemption Request`.' + ' Additionally, if clarification is needed add `Clarification Request` to a comment.', event: 'REQUEST_CHANGES', }); - } - if (exemptionRequest) { - body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; + if (exemptionRequest) { + body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; + } + await this.client.issues.createComment({ + ...this.issueParams, + body, + }); + } else if (existingReview.body !== body) { + this.client.pulls.updateReview({ + ...this.prParams, + review_id: existingReview.id, + body, + }); } - await this.client.issues.createComment({ - ...this.issueParams, - body, - }); // Closing the PR if it is opened from main branch of author's fork if (failureMessages.includes(PR_FROM_MAIN_ERROR)) { @@ -242,7 +252,6 @@ export class PullRequestLinterBase { private addLabel(label: string, pr: Pick) { // already has label, so no-op if (pr.labels.some(l => l.name === label)) { return; } - console.log(`adding ${label} to pr ${pr.number}`); this.client.issues.addLabels({ issue_number: pr.number, owner: this.prParams.owner, @@ -256,7 +265,6 @@ export class PullRequestLinterBase { private removeLabel(label: string, pr: Pick) { // does not have label, so no-op if (!pr.labels.some(l => l.name === label)) { return; } - console.log(`removing ${label} to pr ${pr.number}`); this.client.issues.removeLabel({ issue_number: pr.number, owner: this.prParams.owner, From 8f56bf33f84e381549ca2b8971896c8f15932480 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 20:24:11 +0100 Subject: [PATCH 13/16] Just do reviews --- tools/@aws-cdk/prlint/github.ts | 2 +- tools/@aws-cdk/prlint/linter-base.ts | 119 +++++++++++++-------------- 2 files changed, 60 insertions(+), 61 deletions(-) diff --git a/tools/@aws-cdk/prlint/github.ts b/tools/@aws-cdk/prlint/github.ts index 30efa7135e812..5ba2820752f61 100644 --- a/tools/@aws-cdk/prlint/github.ts +++ b/tools/@aws-cdk/prlint/github.ts @@ -14,7 +14,7 @@ export interface Review { id: number; user: { login: string; - }; + } | null; body: string; state: string; } diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts index f2d9f11340b87..8e14dcb5355dc 100644 --- a/tools/@aws-cdk/prlint/linter-base.ts +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -1,10 +1,8 @@ import { Octokit } from '@octokit/rest'; -import { GitHubComment, GitHubPr, Review } from "./github"; +import { GitHubPr, Review } from "./github"; export const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.'; -const LINTER_MARKER = '\n'; - /** * Props used to perform linting against the pull request. */ @@ -110,14 +108,13 @@ export class PullRequestLinterBase { throw new Error(`It does not make sense to supply both dismissPreviousReview and requestChanges: ${JSON.stringify(actions)}`); } - const existingReview = await this.findExistingPRLinterReview(); - this.deletePRLinterComment(); + const existingReviews = await this.findExistingPRLinterReview(); if (actions.dismissPreviousReview) { - this.dismissPRLinterReview(existingReview); + this.dismissPRLinterReviews(existingReviews, 'passing'); } if (actions.requestChanges) { - this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReview); + this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReviews); } } } @@ -136,31 +133,33 @@ export class PullRequestLinterBase { } } - /** - * Deletes the previous linter comment if it exists. - */ - private async deletePRLinterComment(): Promise { - // Since previous versions of this pr linter didn't add comments, we need to do this check first. - const comment = await this.findExistingPRLinterComment(); - if (comment) { - await this.client.issues.deleteComment({ - ...this.issueParams, - comment_id: comment.id, - }); - }; - }; - /** * Dismisses previous reviews by aws-cdk-automation when the pull request succeeds the linter. * @param existingReview The review created by a previous run of the linter */ - private async dismissPRLinterReview(existingReview?: Review): Promise { - if (existingReview) { - await this.client.pulls.dismissReview({ - ...this.prParams, - review_id: existingReview.id, - message: '✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.', - }); + private async dismissPRLinterReviews(existingReviews: Review[], reason: 'passing' | 'stale'): Promise { + let message: string; + switch (reason) { + case 'passing': + message = '✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.'; + break; + case 'stale': + message = 'Dismissing outdated PRLinter review.'; + break; + } + + for (const existingReview of existingReviews ?? []) { + try { + console.log('Dismissing review'); + await this.client.pulls.dismissReview({ + ...this.prParams, + review_id: existingReview.id, + message, + }); + } catch (e: any) { + // This can fail with a "not found" for some reason + throw new Error(`Dismissing review failed, user is probably not authorized: ${JSON.stringify(e, undefined, 2)}`); + } } } @@ -168,54 +167,53 @@ export class PullRequestLinterBase { * Finds existing review, if present * @returns Existing review, if present */ - private async findExistingPRLinterReview(): Promise { + private async findExistingPRLinterReview(): Promise { const reviews = await this.client.paginate(this.client.pulls.listReviews, this.prParams); - return reviews.find((review) => review.user?.login === this.linterLogin && review.state !== 'DISMISSED') as Review; - } - - /** - * Finds existing comment from previous review, if present - * @returns Existing comment, if present - */ - private async findExistingPRLinterComment(): Promise { - const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); - return comments.find((comment) => comment.user?.login === this.linterLogin && comment.body?.startsWith(LINTER_MARKER)) as GitHubComment; + return reviews.filter((review) => review.user?.login === this.linterLogin && review.state !== 'DISMISSED'); } /** * Creates a new review and comment for first run with failure or creates a new comment with new failures for existing reviews. * + * We assume the PR linter only ever creates "Changes Requested" reviews, or dismisses + * their own "Changes Requested" reviews. + * * @param failureMessages The failures received by the pr linter validation checks. * @param existingReview The review created by a previous run of the linter. */ - private async createOrUpdatePRLinterReview(failureMessages: string[], exemptionRequest?: boolean, existingReview?: Review): Promise { + private async createOrUpdatePRLinterReview(failureMessages: string[], exemptionRequest?: boolean, existingReviews?: Review[]): Promise { // FIXME: this function is doing too much at once. We should split this out into separate // actions on `LinterActions`. - let body = LINTER_MARKER - + `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}` - + 'PRs must pass status checks before we can provide a meaningful review.\n\n' - + 'If you would like to request an exemption from the status checks or clarification on feedback,' - + ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.'; - if (!existingReview || existingReview.state !== 'REQUEST_CHANGES') { + const paras = [ + 'The pull request linter fails with the following errors:', + this.formatErrors(failureMessages), + 'If you believe this pull request should receive an exemption, please comment and provide a justification. ' + + 'A comment requesting an exemption should contain the text `Exemption Request`. ' + + 'Additionally, if clarification is needed, add `Clarification Request` to a comment.', + ]; + + if (exemptionRequest) { + paras.push('✅ A exemption request has been requested. Please wait for a maintainer\'s review.'); + } + const body = paras.join('\n\n'); + + // Dismiss every review except the last one + this.dismissPRLinterReviews((existingReviews ?? []).slice(0, -1), 'stale'); + + // Update the last review + const existingReview = (existingReviews ?? []).slice(-1)[0]; + + if (!existingReview) { + console.log('Creating review'); await this.client.pulls.createReview({ ...this.prParams, - body: LINTER_MARKER - + 'The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons.' - + ' If you believe this pull request should receive an exemption, please comment and provide a justification.' - + '\n\n\nA comment requesting an exemption should contain the text `Exemption Request`.' - + ' Additionally, if clarification is needed add `Clarification Request` to a comment.', event: 'REQUEST_CHANGES', - }); - - if (exemptionRequest) { - body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.'; - } - await this.client.issues.createComment({ - ...this.issueParams, body, }); - } else if (existingReview.body !== body) { + } else if (existingReview.body !== body && existingReview.state === 'CHANGES_REQUESTED') { + // State is good but body is wrong + console.log('Updating review'); this.client.pulls.updateReview({ ...this.prParams, review_id: existingReview.id, @@ -232,6 +230,7 @@ export class PullRequestLinterBase { + '(this setting is enabled by default for personal accounts, and cannot be enabled for organization owned accounts). ' + 'The reason for this is that our automation needs to synchronize your branch with our main after it has been approved, ' + 'and we cannot do that if we cannot push to your branch.' + console.log('Closing pull request'); await this.client.issues.createComment({ ...this.issueParams, @@ -246,7 +245,7 @@ export class PullRequestLinterBase { } private formatErrors(errors: string[]) { - return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; + return errors.map(e => ` ❌ ${e}`).join('\n'); } private addLabel(label: string, pr: Pick) { From b7fa33ce4a1f1e1295f8da46ab5aef04ba314597 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 20:25:51 +0100 Subject: [PATCH 14/16] Fix tests --- tools/@aws-cdk/prlint/test/lint.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index fd78397f88846..40a43a159033a 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1154,6 +1154,8 @@ function configureMock(pr: Subset, prFiles?: GitHubFile[], existingCom dismissReview() {}, + updateReview() { }, + update() {}, }; From 2c0727aab3845eb470b272a37e54a4f5a9ddf198 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 20:29:04 +0100 Subject: [PATCH 15/16] Review comments --- tools/@aws-cdk/prlint/constants.ts | 2 +- tools/@aws-cdk/prlint/github.ts | 3 --- tools/@aws-cdk/prlint/index.ts | 6 +++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts index 7e6f18db49989..ab88a896ca352 100644 --- a/tools/@aws-cdk/prlint/constants.ts +++ b/tools/@aws-cdk/prlint/constants.ts @@ -1,5 +1,5 @@ -export const LINTER_LOGIN = 'aws-cdk-automation'; +export const DEFEAULT_LINTER_LOGIN = 'aws-cdk-automation'; export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)'; diff --git a/tools/@aws-cdk/prlint/github.ts b/tools/@aws-cdk/prlint/github.ts index 5ba2820752f61..17ddc07858147 100644 --- a/tools/@aws-cdk/prlint/github.ts +++ b/tools/@aws-cdk/prlint/github.ts @@ -1,6 +1,5 @@ import { Endpoints } from '@octokit/types'; import { StatusEvent } from '@octokit/webhooks-definitions/schema'; -import type { components } from '@octokit/openapi-types'; export type GitHubPr = Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}']['response']['data']; @@ -32,5 +31,3 @@ export interface GitHubLabel { export interface GitHubFile { readonly filename: string; } - -export type CheckRunConclusion = components['schemas']['check-run']['conclusion'] diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 57b4625ce7091..d430fd745f34f 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -4,7 +4,7 @@ import { Octokit } from '@octokit/rest'; import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema'; import { PullRequestLinter } from './lint'; import { LinterActions } from './linter-base'; -import { LINTER_LOGIN } from './constants'; +import { DEFEAULT_LINTER_LOGIN } from './constants'; /** * Entry point for PR linter @@ -30,7 +30,7 @@ async function run() { console.error(`Could not find PR belonging to status event, but that's not unusual. Skipping.`); process.exit(0); } - throw new Error(`Could not find PR number from context.`); + throw new Error(`Could not find PR number from event: ${github.context.eventName}`); } try { @@ -40,7 +40,7 @@ async function run() { repo, number, // On purpose || instead of ??, also collapse empty string - linterLogin: process.env.LINTER_LOGIN || LINTER_LOGIN, + linterLogin: process.env.LINTER_LOGIN || DEFEAULT_LINTER_LOGIN, }); let actions: LinterActions | undefined; From 15a032c1c9882516525edf82d9e83e83a7d8e8ca Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Jan 2025 20:50:09 +0100 Subject: [PATCH 16/16] Explain motivation for this worfklow --- .github/workflows/pr-linter-review-trigger.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/pr-linter-review-trigger.yml b/.github/workflows/pr-linter-review-trigger.yml index 8408bf1b0fed4..3d8a2086581ae 100644 --- a/.github/workflows/pr-linter-review-trigger.yml +++ b/.github/workflows/pr-linter-review-trigger.yml @@ -1,3 +1,7 @@ +# Re-evaluate the PR linter after reviews. This is used to upgrade the label +# of a PR to `needs-maintainer-review` after a trusted community members leaves +# an approving review. +# # Unprivileged workflow that runs in the context of the PR, when a review is changed. # # Save the PR number, and download it again in the PR Linter workflow which