From 4ad45676f7a2f6c15070969451e5705fdacbe553 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 22 Sep 2022 13:39:57 +0200 Subject: [PATCH] chore(linter): require a test label for CLI changes (#22187) Require a label a human must add for PRs that change CLI code. The linter will give instructions on how to push the code to the right pipeline branch. Also add strong typing to the linter, there were a couple of bugs in there because of the `any`s. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- tools/@aws-cdk/prlint/index.ts | 3 +- tools/@aws-cdk/prlint/lint.ts | 119 ++++++++++++++++-------- tools/@aws-cdk/prlint/test/lint.test.ts | 61 ++++++++++-- 3 files changed, 136 insertions(+), 47 deletions(-) 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, }) }