chore(pr-lint): provide output to user in comments

.github/workflows/pr-linter.yml

@@ -16,7 +16,7 @@ jobs:
   validate-pr:
     permissions:
       contents: read
-      pull-requests: read
+      pull-requests: write
     runs-on: ubuntu-latest
     steps:
 

tools/@aws-cdk/prlint/action.yml

@@ -1,5 +1,5 @@
 name: Pull Request Linter
 description: Execute validation rules on GitHub Pull Requests
 runs:
-  using: node12
-  main: index.js
+  using: node16
+  main: index.js

tools/@aws-cdk/prlint/decs.d.ts

@@ -1,5 +1,3 @@
-declare module 'github-api';
-
 declare module 'conventional-commits-parser' {
   function sync(commitMsg: string): Parsed;
 

tools/@aws-cdk/prlint/index.ts

@@ -3,17 +3,21 @@ import * as github from '@actions/github';
 import * as linter from './lint';
 
 async function run() {
-    const number = github.context.issue.number;
+  const token: string = process.env.GITHUB_TOKEN ?? core.getInput('github-token', { required: true });
+  const client = github.getOctokit(token).rest.pulls;
 
-    try {
-
-        await linter.validatePr(number);
-
-    } catch (error) {
-
-        core.setFailed(error.message);
-    }
+  const prLinter = new linter.PRLinter({
+    client,
+    owner: github.context.repo.owner,
+    repo: github.context.repo.repo,
+    number: github.context.issue.number,
+  });
 
+  try {
+    await prLinter.validate()
+  } catch (error) {
+    core.setFailed(error.message);
+  }
 }
 
 run()

tools/@aws-cdk/prlint/lint.ts

@@ -1,10 +1,7 @@
 import * as path from 'path';
-import * as GitHub from 'github-api';
 import { breakingModules } from './parser';
 import { findModulePath, moduleStability } from './module';
 
-const OWNER = 'aws';
-const REPO = 'aws-cdk';
 const EXEMPT_README = 'pr-linter/exempt-readme';
 const EXEMPT_TEST = 'pr-linter/exempt-test';
 const EXEMPT_INTEG_TEST = 'pr-linter/exempt-integ-test';
@@ -16,28 +13,81 @@ class LinterError extends Error {
   }
 }
 
