tools/@aws-cdk/prlint/lint.ts (426 lines of code) (raw):

import * as path from 'path'; import { findModulePath, moduleStability } from './module'; import { breakingModules } from './parser'; import { CheckRun, GitHubFile, GitHubPr, Review, sumChanges, summarizeRunConclusions } from "./github"; import { TestResult, ValidationCollector } from './results'; import { CODE_BUILD_CONTEXT, CODECOV_CHECKS, 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 extends PullRequestLinterBase { private readonly trustedCommunity: string[] = []; /** * Whether or not the codebuild job for the given commit is successful * * @param sha the commit sha to evaluate */ private async codeBuildJobSucceeded(sha: string): Promise<boolean> { const statuses = await this.client.repos.listCommitStatusesForRef({ owner: this.prParams.owner, repo: this.prParams.repo, ref: sha, }); let status = statuses.data.filter(status => status.context === CODE_BUILD_CONTEXT).map(status => status.state); console.log("CodeBuild Commit Statuses: ", status); return statuses.data.some(status => status.context === CODE_BUILD_CONTEXT && status.state === 'success'); } public async validateStatusEvent(status: StatusEvent): Promise<LinterActions> { if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') { return this.assessNeedsReview(); } return {}; } /** * Assess whether or not a PR is ready for review. * This is needed because some things that we need to evaluate are not filterable on * the builtin issue search. A PR is ready for review when: * * 1. Not a draft * 2. Does not have any merge conflicts * 3. PR linter is not failing OR the user has requested an exemption * 4. A maintainer has not requested changes * 5. A maintainer has not approved * * In addition, we differentiate between ready for review by a core team member * (pr/needs-maintainer-review) or ready for review by core OR the trusted community * (pr/needs-community-review). A PR is prioritized for core team review when: * * 6. It links to a p1 issue * 7. It links to a p2 issue and has an approved community review */ private async assessNeedsReview(): Promise<LinterActions> { const pr = await this.pr(); const reviewsData = await this.client.paginate(this.client.pulls.listReviews, this.prParams); console.log(JSON.stringify(reviewsData.map(r => ({ user: r.user?.login, state: r.state, author_association: r.author_association, submitted_at: r.submitted_at, })), undefined, 2)); // NOTE: MEMBER = a member of the organization that owns the repository // COLLABORATOR = has been invited to collaborate on the repository const maintainerRequestedChanges = reviewsData.some( review => review.author_association === 'MEMBER' && review.user?.login !== 'aws-cdk-automation' && review.state === 'CHANGES_REQUESTED', ); const maintainerApproved = reviewsData.some( review => review.author_association === 'MEMBER' && review.state === 'APPROVED', ); // NOTE: community reviewers may approve, comment, or request changes; however, it // is possible for the same member to perform any combination of those actions on // a single PR. We solve this by: // 1. Filtering reviews to those by trusted community members // 2. Filtering out reviews that only leave comments (without approving or requesting changes). // This allows a reviewer to participate in a conversation about their review without // effectively dismissing their review. While GitHub does not allow community reviewers // to dismiss their reviews (which requires privileges on the repo), they can leave a // new review with the opposite approve/request state to update their review. // 3. Mapping reviewers to only their newest review // 4. Checking if any reviewers' most recent review is an approval // -> If so, the PR is considered community approved; the approval can always // be dismissed by a maintainer to respect another reviewer's requested changes. // 5. Checking if any reviewers' most recent review requested changes // -> If so, the PR is considered to still need changes to meet community review. const trustedCommunityMembers = await this.getTrustedCommunityMembers(); const reviewsByTrustedCommunityMembers = reviewsData .filter(review => trustedCommunityMembers.includes(review.user?.login ?? '')) .filter(review => review.state !== 'PENDING' && review.state !== 'COMMENTED') .reduce((grouping, review) => { // submitted_at is not present for PENDING comments but is present for other states. // Because of that, it is optional on the type but sure to be present here. Likewise, // review.user is sure to be defined because we're operating on reviews by trusted // community members let newest = grouping[review.user!.login] ?? review; if (review.submitted_at! > newest.submitted_at!) { newest = review; } return { ...grouping, [review.user!.login]: newest, }; }, {} as Record<string, typeof reviewsData[0]>); console.log('raw data: ', JSON.stringify(reviewsByTrustedCommunityMembers)); const communityApproved = Object.values(reviewsByTrustedCommunityMembers).some(({state}) => state === 'APPROVED'); const communityRequestedChanges = !communityApproved && Object.values(reviewsByTrustedCommunityMembers).some(({state}) => state === 'CHANGES_REQUESTED') const prLinterFailed = reviewsData.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review; const userRequestsExemption = pr.labels.some(label => (label.name === Exemption.REQUEST_EXEMPTION || label.name === Exemption.REQUEST_CLARIFICATION)); console.log('evaluation: ', JSON.stringify({ draft: pr.draft, mergeable_state: pr.mergeable_state, prLinterFailed, maintainerRequestedChanges, maintainerApproved, communityRequestedChanges, communityApproved, userRequestsExemption, }, undefined, 2)); const fixesP1 = pr.labels.some(label => label.name === 'p1'); let readyForReview = true; if ( // we don't need to review drafts pr.draft // or PRs with conflicts || pr.mergeable_state === 'dirty' // or PRs that already have changes requested by a maintainer || maintainerRequestedChanges // or the PR linter failed and the user didn't request an exemption || (prLinterFailed && !userRequestsExemption) // or a maintainer has already approved the PR || maintainerApproved // or a trusted community member has requested changes on a p2 PR || (!fixesP1 && communityRequestedChanges) ) { readyForReview = false; } // needs-maintainer-review means one of the following // 1) fixes a p1 bug // 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'))) { return { addLabels: ['pr/needs-maintainer-review'], removeLabels: ['pr/needs-community-review'], }; } else if (readyForReview && !fixesP1) { return { addLabels: ['pr/needs-community-review'], removeLabels: ['pr/needs-maintainer-review'], }; } else { return { removeLabels: [ 'pr/needs-community-review', 'pr/needs-maintainer-review', ], }; } } /** * Trusted community reviewers is derived from the source of truth at this wiki: * https://github.com/aws/aws-cdk/wiki/CDK-Community-PR-Reviews */ private async getTrustedCommunityMembers(): Promise<string[]> { if (this.trustedCommunity.length > 0) { return this.trustedCommunity; } const wiki = await (await fetch('https://raw.githubusercontent.com/wiki/aws/aws-cdk/CDK-Community-PR-Reviews.md')).text(); const rawMdTable = wiki.split('<!--section-->')[1].split('\n').filter(l => l !== ''); for (let i = 2; i < rawMdTable.length; i++) { this.trustedCommunity.push(rawMdTable[i].split('|')[1].trim()); } return this.trustedCommunity; } /** * 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(): Promise<LinterActions> { let ret: LinterActions = {}; const number = this.props.number; const sha = (await this.pr()).head.sha; console.log(`⌛ Fetching PR number ${number}`); const pr = await this.pr(); 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); console.log('⌛ Validating...'); const validationCollector = new ValidationCollector(pr, files); validationCollector.validateRuleSet({ exemption: shouldExemptReadme, exemptionMessage: `Not validating README changes since the PR is labeled with '${Exemption.README}'`, testRuleSet: [{ test: featureContainsReadme }], }); validationCollector.validateRuleSet({ exemption: shouldExemptTest, exemptionMessage: `Not validating test changes since the PR is labeled with '${Exemption.TEST}'`, testRuleSet: [{ test: featureContainsTest }, { test: fixContainsTest }], }); validationCollector.validateRuleSet({ exemption: shouldExemptIntegTest, exemptionMessage: `Not validating integration test changes since the PR is labeled with '${Exemption.INTEG_TEST}'`, testRuleSet: [{ test: featureContainsIntegTest }, { test: fixContainsIntegTest }], }); validationCollector.validateRuleSet({ exemption: shouldExemptBreakingChange, exemptionMessage: `Not validating breaking changes since the PR is labeled with '${Exemption.BREAKING_CHANGE}'`, testRuleSet: [{ test: assertStability }], }); validationCollector.validateRuleSet({ exemption: shouldExemptCliIntegTested, testRuleSet: [{ test: noCliChanges }], }); validationCollector.validateRuleSet({ exemption: shouldExemptAnalyticsMetadataChange, testRuleSet: [ { test: noMetadataChanges }, { test: noAnalyticsClassesChanges }, { test: noAnalyticsEnumsChanges }, { test: noAnalyticsEnumAutomationChanges }, { test: noAnalyticsEnumLikeAutomationChanges }, ], }); validationCollector.validateRuleSet({ testRuleSet: [ { test: validateBreakingChangeFormat }, { test: validateTitlePrefix }, { test: validateTitleScope }, { test: validateTitleLowercase }, { test: validateBranch }, ], }); validationCollector.validateRuleSet({ exemption: shouldExemptSizeCheck, testRuleSet: [ { test: prIsSmall }, ], }) if (pr.base.ref === 'main') { // Only check CodeCov for PRs targeting 'main' const runs = await this.checkRuns(sha); const codeCovRuns = CODECOV_CHECKS.map(c => runs[c] as CheckRun | undefined); validationCollector.validateRuleSet({ exemption: () => hasLabel(pr, Exemption.CODECOV), testRuleSet: [{ test: () => { const summary = summarizeRunConclusions(codeCovRuns.map(r => r?.conclusion)); console.log('CodeCov Summary:', summary); switch (summary) { case 'failure': return TestResult.failure('CodeCov is indicating a drop in code coverage'); // If we don't know the result of the CodeCov results yet, we pretend that there isn't a problem. // // It would be safer to ask for changes until we're confident that CodeCov has passed, but if we do // that the following sequence of events happens: // // 1. PR is ready to be merged (approved, everything passes) // 2. Mergify enqueues it and merges from main // 3. CodeCov needs to run again // 4. PR linter requests changes because CodeCov result is uncertain // 5. Mergify dequeues the PR because PR linter requests changes // // This looks very confusing and noisy, and also will never fix itself, so the PR ends up unmerged. // // The better solution would probably be not to do a "Request Changes" review, but leave a comment // and create a GitHub "status" on the PR to say 'success/pending/failure', and make it required. // (https://github.com/aws/aws-cdk/issues/33136) // // For now, not doing anything with a 'waiting' status is a smaller delta, and the race condition posed by it is // unlikely to happen given that there are much slower jobs that the merge is blocked on anyway. case 'waiting': return TestResult.success(); case 'success': return TestResult.success(); } }, }], }); } // We always delete all comments; in the future we will just communicate via reviews. ret.deleteComments = await this.findExistingPRLinterComments(); ret = mergeLinterActions(ret, await this.validationToActions(validationCollector)); // 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'); ret = mergeLinterActions(ret, await this.assessNeedsReview()); } } catch (e) { console.log(`assessing review failed for sha ${sha}: `, e); } return ret; } /** * Creates a new review, requesting changes, with the reasons that the linter did not pass. * @param result The result of the PR Linter run. */ private async validationToActions(result: ValidationCollector): Promise<LinterActions> { 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 { return pr.title.startsWith('feat'); } function isFix(pr: GitHubPr): boolean { return pr.title.startsWith('fix'); } function testChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('test')).length != 0; } function integTestChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().match(/integ.*.ts$/)).length != 0; } function integTestSnapshotChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes('.snapshot')).length != 0; } function readmeChanged(files: GitHubFile[]): boolean { return files.filter(f => path.basename(f.filename) == 'README.md').length != 0; } function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !readmeChanged(files), 'Features must contain a change to a README file.'); return result; } function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !testChanged(files), 'Features must contain a change to a test file.'); return result; } function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && !testChanged(files), 'Fixes must contain a change to a test file.'); return result; } function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Features must contain a change to an integration test file and the resulting snapshot.'); return result; } function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Fixes must contain a change to an integration test file and the resulting snapshot.'); return result; } function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); } function shouldExemptTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.TEST); } function shouldExemptIntegTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.INTEG_TEST); } function shouldExemptBreakingChange(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.BREAKING_CHANGE); } function shouldExemptCliIntegTested(pr: GitHubPr): boolean { return (hasLabel(pr, Exemption.CLI_INTEG_TESTED) || pr.user?.login === 'aws-cdk-automation'); } function shouldExemptSizeCheck(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.SIZE_CHECK); } function shouldExemptAnalyticsMetadataChange(pr: GitHubPr): boolean { return (hasLabel(pr, Exemption.ANALYTICS_METADATA_CHANGE) || pr.user?.login === 'aws-cdk-automation'); } function hasLabel(pr: GitHubPr, labelName: string): boolean { return pr.labels.some(function (l: any) { return l.name === labelName; }); } /** * Check that the 'BREAKING CHANGE:' note in the body is correct. * * Check this by looking for something that most likely was intended * to be said note, but got misspelled as "BREAKING CHANGES:" or * "BREAKING CHANGES(module):" */ function validateBreakingChangeFormat(pr: GitHubPr, _files: GitHubFile[]): TestResult { const title = pr.title; const body = pr.body; const result = new TestResult(); const re = /^BREAKING.*$/m; const m = re.exec(body ?? ''); if (m) { result.assessFailure(!m[0].startsWith('BREAKING CHANGE: '), `Breaking changes should be indicated by starting a line with 'BREAKING CHANGE: ', variations are not allowed. (found: '${m[0]}').`); result.assessFailure(m[0].slice('BREAKING CHANGE:'.length).trim().length === 0, 'The description of the first breaking change should immediately follow the \'BREAKING CHANGE: \' clause.'); const titleRe = /^[a-z]+\([0-9a-z-_]+\)/; result.assessFailure(!titleRe.exec(title), 'The title of this pull request must specify the module name that the first breaking change should be associated to.'); } return result; } /** * Check that the PR title has the correct prefix. */ function validateTitlePrefix(pr: GitHubPr): TestResult { const result = new TestResult(); const validTypes = "feat|fix|build|chore|ci|docs|style|refactor|perf|test|revert"; const titleRe = new RegExp(`^(${validTypes})(\\([\\w_-]+\\))?: `); const m = titleRe.exec(pr.title); result.assessFailure( !m, `The title prefix of this pull request must be one of "${validTypes}"`); return result; } /** * Check that the PR title uses the typical convention for package names, and is lowercase. * * For example, "fix(s3)" is preferred over "fix(aws-s3)". */ function validateTitleScope(pr: GitHubPr): TestResult { const result = new TestResult(); const scopesExemptFromThisRule = ['aws-cdk-lib']; // Specific commit types are handled by `validateTitlePrefix`. This just checks whether // the scope includes an `aws-` prefix or not. // Group 1: Scope with parens - "(aws-<name>)" // Group 2: Scope name - "aws-<name>" // Group 3: Preferred scope name - "<name>" const titleRe = /^\w+(\((aws-([\w_-]+))\))?: /; const m = titleRe.exec(pr.title); if (m && !scopesExemptFromThisRule.includes(m[2])) { result.assessFailure( !!(m[2] && m[3]), `The title scope of the pull request should omit 'aws-' from the name of modified packages. Use '${m[3]}' instead of '${m[2]}'.`, ); } // Title scope is lowercase const scopeRe = /^\w+\(([\w_-]+)\)?: /; // Isolate the scope const scope = scopeRe.exec(pr.title); if (scope && scope[1]) { result.assessFailure( scope[1] !== scope[1].toLocaleLowerCase(), `The title scope of the pull request should be entirely in lowercase. Use '${scope[1].toLocaleLowerCase()}' instead.`, ); } return result; } function validateTitleLowercase(pr: GitHubPr): TestResult { const result = new TestResult(); const start = pr.title.indexOf(':'); const firstLetter = pr.title.charAt(start + 2); result.assessFailure( firstLetter !== firstLetter.toLocaleLowerCase(), '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 "``".', ); return result; } /** * Check that the PR is not opened from main branch of author's fork * * @param pr github pr * @returns test result */ function validateBranch(pr: GitHubPr): TestResult { const result = new TestResult(); if (pr.head && pr.head.ref) { result.assessFailure(pr.head.ref === 'main', PR_FROM_MAIN_ERROR); } return result; } function assertStability(pr: GitHubPr, _files: GitHubFile[]): TestResult { const title = pr.title; const body = pr.body; const result = new TestResult(); const breakingStable = breakingModules(title, body ?? '').filter(mod => 'stable' === moduleStability(findModulePath(mod))); result.assessFailure(breakingStable.length > 0, `Breaking changes in stable modules [${breakingStable.join(', ')}] is disallowed.`); return result; } function noCliChanges(pr: GitHubPr, files: GitHubFile[]): TestResult { const branch = `pull/${pr.number}/head`; const cliCodeChanged = files.some(f => f.filename.toLowerCase().includes('packages/aws-cdk/lib/') && f.filename.endsWith('.ts')); return TestResult.fromFailure( cliCodeChanged, `CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin ${branch} && git push -f origin FETCH_HEAD:test-main-pipeline), then add the '${Exemption.CLI_INTEG_TESTED}' label when the pipeline succeeds.`, ); } function noMetadataChanges(_pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); const condition = files.some(file => file.filename === 'packages/aws-cdk-lib/region-info/build-tools/metadata.ts'); result.assessFailure(condition, 'Manual changes to the metadata.ts file are not allowed.'); return result; } function noAnalyticsClassesChanges(_pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); const condition = files.some(file => file.filename === 'packages/aws-cdk-lib/core/lib/analytics-data-source/classes.ts'); result.assessFailure(condition, 'Manual changes to the classes.ts file are not allowed.'); return result; } function noAnalyticsEnumsChanges(_pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); const condition = files.some(file => file.filename === 'packages/aws-cdk-lib/core/lib/analytics-data-source/enums.ts'); result.assessFailure(condition, 'Manual changes to the enums.ts file are not allowed.'); return result; } function noAnalyticsEnumAutomationChanges(_pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); const condition = files.some(file => file.filename === 'packages/aws-cdk-lib/core/lib/analytics-data-source/enums/module-enums.json'); result.assessFailure(condition, 'Manual changes to the module-enums.json file are not allowed.'); return result; } function noAnalyticsEnumLikeAutomationChanges(_pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); const condition = files.some(file => file.filename === 'packages/aws-cdk-lib/core/lib/analytics-data-source/enums/module-enumlikes.json'); result.assessFailure(condition, 'Manual changes to the module-enumlikes.json file are not allowed.'); return result; } function prIsSmall(_pr: GitHubPr, files: GitHubFile[]): TestResult { const folders = ['packages/aws-cdk/', 'packages/@aws-cdk-testing/cli-integ/']; const exclude = [/THIRD_PARTY_LICENSES/, /.*\.md/, /.*\.test\.ts/]; const maxLinesAdded = 1000; const maxLinesRemoved = 1000; const filesToCheck: GitHubFile[] = files .filter(r => folders.some(folder => r.filename.startsWith(folder))) .filter(r => exclude.every(re => !re.test(r.filename))); const sum = sumChanges(filesToCheck); const result = new TestResult(); result.assessFailure( sum.additions > maxLinesAdded, `The number of lines added (${sum.additions}) is greater than ${maxLinesAdded}` ); result.assessFailure( sum.deletions > maxLinesRemoved, `The number of lines removed (${sum.deletions}) is greater than ${maxLinesAdded}` ); return result; } require('make-runnable/custom')({ printOutputFrame: false, });