Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created BASIC rule implementation #23

Merged
merged 15 commits into from
Jul 25, 2023
Merged
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
12 changes: 6 additions & 6 deletions src/file/types.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
}

Expand All @@ -26,8 +26,8 @@ export interface ConfigurationFile {
* @see {@link Rules}
*/
rules: (BasicRule | DebugRule)[];
preventReviewRequests: {
preventReviewRequests?: {
teams?: string[];
users: string[];
users?: string[];
};
}
33 changes: 31 additions & 2 deletions src/github/pullRequest.ts
Original file line number Diff line number Diff line change
@@ -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<string> {
const { data } = await this.api.rest.repos.getContent({
Expand All @@ -29,4 +38,24 @@ export class PullRequestApi {

return decryptedFile;
}

/** Returns an array with all the files that had been modified */
async listModifiedFiles(): Promise<string[]> {
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<string[]> {
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;
}
}
40 changes: 40 additions & 0 deletions src/github/teams.ts
Original file line number Diff line number Diff line change
@@ -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<string[]>;
}

/**
* 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<string, string[]> = new Map<string, string[]>();

/**
* @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<string[]> {
// 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);
}
}
13 changes: 11 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ 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";

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) => {
Expand All @@ -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);
Expand All @@ -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)
Expand Down
135 changes: 132 additions & 3 deletions src/runner.ts
Original file line number Diff line number Diff line change
@@ -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 ReviewErrorData = {
/** 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 ReviewError = [true] | [false, ReviewErrorData];

/** 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.
Expand All @@ -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<boolean> {
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<ReviewError> {
Bullrich marked this conversation as resolved.
Show resolved Hide resolved
// 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);
Bullrich marked this conversation as resolved.
Show resolved Hide resolved
} 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--;
Bullrich marked this conversation as resolved.
Show resolved Hide resolved
}
}

// 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<string[]> {
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<Inputs, "repoToken">): Promise<boolean> {
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;
}
}
4 changes: 3 additions & 1 deletion src/test/runner/basicRule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PullRequestApi>;
let runner: ActionRunner;
let teamsApi: MockProxy<TeamApi>;
let logger: TestLogger;
beforeEach(() => {
logger = new TestLogger();
api = mock<PullRequestApi>();
runner = new ActionRunner(api, logger);
runner = new ActionRunner(api, teamsApi, logger);
});
test("should get minimal config", async () => {
api.getConfigFile.mockResolvedValue(`
Expand Down
Loading
Loading