diff --git a/.github/workflows/pr-linter-trigger.yml b/.github/workflows/pr-linter-review-trigger.yml similarity index 54% rename from .github/workflows/pr-linter-trigger.yml rename to .github/workflows/pr-linter-review-trigger.yml index fb13df957a1c9..3d8a2086581ae 100644 --- a/.github/workflows/pr-linter-trigger.yml +++ b/.github/workflows/pr-linter-review-trigger.yml @@ -1,3 +1,12 @@ +# 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 +# needs to run in privileged `workflow_run` context (but then must restore the +# PR context). name: PR Linter Trigger on: diff --git a/.github/workflows/pr-linter.yml b/.github/workflows/pr-linter.yml index 89e5944c89ef0..e45b6c7f95ece 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,7 @@ 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 }} + 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 new file mode 100644 index 0000000000000..ab88a896ca352 --- /dev/null +++ b/tools/@aws-cdk/prlint/constants.ts @@ -0,0 +1,18 @@ + +export const DEFEAULT_LINTER_LOGIN = 'aws-cdk-automation'; + +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", +} diff --git a/tools/@aws-cdk/prlint/github.ts b/tools/@aws-cdk/prlint/github.ts new file mode 100644 index 0000000000000..17ddc07858147 --- /dev/null +++ b/tools/@aws-cdk/prlint/github.ts @@ -0,0 +1,33 @@ +import { Endpoints } from '@octokit/types'; +import { StatusEvent } from '@octokit/webhooks-definitions/schema'; + +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; + } | null; + 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; +} diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 4512a28ad1ac9..d430fd745f34f 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -2,55 +2,109 @@ 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'; +import { LinterActions } from './linter-base'; +import { DEFEAULT_LINTER_LOGIN } from './constants'; +/** + * 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) LINTER_LOGIN=my-gh-alias 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 }); + const owner = github.context.repo.owner; + const repo = github.context.repo.repo; + + 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); + } + throw new Error(`Could not find PR number from event: ${github.context.eventName}`); + } + try { + const prLinter = new PullRequestLinter({ + client, + owner, + repo, + number, + // On purpose || instead of ??, also collapse empty string + linterLogin: process.env.LINTER_LOGIN || DEFEAULT_LINTER_LOGIN, + }); + + let actions: LinterActions | undefined; + 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; - const pr = await linter.PullRequestLinter.getPRFromCommit(client, 'aws', 'aws-cdk', 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); - } - 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); + console.log('validating status event'); + actions = await prLinter.validateStatusEvent(statusPayload); 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); + // This is the main PR validator action. + actions = await prLinter.validatePullRequestTarget(); + 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); + } + + await prLinter.actionsToException(actions); } + } catch (error: any) { + // Fail the action core.setFailed(error.message); } } -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 48c5a23819459..30e6c3f2b5708 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -1,335 +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 { 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)'; - -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', -} - -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 { 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'; /** * 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 * @@ -346,10 +31,11 @@ export class PullRequestLinter { 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 {}; } /** @@ -370,9 +56,9 @@ export class PullRequestLinter { * 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)); @@ -462,43 +148,25 @@ export class PullRequestLinter { // 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', + ], + }; } } - 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 @@ -518,11 +186,15 @@ export class PullRequestLinter { * 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.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}`); const files = await this.client.paginate(this.client.pulls.listFiles, this.prParams); @@ -575,29 +247,48 @@ export class PullRequestLinter { ], }); - console.log("Deleting PR Linter Comment now"); - await this.deletePRLinterComment(); + ret = mergeLinterActions(ret, await this.validationToActions(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; } - private formatErrors(errors: string[]) { - return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; - }; + /** + * 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 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 prAuthor = (await this.pr()).user?.login; + + const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams); + const exemptionRequest = comments.some(comment => comment.user?.login === prAuthor && comment.body?.toLowerCase().includes("exemption request")); + + return { + requestChanges: { + failures: result.errors, + exemptionRequest, + }, + }; + } + } } function isFeature(pr: GitHubPr): boolean { @@ -682,6 +373,7 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean { }); } + /** * 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..8e14dcb5355dc --- /dev/null +++ b/tools/@aws-cdk/prlint/linter-base.ts @@ -0,0 +1,322 @@ +import { Octokit } from '@octokit/rest'; +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.'; + +/** + * 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; + + /** + * For cases where the linter needs to know its own username + */ + readonly linterLogin: string; +} + + +/** + * 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 }; + protected readonly linterLogin: string; + + 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 }; + this.linterLogin = props.linterLogin; + } + + 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.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 existingReviews = await this.findExistingPRLinterReview(); + + if (actions.dismissPreviousReview) { + this.dismissPRLinterReviews(existingReviews, 'passing'); + } + if (actions.requestChanges) { + this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReviews); + } + } + } + + /** + * 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')); + } + } + + /** + * 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 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)}`); + } + } + } + + /** + * 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.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, existingReviews?: Review[]): Promise { + // FIXME: this function is doing too much at once. We should split this out into separate + // actions on `LinterActions`. + + 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, + event: 'REQUEST_CHANGES', + 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, + 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.' + console.log('Closing pull request'); + + await this.client.issues.createComment({ + ...this.issueParams, + body: errorMessageBody, + }); + + await this.client.pulls.update({ + ...this.prParams, + state: 'closed', + }); + } + } + + private formatErrors(errors: string[]) { + return errors.map(e => ` ❌ ${e}`).join('\n'); + } + + private addLabel(label: string, pr: Pick) { + // already has label, so no-op + if (pr.labels.some(l => l.name === label)) { return; } + 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; } + 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); + } +} + +/** + * 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[]; + + /** + * Post a "request changes" review + */ + requestChanges?: { + failures: string[]; + exemptionRequest?: boolean; + }; + + /** + * 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 ?? [])]), + 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/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 1de1f3c334ee0..40a43a159033a 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -1,5 +1,8 @@ import * as path from 'path'; -import * as linter from '../lint'; +import { GitHubFile, GitHubLabel, GitHubPr } from '../github'; +import { CODE_BUILD_CONTEXT } from '../constants'; +import { PullRequestLinter } from '../lint'; +import { StatusEvent } from '@octokit/webhooks-definitions/schema'; let mockRemoveLabel = jest.fn(); let mockAddLabel = jest.fn(); @@ -10,7 +13,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 +26,7 @@ afterAll(() => { jest.resetAllMocks(); }); -let mockCreateReview: (errorMessage: string) => Promise; +let mockCreateReview: (errorMessage: string) => Promise = jest.fn(); const SHA = 'ABC'; type Subset = { @@ -38,7 +41,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:', @@ -48,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)).rejects.toThrow(/'BREAKING CHANGE: ', variations are not allowed/); }); test('the first breaking change should immediately follow "BREAKING CHANGE:"', async () => { @@ -63,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)).rejects.toThrow(/description of the first breaking change should immediately follow/); }); test('invalid title', async () => { @@ -77,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)).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 () => { @@ -91,12 +94,12 @@ describe('breaking changes format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; // not throw + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; // not throw }); }); 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', @@ -110,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)).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', () => { @@ -125,7 +128,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('invalid scope with aws- prefix', async () => { @@ -139,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)).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 () => { @@ -153,7 +156,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('valid with missing scope', async () => { @@ -167,7 +170,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('valid with aws-cdk-lib as a scope', async () => { @@ -181,7 +184,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test.each(['core', 'prlint', 'awslint'])('valid scope for packages that dont use aws- prefix', async (scope) => { @@ -195,7 +198,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('invalid capitalized title', async () => { @@ -209,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)).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 () => { @@ -223,7 +226,7 @@ describe('commit message format', () => { }, }; const prLinter = configureMock(issue, undefined); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); }); @@ -239,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)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); test('breaking changes multiple in stable modules', async () => { @@ -257,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)).rejects.toThrow('Breaking changes in stable modules [lambda, ecs] is disallowed.'); }); test('unless exempt-breaking-change label added', async () => { @@ -274,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)).resolves; // not throw }); test('with additional "closes" footer', async () => { @@ -294,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)).rejects.toThrow('Breaking changes in stable modules [s3] is disallowed.'); }); }); @@ -325,7 +328,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('integ files not changed in feat', async () => { @@ -354,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)).rejects.toThrow( + 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -389,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)).rejects.toThrow( + 'Features must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -424,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)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -459,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)).rejects.toThrow( + 'Fixes must contain a change to an integration test file and the resulting snapshot.' ); }); @@ -491,7 +478,7 @@ describe('integration tests required on features', () => { }, ]; const prLinter = configureMock(issue, files); - expect(await prLinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prLinter)).resolves; }); test('integ files not changed, not a feature', async () => { @@ -517,11 +504,11 @@ describe('integration tests required on features', () => { }, ]; const prlinter = configureMock(issue, files); - expect(await prlinter.validatePullRequestTarget(SHA)).resolves; + expect(legacyValidatePullRequestTarget(await prlinter)).resolves; }); describe('CLI file changed', () => { - const labels: linter.GitHubLabel[] = []; + const labels: GitHubLabel[] = []; const issue = { number: 23, title: 'chore(cli): change the help or something', @@ -538,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)).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)).resolves; }); test('with aws-cdk-automation author', async () => { @@ -557,7 +544,7 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(issue, files); - await prLinter.validatePullRequestTarget(SHA); +legacyValidatePullRequestTarget( await prLinter); // THEN: no exception }); }); @@ -580,9 +567,9 @@ describe('integration tests required on features', () => { test('needs a review', async () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -600,9 +587,9 @@ describe('integration tests required on features', () => { // WHEN pr.labels = [{ name: 'p1' }]; const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -631,9 +618,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -662,9 +649,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -699,9 +686,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -732,9 +719,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -765,9 +752,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -805,9 +792,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -843,9 +830,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -873,9 +860,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -901,9 +888,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -930,9 +917,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -960,9 +947,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -993,9 +980,9 @@ describe('integration tests required on features', () => { // WHEN const prLinter = configureMock(pr); - await prLinter.validateStatusEvent(pr as any, { + await legacyValidateStatusEvent(prLinter, { sha: SHA, - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', } as any); @@ -1026,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)).rejects.toThrow(); // THEN expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ @@ -1040,72 +1027,80 @@ describe('integration tests required on features', () => { }); 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']); - 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.', + 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.']); - 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.', + 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.', ); }); 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']); - 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.', + 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', () => { - const files: linter.GitHubFile[] = [{ + const files: GitHubFile[] = [{ filename: 'packages/aws-cdk-lib/region-info/build-tools/metadata.ts', }]; @@ -1120,7 +1115,7 @@ describe('integration tests required on features', () => { }; const prLinter = configureMock(pr, files); - await expect(prLinter.validatePullRequestTarget(SHA)).resolves; + await expect(legacyValidatePullRequestTarget(prLinter)).resolves; }); test('with another author', async () => { @@ -1134,15 +1129,15 @@ describe('integration tests required on features', () => { }; const prLinter = configureMock(pr, files); - await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(); + await expect(legacyValidatePullRequestTarget(prLinter)).rejects.toThrow(); }); }); }); -function configureMock(pr: Subset, prFiles?: linter.GitHubFile[], existingComments?: string[]): linter.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 }; + return { data: { ...pr, base: { ref: 'main', ...pr?.base }, head: { sha: 'ABC', ...pr?.head }} }; }, listFiles(_props: { _owner: string, _repo: string, _pull_number: number }) { @@ -1159,6 +1154,8 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ dismissReview() {}, + updateReview() { }, + update() {}, }; @@ -1170,7 +1167,7 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ 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 }; }, @@ -1183,7 +1180,7 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ listCommitStatusesForRef() { return { data: [{ - context: linter.CODE_BUILD_CONTEXT, + context: CODE_BUILD_CONTEXT, state: 'success', }], }; @@ -1193,10 +1190,11 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ const searchClient = { issuesAndPullRequests() {}, }; - return new linter.PullRequestLinter({ + return new PullRequestLinter({ owner: 'aws', repo: 'aws-cdk', number: 1000, + linterLogin: 'aws-cdk-automation', // hax hax client: { @@ -1208,3 +1206,36 @@ function configureMock(pr: Subset, prFiles?: linter.GitHubFile[ } 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) { + 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); + await prLinter.executeActions(actions); +}