Skip to content

Commit

Permalink
Created BASIC rule implementation (#23)
Browse files Browse the repository at this point in the history
## Basic rule
Implemented the functionality of the basic rule.

Because every rule ends up being simplified into:
```typescript
{
  min_approvals: number;
  teams?: string[];
  users?: string[];
}
```

I created a basic logic that evaluates that. After this, we can use the
result of those smaller conditions to evaluate the more complex rules.

I added a lot of tests and tried to do as many comments to explain the
logic as possible.

## Teams API
Created class which handles the teams token and obtains the team members
of a team.

This class is small but handles the token used for such authentication
and separates the concern.

If we decide to replace GitHub teams for on-chain data (like mentioned
in paritytech/opstooling#245) this would let us to simply switch the
implementation with minimal alterations.

Closes #22 and closes #9
  • Loading branch information
Bullrich authored Jul 25, 2023
1 parent 469de22 commit dfce8aa
Show file tree
Hide file tree
Showing 11 changed files with 414 additions and 17 deletions.
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 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.
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<ReviewState> {
// 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<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

0 comments on commit dfce8aa

Please sign in to comment.