diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 736bf72..8c09c37 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -22,8 +22,8 @@ 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" @@ -31,3 +31,4 @@ jobs: 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 index a529e20..94f71f2 100644 --- 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 index aa7712a..13fda73 100644 --- 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 new file mode 100644 index 0000000..73bd32e --- /dev/null +++ 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 { + 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 index 315929b..6f22493 100644 --- 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 { - 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 index d5a1d1c..de829a4 100644 --- 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 index 0b91fae..6dbc81a 100644 --- 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 index 89ea481..aad3f2f 100644 --- 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; let runner: ActionRunner; let teamsApi: MockProxy; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); 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 index a3bb10b..1f73d85 100644 --- 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; let runner: ActionRunner; let teamsApi: MockProxy; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); 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 index d46462e..7f586fc 100644 --- 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; let runner: ActionRunner; let teamsApi: MockProxy; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); 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 index 208fda3..d3e50fd 100644 --- 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; let teamsApi: MockProxy; + let logger: MockProxy; let runner: ActionRunner; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); + logger = mock(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), 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`), + ); }); }); diff --git a/src/test/rules/or.test.ts b/src/test/rules/or.test.ts index 7d37d0e..f39155f 100644 --- a/src/test/rules/or.test.ts +++ b/src/test/rules/or.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 { OrRule } from "../../rules/types"; import { ActionRunner } from "../../runner"; -import { TestLogger } from "../logger"; describe("'Or' rule parsing", () => { let api: MockProxy; let runner: ActionRunner; let teamsApi: MockProxy; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index a174882..4b27284 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -1,20 +1,19 @@ 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 { ActionRunner } from "../../runner"; -import { TestLogger } from "../logger"; describe("evaluateCondition tests", () => { let api: MockProxy; let teamsApi: MockProxy; let runner: ActionRunner; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); teamsApi = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); test("should throw if no teams or users were set", async () => { diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 96dd374..b846f20 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -1,20 +1,19 @@ 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 { ConfigurationFile, Rule, RuleTypes } from "../../rules/types"; import { ActionRunner } from "../../runner"; -import { TestLogger } from "../logger"; describe("Shared validations", () => { let api: MockProxy; let teamsApi: MockProxy; let runner: ActionRunner; - let logger: TestLogger; beforeEach(() => { - logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); test("validatePullRequest should return true if no rule matches any files", async () => { diff --git a/src/test/runner/validation/and.test.ts b/src/test/runner/validation/and.test.ts index 1a08c74..af1aa79 100644 --- a/src/test/runner/validation/and.test.ts +++ b/src/test/runner/validation/and.test.ts @@ -1,25 +1,24 @@ 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 { ConfigurationFile, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; -import { TestLogger } from "../../logger"; describe("'And' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; let runner: ActionRunner; - let logger: TestLogger; const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { - logger = new TestLogger(); api = mock(); teamsApi = mock(); teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); describe("approvals", () => { diff --git a/src/test/runner/validation/andDistinct.test.ts b/src/test/runner/validation/andDistinct.test.ts index 334b608..db439b4 100644 --- a/src/test/runner/validation/andDistinct.test.ts +++ b/src/test/runner/validation/andDistinct.test.ts @@ -1,6 +1,7 @@ /* eslint-disable @typescript-eslint/unbound-method */ 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"; @@ -20,7 +21,7 @@ describe("'And distinct' rule validation", () => { teamsApi.getTeamMembers.calledWith("team-abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), logger); }); describe("Fail scenarios", () => { diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts index e934981..74fcaff 100644 --- a/src/test/runner/validation/or.test.ts +++ b/src/test/runner/validation/or.test.ts @@ -1,25 +1,24 @@ 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 { ConfigurationFile, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; -import { TestLogger } from "../../logger"; describe("'Or' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; let runner: ActionRunner; - let logger: TestLogger; const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { - logger = new TestLogger(); api = mock(); teamsApi = mock(); teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); api.listApprovedReviewsAuthors.mockResolvedValue([]); - runner = new ActionRunner(api, teamsApi, logger); + runner = new ActionRunner(api, teamsApi, mock(), mock()); }); describe("approvals", () => {