Skip to content

Commit

Permalink
chore(linter): require a test label for CLI changes (#22187)
Browse files Browse the repository at this point in the history 
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*
  • Loading branch information
@rix0rrr
rix0rrr authored Sep 22, 2022
1 parent dd6ead4 commit 4ad4567
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 47 deletions.
3 changes: 2 additions & 1 deletion tools/@aws-cdk/prlint/index.ts
View file
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
119 changes: 81 additions & 38 deletions tools/@aws-cdk/prlint/lint.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as path from 'path';
import { Octokit } from '@octokit/rest';
import { breakingModules } from './parser';
import { findModulePath, moduleStability } from './module';

Expand All @@ -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 {
Expand All @@ -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[] = [];

/**
Expand All @@ -44,7 +70,7 @@ class TestResult {
* Represents a single test.
*/
interface Test {
test: (pr: any, files: any[]) => TestResult;
test: (pr: GitHubPr, files: GitHubFile[]) => TestResult;
}

/**
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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 };


Expand All @@ -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<void> {
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.',
Expand All @@ -161,7 +187,7 @@ export class PullRequestLinter {
*/
private async communicateResult(failureReasons: string[]): Promise<void> {
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);
}

Expand All @@ -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...");

Expand Down Expand Up @@ -214,92 +240,99 @@ 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);
}

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;
})
Expand All @@ -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.`);
Expand All @@ -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
});
Loading

0 comments on commit 4ad4567

Please sign in to comment.