-function createGitHubClient() {
-  const token = process.env.GITHUB_TOKEN;
+export interface PRLinterProps {
+  readonly client: any;
+  readonly owner: string;
+  readonly repo: string;
+  readonly number: number;
+}
+
+export class PRLinter {
+  private readonly client: any;
+  private readonly prParams: { owner: string, repo: string, pull_number: number };
+
 
-  if (token) {
-    console.log("Creating authenticated GitHub Client");
-  } else {
-    console.log("Creating un-authenticated GitHub Client");
+  constructor(private readonly props: PRLinterProps) {
+    this.client = props.client;
+    this.prParams = { owner: props.owner, repo: props.repo, pull_number: props.number };
   }
 
-  return new GitHub({'token': token});
+  public async validate() {
+    const number = this.props.number;
+
+    console.log(`⌛  Fetching PR number ${number}`);
+    const pr = (await this.client.get(this.prParams)).data;
+
+    console.log(`⌛  Fetching files for PR number ${number}`);
+    const files = (await this.client.listFiles(this.prParams)).data;
+
+    console.log("⌛  Validating...");
+
+    try {
+      if (shouldExemptReadme(pr)) {
+        console.log(`Not validating README changes since the PR is labeled with '${EXEMPT_README}'`);
+      } else {
+        featureContainsReadme(pr, files);
+      }
+
+      if (shouldExemptTest(pr)) {
+        console.log(`Not validating test changes since the PR is labeled with '${EXEMPT_TEST}'`);
+      } else {
+        featureContainsTest(pr, files);
+        fixContainsTest(pr, files);
+      }
+
+      if (shouldExemptIntegTest(pr)) {
+        console.log(`Not validating integration test changes since the PR is labeled with '${EXEMPT_INTEG_TEST}'`)
+      } else {
+        featureContainsIntegTest(pr, files);
+        fixContainsIntegTest(pr, files);
+      }
+
+      validateBreakingChangeFormat(pr.title, pr.body!);
+      if (shouldExemptBreakingChange(pr)) {
+        console.log(`Not validating breaking changes since the PR is labeled with '${EXEMPT_BREAKING_CHANGE}'`);
+      } else {
+        assertStability(pr.title, pr.body!);
+      }
+
+      console.log("✅  Success");
+    } catch (error) {
+      const body = `This PR does not fulfill the following requirement: ${error.message} PRs must pass status checks before we can provide a meaningful review.`;
+      await this.client.createReview({ ...this.prParams, body, event: 'REQUEST_CHANGES' });
+      throw error;
+    }
+  }
 }
 
-function isPkgCfnspec(issue: any) {
-  return issue.title.indexOf("(cfnspec)") > -1;
+function isPkgCfnspec(pr: any) {
+  return pr.title.indexOf("(cfnspec)") > -1;
 }
 
-function isFeature(issue: any) {
-  return issue.title.startsWith("feat")
+function isFeature(pr: any) {
+  return pr.title.startsWith("feat")
 }
 
-function isFix(issue: any) {
-  return issue.title.startsWith("fix")
+function isFix(pr: any) {
+  return pr.title.startsWith("fix")
 }
 
 function testChanged(files: any[]) {
@@ -48,52 +98,62 @@ function integTestChanged(files: any[]) {
   return files.filter(f => f.filename.toLowerCase().match(/integ.*.ts$/)).length != 0;
 }
 
+function integTestSnapshotChanged(files: any[]) {
+  return files.filter(f => f.filename.toLowerCase().includes("integ.snapshot")).length != 0;
+}
+
 function readmeChanged(files: any[]) {
   return files.filter(f => path.basename(f.filename) == "README.md").length != 0;
 }
 
-function featureContainsReadme(issue: any, files: any[]) {
-  if (isFeature(issue) && !readmeChanged(files) && !isPkgCfnspec(issue)) {
-    throw new LinterError("Features must contain a change to a README file");
+function featureContainsReadme(pr: any, files: any[]) {
+  if (isFeature(pr) && !readmeChanged(files) && !isPkgCfnspec(pr)) {
+    throw new LinterError("Features must contain a change to a README file.");
   }
 };
 
-function featureContainsTest(issue: any, files: any[]) {
-  if (isFeature(issue) && !testChanged(files)) {
-    throw new LinterError("Features must contain a change to a test file");
+function featureContainsTest(pr: any, files: any[]) {
+  if (isFeature(pr) && !testChanged(files)) {
+    throw new LinterError("Features must contain a change to a test file.");
   }
 };
 
-function fixContainsTest(issue: any, files: any[]) {
-  if (isFix(issue) && !testChanged(files)) {
-    throw new LinterError("Fixes must contain a change to a test file");
+function fixContainsTest(pr: any, files: any[]) {
+  if (isFix(pr) && !testChanged(files)) {
+    throw new LinterError("Fixes must contain a change to a test file.");
   }
 };
 
-function featureContainsIntegTest(issue: any, files: any[]) {
-  if (isFeature(issue) && !integTestChanged(files)) {
-    throw new LinterError("Features must contain a change to an integration test file");
+function featureContainsIntegTest(pr: any, files: any[]) {
+  if (isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files))) {
+    throw new LinterError("Features must contain a change to an integration test file.");
   }
 };
 
-function shouldExemptReadme(issue: any) {
-  return hasLabel(issue, EXEMPT_README);
+function fixContainsIntegTest(pr: any, files: any[]) {
+  if (isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files))) {
+    throw new LinterError("Fixes must contain a change to an integration test file.");
+  }
 }
 
-function shouldExemptTest(issue: any) {
-  return hasLabel(issue, EXEMPT_TEST);
+function shouldExemptReadme(pr: any) {
+  return hasLabel(pr, EXEMPT_README);
 }
 
-function shouldExemptIntegTest(issue: any) {
-  return hasLabel(issue, EXEMPT_INTEG_TEST);
+function shouldExemptTest(pr: any) {
+  return hasLabel(pr, EXEMPT_TEST);
 }
 
-function shouldExemptBreakingChange(issue: any) {
-  return hasLabel(issue, EXEMPT_BREAKING_CHANGE);
+function shouldExemptIntegTest(pr: any) {
+  return hasLabel(pr, EXEMPT_INTEG_TEST);
 }
 
-function hasLabel(issue: any, labelName: string) {
-  return issue.labels.some(function (l: any) {
+function shouldExemptBreakingChange(pr: any) {
+  return hasLabel(pr, EXEMPT_BREAKING_CHANGE);
+}
+
+function hasLabel(pr: any, labelName: string) {
+  return pr.labels.some(function (l: any) {
     return l.name === labelName;
   })
 }
@@ -110,14 +170,14 @@ function validateBreakingChangeFormat(title: string, body: string) {
   const m = re.exec(body);
   if (m) {
     if (!m[0].startsWith('BREAKING CHANGE: ')) {
-      throw new LinterError(`Breaking changes should be indicated by starting a line with 'BREAKING CHANGE: ', variations are not allowed. (found: '${m[0]}')`);
+      throw new LinterError(`Breaking changes should be indicated by starting a line with 'BREAKING CHANGE: ', variations are not allowed. (found: '${m[0]}').`);
     }
     if (m[0].slice('BREAKING CHANGE:'.length).trim().length === 0) {
-      throw new LinterError("The description of the first breaking change should immediately follow the 'BREAKING CHANGE: ' clause");
+      throw new LinterError("The description of the first breaking change should immediately follow the 'BREAKING CHANGE: ' clause.");
     }
     const titleRe = /^[a-z]+\([0-9a-z-_]+\)/;
     if (!titleRe.exec(title)) {
-      throw new LinterError("The title of this PR must specify the module name that the first breaking change should be associated to");
+      throw new LinterError("The title of this PR must specify the module name that the first breaking change should be associated to.");
     }
   }
 }
@@ -131,54 +191,6 @@ function assertStability(title: string, body: string) {
   }
 }
 
-export async function validatePr(number: number) {
-
-  if (!number) {
-    throw new Error('Must provide a PR number');
-  }
-
-  const gh = createGitHubClient();
-
-  const issues = gh.getIssues(OWNER, REPO);
-  const repo = gh.getRepo(OWNER, REPO);
-
-  console.log(`⌛  Fetching PR number ${number}`);
-  const issue = (await issues.getIssue(number)).data;
-
-  console.log(`⌛  Fetching files for PR number ${number}`);
-  const files = (await repo.listPullRequestFiles(number)).data;
-
-  console.log("⌛  Validating...");
-
-  if (shouldExemptReadme(issue)) {
-    console.log(`Not validating README changes since the PR is labeled with '${EXEMPT_README}'`);
-  } else {
-    featureContainsReadme(issue, files);
-  }
-
-  if (shouldExemptTest(issue)) {
-    console.log(`Not validating test changes since the PR is labeled with '${EXEMPT_TEST}'`);
-  } else {
-    featureContainsTest(issue, files);
-    fixContainsTest(issue, files);
-  }
-
-  if (shouldExemptIntegTest(issue)) {
-    console.log(`Not validating integration test changes since the PR is labeled with '${EXEMPT_INTEG_TEST}'`)
-  } else {
-    featureContainsIntegTest(issue, files);
-  }
-
-  validateBreakingChangeFormat(issue.title, issue.body);
-  if (shouldExemptBreakingChange(issue)) {
-    console.log(`Not validating breaking changes since the PR is labeled with '${EXEMPT_BREAKING_CHANGE}'`);
-  } else {
-    assertStability(issue.title, issue.body);
-  }
-
-  console.log("✅  Success");
-}
-
 require('make-runnable/custom')({
   printOutputFrame: false
 })

tools/@aws-cdk/prlint/package.json

@@ -12,10 +12,9 @@
   "license": "Apache-2.0",
   "dependencies": {
     "@actions/core": "^1.9.1",
-    "@actions/github": "^2.2.0",
+    "@actions/github": "^5.0.3",
     "conventional-commits-parser": "^3.2.4",
     "fs-extra": "^9.1.0",
-    "github-api": "^3.4.0",
     "glob": "^7.2.3"
   },
   "devDependencies": {