From a36f22511d35f2609ea41bc7d5417bc80162095c Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 8 Aug 2023 12:27:58 +0100 Subject: [PATCH] Added status checks (#37) Added ability to generate a status check and the ability to overwrite the latest one. This resolves #33 and closes #26 as it merged it into one ticket. There are some limitations as we can not select the check suite that the check will belong (see https://github.com/paritytech/review-bot/issues/33#issuecomment-1662034503), but the name will be constant, making it validable. --- .github/workflows/review-bot.yml | 1 + src/github/pullRequest.ts | 53 ++++++++++++++++++- src/github/types.ts | 9 ++++ src/index.ts | 14 ++--- src/runner.ts | 91 +++++++++++++++++++++++++------- 5 files changed, 141 insertions(+), 27 deletions(-) diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 66c9d56..aece23f 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -12,6 +12,7 @@ on: permissions: contents: read + checks: write jobs: review-approvals: diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 0a52700..7a23362 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -1,7 +1,8 @@ +import { summary } from "@actions/core"; import { PullRequest, PullRequestReview } from "@octokit/webhooks-types"; import { caseInsensitiveEqual } from "../util"; -import { ActionLogger, GitHubClient } from "./types"; +import { ActionLogger, CheckData, GitHubClient } from "./types"; /** API class that uses the default token to access the data from the pull request and the repository */ export class PullRequestApi { @@ -11,6 +12,7 @@ export class PullRequestApi { private readonly pr: PullRequest, private readonly logger: ActionLogger, private readonly repoInfo: { repo: string; owner: string }, + private readonly detailsUrl: string, ) { this.number = pr.number; } @@ -102,4 +104,53 @@ 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 + .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 ?? "") + .addRaw(checkResult.output.text) + .write(); + + this.logger.debug(JSON.stringify(check.data)); + } } diff --git a/src/github/types.ts b/src/github/types.ts index dbef8a5..50afb98 100644 --- a/src/github/types.ts +++ b/src/github/types.ts @@ -8,3 +8,12 @@ export interface ActionLogger { } export type GitHubClient = InstanceType; + +export interface CheckData { + conclusion: "action_required" | "failure" | "success"; + output: { + title: string; + summary: string; + text: string; + }; +} diff --git a/src/index.ts b/src/index.ts index e40cfd3..627ecc0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -50,11 +50,14 @@ debug("Got payload:" + JSON.stringify(context.payload.pull_request)); 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(), repo, + actionId, ); const logger = generateCoreLogger(); @@ -66,10 +69,9 @@ const runner = new ActionRunner(api, teamApi, logger); runner .runAction(inputs) .then((result) => { - if (result) { - info("Action completed succesfully"); - } else { - setFailed("Action failed"); - } + info(`Action run without problem. Evaluation result was '${result.conclusion}'`); }) - .catch(setFailed); + .catch((error) => { + console.error(error); + setFailed(error as Error | string); + }); diff --git a/src/runner.ts b/src/runner.ts index 7e97f60..1d649e6 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,3 +1,4 @@ +import { summary } from "@actions/core"; import { parse } from "yaml"; import { Inputs } from "."; @@ -5,7 +6,7 @@ import { ConfigurationFile, Reviewers, Rule } from "./file/types"; import { validateConfig, validateRegularExpressions } from "./file/validator"; import { PullRequestApi } from "./github/pullRequest"; import { TeamApi } from "./github/teams"; -import { ActionLogger } from "./github/types"; +import { ActionLogger, CheckData } from "./github/types"; import { concatArraysUniquely } from "./util"; type ReviewReport = { @@ -18,6 +19,9 @@ type ReviewReport = { /** If applicable, the users that should be requested to review */ usersToRequest?: string[]; }; + +type RuleReport = { name: string } & ReviewReport; + type ReviewState = [true] | [false, ReviewReport]; /** Action in charge of running the GitHub action */ @@ -51,10 +55,10 @@ export class ActionRunner { /** * The action evaluates if the rules requirements are meet for a PR - * @returns a true/false statement if the rule failed. This WILL BE CHANGED for an object with information (see issue #26) + * @returns an array of error reports for each failed rule. An empty array means no errors */ - async validatePullRequest({ rules }: ConfigurationFile): Promise { - const errorReports: ReviewReport[] = []; + async validatePullRequest({ rules }: ConfigurationFile): Promise { + const errorReports: RuleReport[] = []; for (const rule of rules) { try { this.logger.info(`Validating rule '${rule.name}'`); @@ -70,7 +74,7 @@ export class ActionRunner { const [result, missingData] = await this.evaluateCondition(rule); if (!result) { this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - errorReports.push(missingData); + errorReports.push({ ...missingData, name: rule.name }); } } } catch (error: unknown) { @@ -80,18 +84,14 @@ export class ActionRunner { } this.logger.info(`Finish validating '${rule.name}'`); } - if (errorReports.length > 0) { - const finalReport = this.aggregateReports(errorReports); - // Preview, this will be improved in a future commit - this.logger.warn(`Missing reviews: ${JSON.stringify(finalReport)}`); - return false; - } - // TODO: Convert this into a list of users/teams missing and convert the output into a nice summary object -> Issue #26 - return true; + return errorReports; } - /** Aggregates all the reports and generate a final combined one */ - aggregateReports(reports: ReviewReport[]): ReviewReport { + /** WIP - Class that will assign the requests for review */ + requestReviewers(reports: RuleReport[]): void { + if (reports.length === 0) { + return; + } const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [] }; for (const report of reports) { @@ -101,7 +101,47 @@ export class ActionRunner { finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest); } - return finalReport; + const { teamsToRequest, usersToRequest } = finalReport; + const reviewersLog = [ + teamsToRequest ? `Teams: ${JSON.stringify(teamsToRequest)} - ` : "", + usersToRequest ? `Users: ${JSON.stringify(usersToRequest)}` : "", + ].join(" - "); + + this.logger.info(`Need to request reviews from ${reviewersLog}`); + } + + /** Aggregates all the reports and generate a status report */ + generateCheckRunData(reports: RuleReport[]): CheckData { + // Count how many reviews are missing + const missingReviews = reports.reduce((a, b) => a + b.missingReviews, 0); + const failed = missingReviews > 0; + const check: CheckData = { + conclusion: failed ? "failure" : "success", + output: { + title: failed ? `Missing ${missingReviews} reviews` : "All required reviews fulfilled", + summary: failed ? "# The following rules have failed:\n" : "All neccesary users have reviewed the PR", + text: failed ? "Details per rule:\n" : "", + }, + }; + + if (!failed) { + return check; + } + + for (const report of reports) { + check.output.summary += `- **${report.name}**\n`; + let text = summary.addHeading(report.name, 2).addHeading(`Missing ${report.missingReviews} reviews`, 4); + if (report.usersToRequest && report.usersToRequest.length > 0) { + text = text.addHeading("Missing users", 3).addList(report.usersToRequest); + } + if (report.teamsToRequest && report.teamsToRequest.length > 0) { + text = text.addHeading("Missing reviews from teams", 3).addList(report.teamsToRequest); + } + + check.output.text += text.stringify() + "\n"; + } + + return check; } /** Evaluates if the required reviews for a condition have been meet @@ -201,13 +241,24 @@ export class ActionRunner { return matches; } - async runAction(inputs: Omit): Promise { + /** Core runner of the app. + * 1. It fetches the config + * 2. It validates all the pull request requirements based on the config file + * 3. It generates a status check in the Pull Request + * 4. WIP - It assigns the required reviewers to review the PR + */ + async runAction(inputs: Omit): Promise { const config = await this.getConfigFile(inputs.configLocation); - const success = await this.validatePullRequest(config); + const reports = await this.validatePullRequest(config); + + 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); - this.logger.info(success ? "The PR has been successful" : "There was an error with the PR reviews."); + this.requestReviewers(reports); - return success; + return checkRunData; } }