diff --git a/action.yml b/action.yml index bf1301c..cadbd84 100644 --- a/action.yml +++ b/action.yml @@ -8,6 +8,9 @@ inputs: repo-token: required: true description: The token to access the repo and the pull request data + team-token: + required: true + description: A GitHub Token with read:org access config-file: description: 'Location of the configuration file' required: false diff --git a/package.json b/package.json index 5f89ca7..4978293 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,9 @@ "fix": "eslint --fix '{src,test}/**/*'", "lint": "eslint '{src,test}/**/*'" }, + "engines": { + "node": ">=18.0.0" + }, "repository": { "type": "git", "url": "git+/~https://github.com/paritytech/review-bot.git" diff --git a/src/file/types.ts b/src/file/types.ts index 4c228db..a4290a1 100644 --- a/src/file/types.ts +++ b/src/file/types.ts @@ -1,9 +1,9 @@ -enum Rules { +export enum RuleTypes { Basic = "basic", Debug = "debug", } -type Reviewers = { users?: string[]; teams?: string[] }; +export type Reviewers = { users?: string[]; teams?: string[] }; export interface Rule { name: string; @@ -12,12 +12,12 @@ export interface Rule { // TODO: Delete this once we add a second type of rule export interface DebugRule extends Rule { - type: Rules.Debug; + type: RuleTypes.Debug; size: number; } export interface BasicRule extends Rule, Reviewers { - type: Rules.Basic; + type: RuleTypes.Basic; min_approvals: number; } @@ -26,8 +26,8 @@ export interface ConfigurationFile { * @see {@link Rules} */ rules: (BasicRule | DebugRule)[]; - preventReviewRequests: { + preventReviewRequests?: { teams?: string[]; - users: string[]; + users?: string[]; }; } diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 390d20b..02973b9 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -1,14 +1,23 @@ -import { PullRequest } from "@octokit/webhooks-types"; +import { PullRequest, PullRequestReview } from "@octokit/webhooks-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 { + private readonly number: number; constructor( private readonly api: GitHubClient, private readonly pr: PullRequest, private readonly logger: ActionLogger, - ) {} + private readonly repoInfo: { repo: string; owner: string }, + ) { + this.number = pr.number; + } + + /** Cache of the list of files that have been modified by a PR */ + private filesChanged: string[] = []; + /** Cache for the list of logins that have approved the PR */ + private usersThatApprovedThePr: string[] | null = null; async getConfigFile(configFilePath: string): Promise { const { data } = await this.api.rest.repos.getContent({ @@ -29,4 +38,24 @@ export class PullRequestApi { return decryptedFile; } + + /** Returns an array with all the files that had been modified */ + async listModifiedFiles(): Promise { + if (this.filesChanged.length === 0) { + const { data } = await this.api.rest.pulls.listFiles({ ...this.repoInfo, pull_number: this.number }); + this.filesChanged = data.map((f) => f.filename); + } + return this.filesChanged; + } + + /** List all the approved reviews in a PR */ + async listApprovedReviewsAuthors(): Promise { + if (!this.usersThatApprovedThePr) { + const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number }); + const reviews = request.data as PullRequestReview[]; + const approvals = reviews.filter((review) => review.state === "approved"); + this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); + } + return this.usersThatApprovedThePr; + } } diff --git a/src/github/teams.ts b/src/github/teams.ts new file mode 100644 index 0000000..49d0596 --- /dev/null +++ b/src/github/teams.ts @@ -0,0 +1,40 @@ +import { getOctokit } from "@actions/github"; + +import { ActionLogger, GitHubClient } from "./types"; + +/** + * Interface for the acquisition of members of a team. + * As we may be using blockchain instead of GitHub teams, let's wrap the functionality inside a interface + */ +export interface TeamApi { + /** Returns all the GitHub account's logins which belong to a given team. */ + getTeamMembers(teamName: string): Promise; +} + +/** + * Implementation of the TeamApi interface using GitHub teams + * @see-also {@link TeamApi} + */ +export class GitHubTeamsApi implements TeamApi { + private readonly api: GitHubClient; + + /** Cache variable so we don't request the same information from GitHub in one run */ + private readonly teamsCache: Map = new Map(); + + /** + * @param teamOrgToken GitHub token with read:org access. It is used to access the organization team members + * @param org Name of the organization the team will belong to. Should be available in context.repo.owner + */ + constructor(teamOrgToken: string, private readonly org: string, private readonly logger: ActionLogger) { + this.api = getOctokit(teamOrgToken); + } + + async getTeamMembers(teamName: string): Promise { + // We first verify that this information hasn't been fetched yet + if (this.teamsCache.has(teamName)) { + return this.teamsCache.get(teamName) as string[]; + } + const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); + return data.map((d) => d.login); + } +} diff --git a/src/index.ts b/src/index.ts index 4a274eb..e40cfd3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,6 +4,7 @@ import { Context } from "@actions/github/lib/context"; import { PullRequest } from "@octokit/webhooks-types"; import { PullRequestApi } from "./github/pullRequest"; +import { GitHubTeamsApi } from "./github/teams"; import { ActionRunner } from "./runner"; import { generateCoreLogger } from "./util"; @@ -11,6 +12,8 @@ export interface Inputs { configLocation: string; /** GitHub's action default secret */ repoToken: string; + /** A custom access token with the read:org access */ + teamApiToken: string; } const getRepo = (ctx: Context) => { @@ -30,8 +33,9 @@ const getRepo = (ctx: Context) => { const getInputs = (): Inputs => { const configLocation = getInput("config-file"); const repoToken = getInput("repo-token", { required: true }); + const teamApiToken = getInput("team-token", { required: true }); - return { configLocation, repoToken }; + return { configLocation, repoToken, teamApiToken }; }; const repo = getRepo(context); @@ -50,9 +54,14 @@ const api = new PullRequestApi( getOctokit(inputs.repoToken), context.payload.pull_request as PullRequest, generateCoreLogger(), + repo, ); -const runner = new ActionRunner(api, generateCoreLogger()); +const logger = generateCoreLogger(); + +const teamApi = new GitHubTeamsApi(inputs.teamApiToken, repo.owner, logger); + +const runner = new ActionRunner(api, teamApi, logger); runner .runAction(inputs) diff --git a/src/runner.ts b/src/runner.ts index 4ad7e98..bdbabae 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,14 +1,31 @@ import { parse } from "yaml"; import { Inputs } from "."; -import { ConfigurationFile } from "./file/types"; +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"; +type ReviewReport = { + /** The amount of missing reviews to fulfill the requirements */ + missingReviews: number; + /** The users who would qualify to complete those reviews */ + missingUsers: string[]; + /** If applicable, the teams that should be requested to review */ + teamsToRequest?: string[]; + /** If applicable, the users that should be requested to review */ + usersToRequest?: string[]; +}; +type ReviewState = [true] | [false, ReviewReport]; + /** Action in charge of running the GitHub action */ export class ActionRunner { - constructor(private readonly prApi: PullRequestApi, private readonly logger: ActionLogger) {} + constructor( + private readonly prApi: PullRequestApi, + private readonly teamApi: TeamApi, + private readonly logger: ActionLogger, + ) {} /** * Fetches the configuration file, parses it and validates it. @@ -31,9 +48,121 @@ export class ActionRunner { return configFile; } + /** + * 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) + */ + async validatePullRequest({ rules }: ConfigurationFile): Promise { + for (const rule of rules) { + // We get all the files that were modified and match the rules condition + const files = await this.listFilesThatMatchRuleCondition(rule); + // We check if there are any matches + if (files.length === 0) { + this.logger.debug(`Skipping rule ${rule.name} as no condition matched`); + // If there are no matches, we simply skip the check + continue; + } + if (rule.type === "basic") { + const [result, missingData] = await this.evaluateCondition(rule); + if (!result) { + this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); + 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; + } + + /** Evaluates if the required reviews for a condition have been meet + * @param rule Every rule check has this values which consist on the min required approvals and the reviewers. + * @returns a [bool, error data] tuple which evaluates if the condition (not the rule itself) has fulfilled the requirements + * @see-also ReviewError + */ + async evaluateCondition(rule: { min_approvals: number } & Reviewers): Promise { + // This is a list of all the users that need to approve a PR + const requiredUsers: string[] = []; + // If team is set, we fetch the members of such team + if (rule.teams) { + for (const team of rule.teams) { + const members = await this.teamApi.getTeamMembers(team); + for (const member of members) { + // simple check to stop us from having duplicates + if (requiredUsers.indexOf(member) < 0) { + requiredUsers.push(member); + } + } + } + // If, instead, users are set, we simply push them to the array as we don't need to scan a team + } else if (rule.users) { + requiredUsers.push(...rule.users); + } else { + // This should be captured before by the validation + throw new Error("Teams and Users field are not set for rule."); + } + + // We get the list of users that approved the PR + const approvals = await this.prApi.listApprovedReviewsAuthors(); + + // This is the amount of reviews required. To succeed this should be 0 or lower + let missingReviews = rule.min_approvals; + for (const requiredUser of requiredUsers) { + // We check for the approvals, if it is a required reviewer we lower the amount of missing reviews + if (approvals.indexOf(requiredUser) > -1) { + missingReviews--; + } + } + + // Now we verify if we have any remaining missing review. + if (missingReviews > 0) { + // If we have at least one missing review, we return an object with the list of missing reviewers, and + // which users/teams we should request to review + return [ + false, + { + missingReviews, + missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0), + teamsToRequest: rule.teams ? rule.teams : undefined, + usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, + }, + ]; + } else { + // If we don't have any missing reviews, we return the succesful case + return [true]; + } + } + + /** Using the include and exclude condition, it returns a list of all the files in a PR that matches the criteria */ + async listFilesThatMatchRuleCondition({ condition }: Rule): Promise { + const files = await this.prApi.listModifiedFiles(); + let matches: string[] = []; + for (const regex of condition.include) { + for (const fileName of files) { + // If the file name matches the regex, and it has not been added to the list, we add it + if (fileName.match(regex) && matches.indexOf(fileName) < 0) { + matches.push(fileName); + } + } + } + + if (condition.exclude && matches.length > 0) { + for (const regex of condition.exclude) { + // We remove every case were it matches the exclude regex + matches = matches.filter((match) => !match.match(regex)); + } + } + + return matches; + } + async runAction(inputs: Omit): Promise { const config = await this.getConfigFile(inputs.configLocation); - return config !== null; + const success = await this.validatePullRequest(config); + + this.logger.info(success ? "The PR has been successful" : "There was an error with the PR reviews."); + + return success; } } diff --git a/src/test/runner/basicRule.test.ts b/src/test/runner/basicRule.test.ts index 8951df3..87852e7 100644 --- a/src/test/runner/basicRule.test.ts +++ b/src/test/runner/basicRule.test.ts @@ -5,17 +5,19 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BasicRule } from "../../file/types"; import { PullRequestApi } from "../../github/pullRequest"; +import { TeamApi } from "../../github/teams"; 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, logger); + runner = new ActionRunner(api, teamsApi, logger); }); 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 new file mode 100644 index 0000000..7a36b7b --- /dev/null +++ b/src/test/runner/conditions.test.ts @@ -0,0 +1,115 @@ +import { mock, MockProxy } from "jest-mock-extended"; + +import { PullRequestApi } from "../../github/pullRequest"; +import { TeamApi } from "../../github/teams"; +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); + }); + + test("should throw if no teams or users were set", async () => { + await expect(runner.evaluateCondition({ min_approvals: 99 })).rejects.toThrowError( + "Teams and Users field are not set for rule.", + ); + }); + + describe("users tests", () => { + const users = ["user-1", "user-2", "user-3"]; + beforeEach(() => { + api.listApprovedReviewsAuthors.mockResolvedValue(users); + }); + + test("should pass if required users approved the PR", async () => { + const [result] = await runner.evaluateCondition({ min_approvals: 1, users: [users[0]] }); + expect(result).toBeTruthy(); + }); + + test("should pass if required amount of users approved the PR", async () => { + const [result] = await runner.evaluateCondition({ min_approvals: 2, users: [users[0], users[users.length - 1]] }); + expect(result).toBeTruthy(); + }); + + test("should fail if not all required users approved the PR", async () => { + const newUser = "missing-user"; + const [result, missingData] = await runner.evaluateCondition({ min_approvals: 2, users: [users[0], newUser] }); + expect(result).toBeFalsy(); + expect(missingData?.missingUsers).toContainEqual(newUser); + expect(missingData?.missingUsers).not.toContainEqual(users[0]); + expect(missingData?.usersToRequest).toContainEqual(newUser); + expect(missingData?.usersToRequest).not.toContainEqual(users[0]); + expect(missingData?.missingReviews).toBe(1); + }); + }); + + describe("teams tests", () => { + const users = ["user-1", "user-2", "user-3"]; + const team = "team-example"; + beforeEach(() => { + api.listApprovedReviewsAuthors.mockResolvedValue(users); + }); + + test("should pass if required users approved the PR", async () => { + teamsApi.getTeamMembers.mockResolvedValue(users); + const [result] = await runner.evaluateCondition({ min_approvals: 1, teams: [team] }); + expect(result).toBeTruthy(); + }); + + test("should pass if required amount of users approved the PR", async () => { + teamsApi.getTeamMembers.mockResolvedValue(users); + const [result] = await runner.evaluateCondition({ min_approvals: 2, teams: [team] }); + expect(result).toBeTruthy(); + }); + + test("should fail if not enough members of a team approved the PR", async () => { + api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); + teamsApi.getTeamMembers.mockResolvedValue(users); + const [result, missingData] = await runner.evaluateCondition({ min_approvals: 2, teams: [team] }); + expect(result).toBeFalsy(); + expect(missingData?.missingUsers).toEqual(users.slice(1)); + expect(missingData?.missingUsers).not.toContainEqual(users[0]); + expect(missingData?.usersToRequest).toBeUndefined(); + expect(missingData?.teamsToRequest).toContainEqual(team); + expect(missingData?.missingReviews).toBe(1); + }); + + describe("multiple teams", () => { + test("should work with more than one team", async () => { + const team1 = { name: "team-1", users: ["team-1-user-1", "team-1-user-2", "team-1-user-3"] }; + const team2 = { name: "team-2", users: ["team-2-user-1", "team-2-user-2", "team-2-user-3"] }; + api.listApprovedReviewsAuthors.mockResolvedValue([ + team1.users[0], + team1.users[1], + team2.users[0], + team2.users[1], + ]); + teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users); + teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users); + const [result] = await runner.evaluateCondition({ min_approvals: 4, teams: [team1.name, team2.name] }); + expect(result).toBeTruthy(); + }); + + test("should not duplicate user if they belong to more than one team", async () => { + const team1 = { name: "team-1", users: ["team-1-user-1", "team-1-user-2"] }; + const team2 = { name: "team-2", users: ["team-2-user-1", team1.users[0], team1.users[1]] }; + teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users); + teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users); + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const [result, report] = await runner.evaluateCondition({ min_approvals: 4, teams: [team1.name, team2.name] }); + expect(result).toBeFalsy(); + // Should not send required users more than once + expect(report?.missingUsers).toEqual([...team1.users, team2.users[0]]); + expect(report?.teamsToRequest).toEqual([team1.name, team2.name]); + }); + }); + }); +}); diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index 751b6a5..db2eb33 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -4,17 +4,19 @@ import { mock, MockProxy } from "jest-mock-extended"; import { PullRequestApi } from "../../github/pullRequest"; +import { TeamApi } from "../../github/teams"; import { ActionRunner } from "../../runner"; import { TestLogger } from "../logger"; describe("Config Parsing", () => { let api: MockProxy; + let teamsApi: MockProxy; let runner: ActionRunner; let logger: TestLogger; beforeEach(() => { logger = new TestLogger(); api = mock(); - runner = new ActionRunner(api, logger); + runner = new ActionRunner(api, teamsApi, logger); }); test("should get minimal config", async () => { api.getConfigFile.mockResolvedValue(` @@ -78,7 +80,7 @@ describe("Config Parsing", () => { - team-b `); const config = await runner.getConfigFile(""); - expect(config.preventReviewRequests.teams).toEqual(["team-a", "team-b"]); + expect(config.preventReviewRequests?.teams).toEqual(["team-a", "team-b"]); }); test("should get users", async () => { @@ -100,7 +102,7 @@ describe("Config Parsing", () => { - user-b `); const config = await runner.getConfigFile(""); - expect(config.preventReviewRequests.users).toEqual(["user-a", "user-b"]); + expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]); }); test("should fail with both users and teams", async () => { diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts new file mode 100644 index 0000000..f5e0283 --- /dev/null +++ b/src/test/runner/runner.test.ts @@ -0,0 +1,65 @@ +import { mock, MockProxy } from "jest-mock-extended"; + +import { ConfigurationFile, Rule, RuleTypes } from "../../file/types"; +import { PullRequestApi } from "../../github/pullRequest"; +import { TeamApi } from "../../github/teams"; +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); + }); + + test("validatePullRequest should return true if no rule matches any files", async () => { + const config: ConfigurationFile = { + rules: [ + { name: "Rule 1", type: RuleTypes.Basic, condition: { include: ["src"] }, min_approvals: 1 }, + { name: "Rule 2", type: RuleTypes.Basic, condition: { include: ["README.md"] }, min_approvals: 99 }, + ], + }; + api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml", "LICENSE"]); + const evaluation = await runner.validatePullRequest(config); + expect(evaluation).toBeTruthy(); + }); + + describe("listFilesThatMatchRuleCondition tests", () => { + test("should get values that match the condition", async () => { + const mockRule = { condition: { include: ["src"] } }; + api.listModifiedFiles.mockResolvedValue(["src/index.ts", "README.md"]); + const result = await runner.listFilesThatMatchRuleCondition(mockRule as Rule); + expect(result).toContainEqual("src/index.ts"); + }); + + test("should return only one file even if more than one rule matches it", async () => { + const mockRule = { condition: { include: ["\\.ts", "src"] } }; + api.listModifiedFiles.mockResolvedValue(["src/index.ts"]); + const result = await runner.listFilesThatMatchRuleCondition(mockRule as Rule); + expect(result).toEqual(["src/index.ts"]); + }); + + test("should include all the files with a global value", async () => { + const mockRule = { condition: { include: [".+", "src"] } }; + const listedFiles = ["src/index.ts", ".github/workflows/review-bot.yml", "yarn-error.log"]; + api.listModifiedFiles.mockResolvedValue(listedFiles); + const result = await runner.listFilesThatMatchRuleCondition(mockRule as Rule); + expect(result).toEqual(listedFiles); + }); + + test("should exclude files if they are captured by the include condition", async () => { + const mockRule = { condition: { include: [".+"], exclude: ["\\.yml"] } }; + const listedFiles = ["src/index.ts", ".github/workflows/review-bot.yml", "yarn-error.log"]; + api.listModifiedFiles.mockResolvedValue(listedFiles); + const result = await runner.listFilesThatMatchRuleCondition(mockRule as Rule); + expect(result).toContainEqual("src/index.ts"); + expect(result).toContainEqual("yarn-error.log"); + expect(result).not.toContain(".github/workflows/review-bot.yml"); + }); + }); +});