diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts index 3519d02d2643f..46b41908a7a4b 100644 --- a/tools/@aws-cdk/prlint/index.ts +++ b/tools/@aws-cdk/prlint/index.ts @@ -1,10 +1,11 @@ import * as core from '@actions/core'; import * as github from '@actions/github'; +import { Octokit } from '@octokit/rest'; import * as linter from './lint'; async function run() { const token: string = process.env.GITHUB_TOKEN!; - const client = github.getOctokit(token).rest.pulls; + const client = new Octokit({ auth: token }); const prLinter = new linter.PullRequestLinter({ client, diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 37c124235030c..ec2be8a86bae0 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -1,4 +1,5 @@ import * as path from 'path'; +import { Octokit } from '@octokit/rest'; import { breakingModules } from './parser'; import { findModulePath, moduleStability } from './module'; @@ -9,7 +10,23 @@ 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' + BREAKING_CHANGE = 'pr-linter/exempt-breaking-change', + CLI_INTEG_TESTED = 'pr-linter/cli-integ-tested', +} + +export interface GitHubPr { + readonly number: number; + readonly title: string; + readonly body: string | null; + readonly labels: GitHubLabel[]; +} + +export interface GitHubLabel { + readonly name: string; +} + +export interface GitHubFile { + readonly filename: string; } class LinterError extends Error { @@ -25,6 +42,15 @@ class LinterError extends Error { * 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[] = []; /** @@ -44,7 +70,7 @@ class TestResult { * Represents a single test. */ interface Test { - test: (pr: any, files: any[]) => TestResult; + test: (pr: GitHubPr, files: GitHubFile[]) => TestResult; } /** @@ -55,7 +81,7 @@ interface ValidateRuleSetOptions { /** * The function to test for exemption from the rules in testRuleSet. */ - exemption?: (pr: any) => boolean; + exemption?: (pr: GitHubPr) => boolean; /** * The log message printed if the exemption is granted. @@ -75,7 +101,7 @@ interface ValidateRuleSetOptions { class ValidationCollector { public errors: string[] = []; - constructor(private pr: any, private files: string[]) { } + constructor(private pr: GitHubPr, private files: GitHubFile[]) { } /** * Checks for exemption criteria and then validates against the ruleset when not exempt to it. @@ -107,7 +133,7 @@ export interface PullRequestLinterProps { /** * GitHub client scoped to pull requests. Imported via @actions/github. */ - readonly client: any; + readonly client: Octokit; /** * Repository owner. @@ -130,7 +156,7 @@ export interface PullRequestLinterProps { * in the body of the review, and dismiss any previous reviews upon changes to the pull request. */ export class PullRequestLinter { - private readonly client: any; + private readonly client: Octokit; private readonly prParams: { owner: string, repo: string, pull_number: number }; @@ -143,10 +169,10 @@ export class PullRequestLinter { * Dismisses previous reviews by aws-cdk-automation when changes have been made to the pull request. */ private async dismissPreviousPRLinterReviews(): Promise { - const reviews = await this.client.listReviews(this.prParams); + const reviews = await this.client.pulls.listReviews(this.prParams); reviews.data.forEach(async (review: any) => { if (review.user?.login === 'github-actions[bot]' && review.state !== 'DISMISSED') { - await this.client.dismissReview({ + await this.client.pulls.dismissReview({ ...this.prParams, review_id: review.id, message: 'Pull Request updated. Dissmissing previous PRLinter Review.', @@ -161,7 +187,7 @@ export class PullRequestLinter { */ private async communicateResult(failureReasons: string[]): Promise { const body = `The Pull Request Linter fails with the following errors:${this.formatErrors(failureReasons)}PRs must pass status checks before we can provide a meaningful review.`; - await this.client.createReview({ ...this.prParams, body, event: 'REQUEST_CHANGES', }); + await this.client.pulls.createReview({ ...this.prParams, body, event: 'REQUEST_CHANGES', }); throw new LinterError(body); } @@ -173,10 +199,10 @@ export class PullRequestLinter { const number = this.props.number; console.log(`⌛ Fetching PR number ${number}`); - const pr = (await this.client.get(this.prParams)).data; + const pr = (await this.client.pulls.get(this.prParams)).data; console.log(`⌛ Fetching files for PR number ${number}`); - const files = (await this.client.listFiles(this.prParams)).data; + const files = (await this.client.pulls.listFiles(this.prParams)).data; console.log("⌛ Validating..."); @@ -214,6 +240,11 @@ export class PullRequestLinter { testRuleSet: [ { test: assertStability } ] }); + validationCollector.validateRuleSet({ + exemption: (pr) => hasLabel(pr, Exemption.CLI_INTEG_TESTED), + testRuleSet: [ { test: noCliChanges } ], + }); + await this.dismissPreviousPRLinterReviews(); validationCollector.isValid() ? console.log("✅ Success") : await this.communicateResult(validationCollector.errors); } @@ -221,85 +252,87 @@ export class PullRequestLinter { private formatErrors(errors: string[]) { return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; }; + } -function isPkgCfnspec(pr: any): boolean { +function isPkgCfnspec(pr: GitHubPr): boolean { return pr.title.indexOf("(cfnspec)") > -1; } -function isFeature(pr: any): boolean { +function isFeature(pr: GitHubPr): boolean { return pr.title.startsWith("feat") } -function isFix(pr: any): boolean { +function isFix(pr: GitHubPr): boolean { return pr.title.startsWith("fix") } -function testChanged(files: any[]): boolean { +function testChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes("test")).length != 0; } -function integTestChanged(files: any[]): boolean { +function integTestChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().match(/integ.*.ts$/)).length != 0; } -function integTestSnapshotChanged(files: any[]): boolean { +function integTestSnapshotChanged(files: GitHubFile[]): boolean { return files.filter(f => f.filename.toLowerCase().includes("integ.snapshot")).length != 0; } -function readmeChanged(files: any[]): boolean { +function readmeChanged(files: GitHubFile[]): boolean { return files.filter(f => path.basename(f.filename) == "README.md").length != 0; } -function featureContainsReadme(pr: any, files: any[]): TestResult { +function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !readmeChanged(files) && !isPkgCfnspec(pr), 'Features must contain a change to a README file.'); return result; }; -function featureContainsTest(pr: any, files: any[]): TestResult { +function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !testChanged(files), 'Features must contain a change to a test file.'); return result; }; -function fixContainsTest(pr: any, files: any[]): TestResult { +function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && !testChanged(files), 'Fixes must contain a change to a test file.'); return result; }; -function featureContainsIntegTest(pr: any, files: any[]): TestResult { +function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Features must contain a change to an integration test file and the resulting snapshot.'); return result; }; -function fixContainsIntegTest(pr: any, files: any[]): TestResult { +function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Fixes must contain a change to an integration test file and the resulting snapshot.'); return result; }; -function shouldExemptReadme(pr: any): boolean { + +function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); }; -function shouldExemptTest(pr: any): boolean { +function shouldExemptTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.TEST); }; -function shouldExemptIntegTest(pr: any): boolean { +function shouldExemptIntegTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.INTEG_TEST); }; -function shouldExemptBreakingChange(pr: any): boolean { +function shouldExemptBreakingChange(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.BREAKING_CHANGE); }; -function hasLabel(pr: any, labelName: string): boolean { +function hasLabel(pr: GitHubPr, labelName: string): boolean { return pr.labels.some(function (l: any) { return l.name === labelName; }) @@ -312,12 +345,12 @@ function hasLabel(pr: any, labelName: string): boolean { * to be said note, but got misspelled as "BREAKING CHANGES:" or * "BREAKING CHANGES(module):" */ - function validateBreakingChangeFormat(pr: any, _files: any[]): TestResult { + 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); + 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.`); @@ -330,25 +363,35 @@ function hasLabel(pr: any, labelName: string): boolean { /** * Check that the PR title has the correct prefix. */ - function validateTitlePrefix(title: string): TestResult { + function validateTitlePrefix(pr: GitHubPr): TestResult { const result = new TestResult(); - const titleRe = /^(feat|fix|build|chore|ci|docs|style|refactor|perf|test)(\([\w-]+\)){0,1}: /; - const m = titleRe.exec(title); - if (m) { - result.assessFailure(m[0] !== undefined, "The title of this pull request must specify the module name that the first breaking change should be associated to."); - } + const titleRe = /^(feat|fix|build|chore|ci|docs|style|refactor|perf|test)(\([\w_-]+\))?: /; + const m = titleRe.exec(pr.title); + result.assessFailure( + !m, + "The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/."); return result; }; -function assertStability(pr: any, _files: any[]): TestResult { +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))); + 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.`); +} + require('make-runnable/custom')({ printOutputFrame: false }); diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 703108868cc77..d7f0140a2fe76 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -14,6 +14,7 @@ let mockCreateReview: (errorMessage: string) => Promise; describe('breaking changes format', () => { test('disallow variations to "BREAKING CHANGE:"', async () => { const issue = { + number: 1, title: 'chore: some title', body: 'BREAKING CHANGES:', labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }] @@ -24,6 +25,7 @@ describe('breaking changes format', () => { test('the first breaking change should immediately follow "BREAKING CHANGE:"', async () => { const issue = { + number: 1, title: 'chore(cdk-build-tools): some title', body: `BREAKING CHANGE:\x20 * **module:** another change`, @@ -35,6 +37,7 @@ describe('breaking changes format', () => { test('invalid title', async () => { const issue = { + number: 1, title: 'chore(): some title', body: 'BREAKING CHANGE: this breaking change', labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }] @@ -45,6 +48,7 @@ describe('breaking changes format', () => { test('valid title', async () => { const issue = { + number: 1, title: 'chore(cdk-build-tools): some title', body: 'BREAKING CHANGE: this breaking change', labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }] @@ -57,6 +61,7 @@ describe('breaking changes format', () => { describe('ban breaking changes in stable modules', () => { test('breaking change in stable module', async () => { const issue = { + number: 1, title: 'chore(s3): some title', body: 'BREAKING CHANGE: this breaking change', labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-readme' }] @@ -67,6 +72,7 @@ describe('ban breaking changes in stable modules', () => { test('breaking changes multiple in stable modules', async () => { const issue = { + number: 1, title: 'chore(lambda): some title', body: ` BREAKING CHANGE: this breaking change @@ -81,6 +87,7 @@ describe('ban breaking changes in stable modules', () => { test('unless exempt-breaking-change label added', async () => { const issue = { + number: 1, title: 'chore(lambda): some title', body: ` BREAKING CHANGE: this breaking change @@ -94,6 +101,7 @@ describe('ban breaking changes in stable modules', () => { test('with additional "closes" footer', async () => { const issue = { + number: 1, title: 'chore(s3): some title', body: ` description of the commit @@ -112,6 +120,7 @@ describe('ban breaking changes in stable modules', () => { describe('integration tests required on features', () => { test('integ files changed', async () => { const issue = { + number: 1, title: 'feat(s3): some title', body: ` description of the commit @@ -137,6 +146,7 @@ describe('integration tests required on features', () => { test('integ files not changed in feat', async () => { const issue = { + number: 1, title: 'feat(s3): some title', body: ` description of the commit @@ -166,6 +176,7 @@ describe('integration tests required on features', () => { test('integ snapshots not changed in feat', async () => { const issue = { + number: 1, title: 'feat(s3): some title', body: ` description of the commit @@ -195,6 +206,7 @@ describe('integration tests required on features', () => { test('integ files not changed in fix', async () => { const issue = { + number: 1, title: 'fix(s3): some title', body: ` description of the commit @@ -224,6 +236,7 @@ describe('integration tests required on features', () => { test('integ snapshots not changed in fix', async () => { const issue = { + number: 1, title: 'fix(s3): some title', body: ` description of the commit @@ -253,6 +266,7 @@ describe('integration tests required on features', () => { test('integ files not changed, pr exempt', async () => { const issue = { + number: 1, title: 'feat(s3): some title', body: ` description of the commit @@ -275,6 +289,7 @@ describe('integration tests required on features', () => { test('integ files not changed, not a feature', async () => { const issue = { + number: 1, title: 'chore(s3): some title', body: ` description of the commit @@ -288,23 +303,49 @@ describe('integration tests required on features', () => { filename: 'some-test.test.ts' }, { - filename: 'README.md' + filename: 'readme.md' } ]; - const prLinter = configureMock(issue, files); - expect(await prLinter.validate()).resolves; + const prlinter = configureMock(issue, files); + expect(await prlinter.validate()).resolves; + }); + + describe('CLI file changed', () => { + const labels: linter.GitHubLabel[] = []; + const issue = { + number: 23, + title: 'chore(cli): change the help or something', + body: ` + description of the commit + closes #123456789 + `, + labels, + }; + const files = [ { filename: 'packages/aws-cdk/lib/cdk-toolkit.ts' } ]; + + test('no label throws error', async () => { + const prLinter = configureMock(issue, files); + await expect(prLinter.validate()).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); + await prLinter.validate(); + // THEN: no exception + }); }); }); -function configureMock(issue: any, prFiles: any[] | undefined): linter.PullRequestLinter { - const client = { +function configureMock(pr: linter.GitHubPr, prFiles?: linter.GitHubFile[]): linter.PullRequestLinter { + const pullsClient = { get(_props: { _owner: string, _repo: string, _pull_number: number }) { - return { data: issue }; + return { data: pr }; }, listFiles(_props: { _owner: string, _repo: string, _pull_number: number }) { - return { data: prFiles }; + return { data: prFiles ?? [] }; }, createReview(errorMessage: string) { @@ -324,6 +365,10 @@ function configureMock(issue: any, prFiles: any[] | undefined): linter.PullReque owner: 'aws', repo: 'aws-cdk', number: 1000, - client + + // hax hax + client: { + pulls: pullsClient as any, + } as any, }) }