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/CONTRIBUTING.md b/CONTRIBUTING.md index 98f02c5fa0f92..d3396c4c683cc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -718,6 +718,10 @@ To make this easier we have a `pr/needs-review` label that we can add to each PR. If you do not see this label on your PR then it means that something needs to be fixed before it can be reviewed. +> [!NOTE] +> The `aws-cdk` repository is frequently updated, so PR branches may quickly become out-of-date, showing "This branch is out-of-date with the base branch." This is not an issue as long as there are no conflicts with the newly merged commits. Once the PR is approved, our automation will update it with the latest `main` branch and handle the merge. No action is needed on your part. + + #### Adding construct runtime dependencies Any tool that is not part of the CDK, and needs to be used by a construct during diff --git a/packages/aws-cdk-lib/aws-appsync/README.md b/packages/aws-cdk-lib/aws-appsync/README.md index 1f85e8bdedc08..0ada4db50c5ec 100644 --- a/packages/aws-cdk-lib/aws-appsync/README.md +++ b/packages/aws-cdk-lib/aws-appsync/README.md @@ -23,6 +23,7 @@ type demo { } type Query { getDemos: [ demo! ] + getDemosConsistent: [demo!] } input DemoInput { version: String! @@ -928,4 +929,4 @@ const api = new appsync.GraphqlApi(this, 'OwnerContact', { definition: appsync.Definition.fromSchema(appsync.SchemaFile.fromAsset(path.join(__dirname, 'appsync.test.graphql'))), ownerContact: 'test-owner-contact', }); -``` \ No newline at end of file +``` 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); +}