Status check fix (paritytech#63)

Fixes paritytech#54 Abstracted the code to generate check runs to its own class. In theory, it can share the same key as the `Teams` class, but we want to keep it separated so the `Team` class can be hot swapped for a chain data class.
paritytech-stg · Aug 30, 2023 · 665fa80 · 665fa80
1 parent ecb7c1e
commit 665fa80
Show file tree

Hide file tree

Showing 17 changed files with 135 additions and 101 deletions.
diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml
@@ -22,12 +22,13 @@ jobs:
         id: team_token
         uses: tibdex/github-app-token@v1
         with:
-          app_id: ${{ secrets.TEAM_APP_ID }}
-          private_key: ${{ secrets.TEAM_APP_KEY }}
+          app_id: ${{ secrets.REVIEW_APP_ID }}
+          private_key: ${{ secrets.REVIEW_APP_KEY }}
       # !This must always point to main. 
       # Change it for the PRs but remember to change it back
       - name: "Evaluates PR reviews and assigns reviewers"
         uses: paritytech/review-bot@main
         with:
           repo-token: ${{ secrets.GITHUB_TOKEN }}
           team-token: ${{ steps.team_token.outputs.token }}
+          checks-token: ${{ steps.team_token.outputs.token }}
diff --git a/README.md b/README.md
@@ -66,6 +66,7 @@ jobs:
         with:
           repo-token: ${{ github.token }}
           team-token: ${{ secrets.TEAM_TOKEN }}
+          checks-token: ${{ secrets.CHECKS_TOKEN }}
 
 ```
 Create a new PR and see if it is working.
@@ -99,6 +100,13 @@ You can find all the inputs in [the action file](./action.yml), but let's walk t
 	- **required**.
 	- This needs to be a [GitHub Personal Access](https://github.com/settings/tokens/new) token with `read:org` permission.
 	- It is used to extract the members of teams.
+- `checks-token`: Token to write the status checks.
+	- **required**.
+	- This needs to be a [GitHub Personal Access](https://github.com/settings/tokens/new) token with `checks:write` permission.
+	- It is used to write the status checks of successful/failed runs.
+	- Can be `${{ github.token }}` but there is a [known bug](https://github.com/paritytech/review-bot/issues/54).
+		- If you use a GitHub app, this bug will be fixed.
+			- You can use the same GitHub app for `checks-token` and `team-token`.
 - `config-file`: The location of the config file.
 	- **default**: `.github/review-bot.yml`
 
@@ -107,6 +115,9 @@ In some cases, specially in big organizations, it is more organized to use a Git
 - Organization permissions:
 	- Members
 		- [x] Read
+- Repository permissions:
+	- Checks
+		- [x] Write
 
 Because this project is intended to be used with a token, we need to do an extra step to generate one from the GitHub app:
 - After you create the app, copy the *App ID* and the *private key* and set them as secrets.
@@ -125,6 +136,7 @@ Because this project is intended to be used with a token, we need to do an extra
           repo-token: ${{ github.token }}
           # The previous step generates a token which is used as the input for this action
           team-token: ${{ steps.generate_token.outputs.token }
+          checks-token: ${{ steps.generate_token.outputs.token }
 ```
 ### Outputs
 Outputs are needed for your chained actions. If you want to use this information, remember to set an `id` field in the step, so you can access it.

diff --git a/action.yml b/action.yml
@@ -11,6 +11,9 @@ inputs:
   team-token:
     required: true
     description: A GitHub Token with read:org access
+  checks-token:
+    required: true
+    description: A GitHub Token with check:write access
   config-file:
     description: 'Location of the configuration file'
     required: false

diff --git a/src/github/check.ts b/src/github/check.ts
@@ -0,0 +1,72 @@
+import { summary } from "@actions/core";
+import { PullRequest } from "@octokit/webhooks-types";
+
+import { ActionLogger, CheckData, GitHubClient } from "./types";
+
+/** GitHub client with access to Checks:Write
+ * Ideally, a GitHub action.
+ * This is the solution to the https://github.com/paritytech/review-bot/issues/54
+ */
+export class GitHubChecksApi {
+  private readonly repoInfo: { repo: string; owner: string };
+  constructor(
+    private readonly api: GitHubClient,
+    private readonly pr: PullRequest,
+    private readonly logger: ActionLogger,
+    private readonly detailsUrl: string,
+  ) {
+    this.repoInfo = { owner: this.pr.base.repo.owner.login, repo: this.pr.base.repo.name };
+  }
+
+  /**
+   * Generates a Check Run or modifies the existing one.
+   * This way we can aggregate all the results from different causes into a single one
+   * {@link https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28}
+   * @param checkResult a CheckData object with the final conclussion of action and the output text
+   * {@link CheckData}
+   */
+  async generateCheckRun(checkResult: CheckData): Promise<void> {
+    const checkData = {
+      ...checkResult,
+      owner: this.repoInfo.owner,
+      repo: this.repoInfo.repo,
+      external_id: "review-bot",
+      head_sha: this.pr.head.sha,
+      name: "review-bot",
+      details_url: this.detailsUrl,
+    };
+
+    const { data } = await this.api.rest.checks.listForRef({
+      owner: this.repoInfo.owner,
+      repo: this.repoInfo.repo,
+      ref: this.pr.head.sha,
+    });
+
+    this.logger.debug(`Searching for a match for id ${checkData.external_id}. Found ${data.total_count} checks`);
+
+    for (const check of data.check_runs) {
+      if (check.external_id === checkData.external_id) {
+        this.logger.debug(`Found match: ${JSON.stringify(check)}`);
+        await this.api.rest.checks.update({ ...checkData, check_run_id: check.id });
+        this.logger.debug("Updated check data");
+        return;
+      }
+    }
+
+    this.logger.debug("Did not find any matching status check. Creating a new one");
+
+    const check = await this.api.rest.checks.create(checkData);
+
+    // We publish it in the action summary
+    await summary
+      .emptyBuffer()
+      .addHeading(checkResult.output.title)
+      // We redirect to the check as it can changed if it is triggered again
+      .addLink("Find the result here", check.data.html_url ?? "")
+      .addBreak()
+      .addRaw(checkResult.output.text)
+      .write();
+
+    this.logger.debug(JSON.stringify(check.data));
+  }
+}
diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts
@@ -1,8 +1,7 @@
-import { summary } from "@actions/core";
 import { PullRequest, PullRequestReview } from "@octokit/webhooks-types";
 
 import { caseInsensitiveEqual } from "../util";
-import { ActionLogger, CheckData, GitHubClient } from "./types";
+import { ActionLogger, GitHubClient } from "./types";
 
 /** API class that uses the default token to access the data from the pull request and the repository */
 export class PullRequestApi {
@@ -111,55 +110,4 @@ export class PullRequestApi {
   getAuthor(): string {
     return this.pr.user.login;
   }
-
-  /**
-   * Generates a Check Run or modifies the existing one.
-   * This way we can aggregate all the results from different causes into a single one
-   * {@link https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28}
-   * @param checkResult a CheckData object with the final conclussion of action and the output text
-   * {@link CheckData}
-   */
-  async generateCheckRun(checkResult: CheckData): Promise<void> {
-    const checkData = {
-      ...checkResult,
-      owner: this.repoInfo.owner,
-      repo: this.repoInfo.repo,
-      external_id: "review-bot",
-      head_sha: this.pr.head.sha,
-      name: "review-bot",
-    };
-
-    const { data } = await this.api.rest.checks.listForRef({
-      owner: this.repoInfo.owner,
-      repo: this.repoInfo.repo,
-      ref: this.pr.head.sha,
-    });
-
-    this.logger.debug(`Searching for a match for id ${checkData.external_id}. Found ${data.total_count} checks`);
-
-    for (const check of data.check_runs) {
-      if (check.external_id === checkData.external_id) {
-        this.logger.debug(`Found match: ${JSON.stringify(check)}`);
-        await this.api.rest.checks.update({ ...checkData, check_run_id: check.id });
-        this.logger.debug("Updated check data");
-        return;
-      }
-    }
-
-    this.logger.debug("Did not find any matching status check. Creating a new one");
-
-    const check = await this.api.rest.checks.create(checkData);
-
-    // We publish it in the action summary
-    await summary
-      .emptyBuffer()
-      .addHeading(checkResult.output.title)
-      // We redirect to the check as it can changed if it is triggered again
-      .addLink("Find the result here", check.data.html_url ?? "")
-      .addBreak()
-      .addRaw(checkResult.output.text)
-      .write();
-
-    this.logger.debug(JSON.stringify(check.data));
-  }
 }
diff --git a/src/index.ts b/src/index.ts
@@ -3,6 +3,7 @@ import { context, getOctokit } from "@actions/github";
 import { Context } from "@actions/github/lib/context";
 import { PullRequest } from "@octokit/webhooks-types";
 
+import { GitHubChecksApi } from "./github/check";
 import { PullRequestApi } from "./github/pullRequest";
 import { GitHubTeamsApi } from "./github/teams";
 import { ActionRunner } from "./runner";
@@ -52,18 +53,17 @@ const inputs = getInputs();
 
 const actionId = `${context.serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${context.runId}`;
 
-const api = new PullRequestApi(
-  getOctokit(inputs.repoToken),
-  context.payload.pull_request as PullRequest,
-  generateCoreLogger(),
-  actionId,
-);
+const pr = context.payload.pull_request as PullRequest;
+
+const api = new PullRequestApi(getOctokit(inputs.repoToken), pr, generateCoreLogger(), actionId);
 
 const logger = generateCoreLogger();
 
 const teamApi = new GitHubTeamsApi(getOctokit(inputs.teamApiToken), repo.owner, logger);
 
-const runner = new ActionRunner(api, teamApi, logger);
+const checks = new GitHubChecksApi(getOctokit(inputs.teamApiToken), pr, logger, actionId);
+
+const runner = new ActionRunner(api, teamApi, checks, logger);
 
 runner
   .runAction(inputs)

diff --git a/src/runner.ts b/src/runner.ts
@@ -2,6 +2,7 @@ import { setOutput, summary } from "@actions/core";
 import { parse } from "yaml";
 
 import { Inputs } from ".";
+import { GitHubChecksApi } from "./github/check";
 import { PullRequestApi } from "./github/pullRequest";
 import { TeamApi } from "./github/teams";
 import { ActionLogger, CheckData } from "./github/types";
@@ -36,6 +37,7 @@ export class ActionRunner {
   constructor(
     private readonly prApi: PullRequestApi,
     private readonly teamApi: TeamApi,
+    private readonly checks: GitHubChecksApi,
     private readonly logger: ActionLogger,
   ) {}
 
@@ -444,7 +446,7 @@ export class ActionRunner {
     this.logger.info(reports.length > 0 ? "There was an error with the PR reviews." : "The PR has been successful");
 
     const checkRunData = this.generateCheckRunData(reports);
-    await this.prApi.generateCheckRun(checkRunData);
+    await this.checks.generateCheckRun(checkRunData);
 
     this.requestReviewers(reports);
 

diff --git a/src/test/rules/andDistinct.test.ts b/src/test/rules/andDistinct.test.ts
@@ -3,21 +3,20 @@
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 import { mock, MockProxy } from "jest-mock-extended";
 
+import { GitHubChecksApi } from "../../github/check";
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { ActionLogger } from "../../github/types";
 import { AndDistinctRule } from "../../rules/types";
 import { ActionRunner } from "../../runner";
-import { TestLogger } from "../logger";
 
 describe("'AndDistinctRule' rule parsing", () => {
   let api: MockProxy<PullRequestApi>;
   let runner: ActionRunner;
   let teamsApi: MockProxy<TeamApi>;
-  let logger: TestLogger;
   beforeEach(() => {
-    logger = new TestLogger();
     api = mock<PullRequestApi>();
-    runner = new ActionRunner(api, teamsApi, logger);
+    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
   });
   test("should get minimal config", async () => {
     api.getConfigFile.mockResolvedValue(`

diff --git a/src/test/rules/andRule.test.ts b/src/test/rules/andRule.test.ts
@@ -3,21 +3,20 @@
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 import { mock, MockProxy } from "jest-mock-extended";
 
+import { GitHubChecksApi } from "../../github/check";
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { ActionLogger } from "../../github/types";
 import { AndRule } from "../../rules/types";
 import { ActionRunner } from "../../runner";
-import { TestLogger } from "../logger";
 
 describe("'And' rule parsing", () => {
   let api: MockProxy<PullRequestApi>;
   let runner: ActionRunner;
   let teamsApi: MockProxy<TeamApi>;
-  let logger: TestLogger;
   beforeEach(() => {
-    logger = new TestLogger();
     api = mock<PullRequestApi>();
-    runner = new ActionRunner(api, teamsApi, logger);
+    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
   });
   test("should get minimal config", async () => {
     api.getConfigFile.mockResolvedValue(`

diff --git a/src/test/rules/basicRule.test.ts b/src/test/rules/basicRule.test.ts
@@ -3,21 +3,20 @@
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 import { mock, MockProxy } from "jest-mock-extended";
 
+import { GitHubChecksApi } from "../../github/check";
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { ActionLogger } from "../../github/types";
 import { BasicRule } from "../../rules/types";
 import { ActionRunner } from "../../runner";
-import { TestLogger } from "../logger";
 
 describe("Basic rule parsing", () => {
   let api: MockProxy<PullRequestApi>;
   let runner: ActionRunner;
   let teamsApi: MockProxy<TeamApi>;
-  let logger: TestLogger;
   beforeEach(() => {
-    logger = new TestLogger();
     api = mock<PullRequestApi>();
-    runner = new ActionRunner(api, teamsApi, logger);
+    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
   });
   test("should get minimal config", async () => {
     api.getConfigFile.mockResolvedValue(`

diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts
@@ -3,21 +3,22 @@
 /* eslint-disable @typescript-eslint/no-unsafe-call */
 import { mock, MockProxy } from "jest-mock-extended";
 
+import { GitHubChecksApi } from "../../github/check";
 import { PullRequestApi } from "../../github/pullRequest";
 import { TeamApi } from "../../github/teams";
+import { ActionLogger } from "../../github/types";
 import { RuleTypes } from "../../rules/types";
 import { ActionRunner } from "../../runner";
-import { TestLogger } from "../logger";
 
 describe("Config Parsing", () => {
   let api: MockProxy<PullRequestApi>;
   let teamsApi: MockProxy<TeamApi>;
+  let logger: MockProxy<ActionLogger>;
   let runner: ActionRunner;
-  let logger: TestLogger;
   beforeEach(() => {
-    logger = new TestLogger();
+    logger = mock<ActionLogger>();
     api = mock<PullRequestApi>();
-    runner = new ActionRunner(api, teamsApi, logger);
+    runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), logger);
   });
   test("should get minimal config", async () => {
     api.getConfigFile.mockResolvedValue(`
@@ -117,7 +118,9 @@ describe("Config Parsing", () => {
       await expect(runner.getConfigFile("")).rejects.toThrowError(
         `Regular expression is invalid: Include condition '${invalidRegex}' is not a valid regex`,
       );
-      expect(logger.logHistory).toContainEqual(`Invalid regular expression: /${invalidRegex}/: Invalid group`);
+      expect(logger.error).toHaveBeenCalledWith(
+        new Error(`Invalid regular expression: /${invalidRegex}/: Invalid group`),
+      );
     });
   });