Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Abstracted code logic #108

Merged
merged 11 commits into from
Mar 14, 2023
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
"fix:eslint": "eslint --fix",
"fix:prettier": "prettier --write",
"fix": "yarn fix:eslint '{*,**/*}.{cjs,ts}' && yarn fix:prettier '{*,**/*}.json'",
"prettier": "prettier --check --loglevel silent '{*,**/*}.{json,html}'",
"eslint": "eslint --quiet '{*,**/*}.ts'",
"lint": "yarn eslint && yarn prettier",
"build": "rimraf build/prcr; tsc",
"test": "jest",
"coverage": "yarn test --coverage",
Expand Down
103 changes: 20 additions & 83 deletions src/core.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,11 @@
import { OctokitResponse } from "@octokit/types";
import assert from "assert";
import Permutator from "iterative-permutation";
import YAML from "yaml";

import {
actionReviewTeamFiles,
commitStateFailure,
commitStateSuccess,
configFilePath,
maxGithubApiFilesPerPage,
maxGithubApiReviewsPerPage,
maxGithubApiTeamMembersPerPage,
} from "./constants";

import { actionReviewTeamFiles, commitStateFailure, commitStateSuccess } from "./constants";
import { ActionData } from "./github/action/types";
import { GitHubApi } from "./github/api";
import { CommitState } from "./github/types";
import { BaseRule, Context, MatchedRule, PR, RuleCriteria, RuleFailure, RuleUserInfo } from "./types";
import { configurationSchema } from "./validation";

const displayUserWithTeams = (user: string, teams: Set<string> | undefined | null) =>
`${user}${teams ? ` (team${teams.size === 1 ? "" : "s"}: ${Array.from(teams).join(", ")})` : ""}`;
Expand Down Expand Up @@ -65,8 +55,8 @@ const processSubconditionMissingApprovers = (
};

type TeamsCache = Map<string /* Team slug */, string[] /* Usernames of team members */>;
const combineUsers = async (
{ octokit }: Context,
export const combineUsers = async (
api: GitHubApi,
pr: PR,
presetUsers: string[],
teams: string[],
Expand All @@ -84,11 +74,7 @@ const combineUsers = async (
let teamMembers = teamsCache.get(team);

if (teamMembers === undefined) {
teamMembers = await octokit.paginate(
octokit.rest.teams.listMembersInOrg,
{ org: pr.base.repo.owner.login, team_slug: team, per_page: maxGithubApiTeamMembersPerPage },
(response) => response.data.map(({ login }) => login),
);
teamMembers = await api.getTeamMembers(team);
teamsCache.set(team, teamMembers);
}

Expand Down Expand Up @@ -120,33 +106,9 @@ const combineUsers = async (
without inconveniences. If you need more external input then pass it as a
function argument.
*/
export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
const { octokit, logger } = ctx;

const configFileResponse = await octokit.rest.repos.getContent({
owner: pr.base.repo.owner.login,
repo: pr.base.repo.name,
path: configFilePath,
});
if (!("content" in configFileResponse.data)) {
logger.fatal(`Did not find "content" key in the response for ${configFilePath}`);
logger.info(configFileResponse.data);
return commitStateFailure;
}

const { content: configFileContentsEnconded } = configFileResponse.data;
if (typeof configFileContentsEnconded !== "string") {
logger.fatal(`Content response for ${configFilePath} had unexpected type (expected string)`);
logger.info(configFileResponse.data);
return commitStateFailure;
}

const configFileContents = Buffer.from(configFileContentsEnconded, "base64").toString("utf-8");

const configValidationResult = configurationSchema.validate(YAML.parse(configFileContents));
if (configValidationResult.error) {
logger.fatal("Configuration file is invalid");
logger.info(configValidationResult.error);
export const runChecks = async ({ pr, logger }: Context & { pr: PR }, api: GitHubApi) => {
const config = await api.fetchConfigFile();
if (!config) {
return commitStateFailure;
}

Expand All @@ -156,7 +118,7 @@ export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
"action-review-team": actionReviewTeam,
rules,
"prevent-review-request": preventReviewRequest,
} = configValidationResult.value;
} = config;

const getUsersInfo = (() => {
/*
Expand All @@ -165,16 +127,10 @@ export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
*/
const teamsCache: TeamsCache = new Map();

return (users: string[], teams: string[]) => combineUsers(ctx, pr, users, teams, teamsCache);
return (users: string[], teams: string[]) => combineUsers(api, pr, users, teams, teamsCache);
})();

const diffResponse = (await octokit.rest.pulls.get({
owner: pr.base.repo.owner.login,
repo: pr.base.repo.name,
pull_number: pr.number,
mediaType: { format: "diff" },
})) /* Octokit doesn't inform the right return type for mediaType: { format: "diff" } */ as unknown as OctokitResponse<string>;
const { data: diff } = diffResponse;
const diff = await api.fetchDiff();

const matchedRules: MatchedRule[] = [];

Expand All @@ -197,16 +153,7 @@ export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
});
}

const changedFiles = new Set(
(
await octokit.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/files", {
owner: pr.base.repo.owner.login,
repo: pr.base.repo.name,
pull_number: pr.number,
per_page: maxGithubApiFilesPerPage,
})
).map(({ filename }) => filename),
);
const changedFiles = new Set(await api.fetchChangedFiles());
logger.info("Changed files", changedFiles);

for (const actionReviewFile of actionReviewTeamFiles) {
Expand Down Expand Up @@ -345,12 +292,7 @@ export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
}

if (matchedRules.length !== 0) {
const reviews = await octokit.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews", {
owner: pr.base.repo.owner.login,
repo: pr.base.repo.name,
pull_number: pr.number,
per_page: maxGithubApiReviewsPerPage,
});
const reviews = await api.fetchReviews();

const latestReviews: Map<number, { id: number; user: string; isApproval: boolean }> = new Map();
for (const review of reviews) {
Expand Down Expand Up @@ -770,13 +712,7 @@ export const runChecks = async ({ pr, ...ctx }: Context & { pr: PR }) => {
}
}
if (users.size || teams.size) {
await octokit.request("POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers", {
owner: pr.base.repo.owner.login,
repo: pr.base.repo.name,
pull_number: pr.number,
reviewers: Array.from(users),
team_reviewers: Array.from(teams),
});
await api.requestReviewers(Array.from(users), Array.from(teams));
}
}

Expand All @@ -802,9 +738,9 @@ export const getFinishProcessReviews =
// Fallback URL in case we are not able to detect the current job
if (state === "failure" && jobName !== undefined) {
/*
Fetch the jobs so that we'll be able to detect this step and provide a
more accurate logging location
*/
Fetch the jobs so that we'll be able to detect this step and provide a
more accurate logging location
*/
const {
data: { jobs },
} = await octokit.rest.actions.listJobsForWorkflowRun({
Expand Down Expand Up @@ -851,7 +787,8 @@ export const getFinishProcessReviews =

export const processReviews = async (ctx: Context, { pr }: ActionData) => {
const { finishProcessReviews, logger } = ctx;
return await runChecks({ ...ctx, pr })
const githubApi = new GitHubApi(pr, ctx);
return await runChecks({ ...ctx, pr }, githubApi)
.then((state) => {
if (finishProcessReviews) {
return finishProcessReviews(state);
Expand Down
117 changes: 117 additions & 0 deletions src/github/api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { GitHub } from "@actions/github/lib/utils";
import { OctokitResponse } from "@octokit/types";
import YAML from "yaml";

import {
configFilePath,
maxGithubApiFilesPerPage,
maxGithubApiReviewsPerPage,
maxGithubApiTeamMembersPerPage,
} from "src/constants";
import { Configuration, Context, PR } from "src/types";
import { configurationSchema } from "src/validation";

import { ActionLoggerInterface } from "./action/logger";

export interface Review {
state: "COMMENTED" | "REQUEST_CHANGES" | "APPROVE" | string;
user?: { id: number; login: string } | null;
id: number;
}

/** Class in charge of interacting with GitHub. */
export class GitHubApi {
private readonly octokit: InstanceType<typeof GitHub>;
private readonly logger: ActionLoggerInterface;

constructor(private readonly pr: PR, { octokit, logger }: Context) {
this.octokit = octokit;
this.logger = logger;
}

/** Fetches the config file and validates that is the correct type/format */
async fetchConfigFile(): Promise<Configuration | null> {
const configFileResponse = await this.octokit.rest.repos.getContent({
owner: this.pr.base.repo.owner.login,
repo: this.pr.base.repo.name,
path: configFilePath,
});
if (!("content" in configFileResponse.data)) {
this.logger.fatal(`Did not find "content" key in the response for ${configFilePath}`);
this.logger.info(configFileResponse.data);
return null;
}

const { content: configFileContentsEnconded } = configFileResponse.data;
if (typeof configFileContentsEnconded !== "string") {
this.logger.fatal(`Content response for ${configFilePath} had unexpected type (expected string)`);
this.logger.info(configFileResponse.data);
return null;
}

const configFileContents = Buffer.from(configFileContentsEnconded, "base64").toString("utf-8");

const configValidationResult = configurationSchema.validate(YAML.parse(configFileContents));
if (configValidationResult.error) {
this.logger.fatal("Configuration file is invalid");
this.logger.info(configValidationResult.error);
return null;
}

return configValidationResult.value;
}

/** Fetches the diff in the PR */
async fetchDiff(): Promise<string> {
const diffResponse = (await this.octokit.rest.pulls.get({
owner: this.pr.base.repo.owner.login,
repo: this.pr.base.repo.name,
pull_number: this.pr.number,
mediaType: { format: "diff" },
})) /* Octokit doesn't inform the right return type for mediaType: { format: "diff" } */ as unknown as OctokitResponse<string>;
return diffResponse.data;
}

/** Returns a list of all files that were changed */
async fetchChangedFiles(): Promise<string[]> {
const data = await this.octokit.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/files", {
owner: this.pr.base.repo.owner.login,
repo: this.pr.base.repo.name,
pull_number: this.pr.number,
per_page: maxGithubApiFilesPerPage,
});
return data.map(({ filename }) => filename);
}

/** Fetches the list of reviews to a repo
* Includes comments and failed reviews
*/
async fetchReviews(): Promise<Review[]> {
return await this.octokit.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews", {
owner: this.pr.base.repo.owner.login,
repo: this.pr.base.repo.name,
pull_number: this.pr.number,
per_page: maxGithubApiReviewsPerPage,
});
}

/** Request users/teams to review this PR */
async requestReviewers(users: string[], teams: string[]): Promise<void> {
await this.octokit.request("POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers", {
owner: this.pr.base.repo.owner.login,
repo: this.pr.base.repo.name,
pull_number: this.pr.number,
reviewers: users,
team_reviewers: teams,
});
}

/** Gets all the members belonging to a team */
async getTeamMembers(team_slug: string): Promise<string[]> {
return await this.octokit.paginate(
this.octokit.rest.teams.listMembersInOrg,
{ org: this.pr.base.repo.owner.login, team_slug, per_page: maxGithubApiTeamMembersPerPage },
(response) => response.data.map(({ login }) => login),
);
}
}
7 changes: 5 additions & 2 deletions test/batch/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import YAML from "yaml";

import { rulesConfigurations } from "src/constants";
import { runChecks } from "src/core";
import { GitHubApi } from "src/github/api";

const setup = ({ rules }: { rules?: Record<string, unknown>[] }) => {
nock(githubApi)
Expand Down Expand Up @@ -69,7 +70,8 @@ describe("Configuration", () => {

setup({ rules: [badRule] });

expect(await runChecks({ pr: basePR, octokit, logger, finishProcessReviews: null })).toBe("failure");
const api = new GitHubApi(basePR, { logger, finishProcessReviews: null, octokit });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const api = new GitHubApi(basePR, { logger, finishProcessReviews: null, octokit });
const api = new GitHubApi(basePR, { logger, octokit });

It's not taken by GitHubApi. Same in other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it's a requirement in the object union that the method is receiving:

Argument of type '{ pr: PR; octokit: Octokit & { paginate: PaginateInterface; } & RestEndpointMethods & Api; logger: TestLogger; }' is not assignable to parameter of type 'Context & { pr: PR; }'.
Property 'finishProcessReviews' is missing in type '{ pr: PR; octokit: Octokit & { paginate: PaginateInterface; } & RestEndpointMethods & Api; logger: TestLogger; }' but required in type 'Context'.ts(2345)

I'll look into cleaning it in the future

expect(await runChecks({ pr: basePR, octokit, logger, finishProcessReviews: null }, api)).toBe("failure");

expect(logHistory).toMatchSnapshot();
});
Expand Down Expand Up @@ -99,7 +101,8 @@ describe("Configuration", () => {
],
});

expect(await runChecks({ pr: basePR, octokit, logger, finishProcessReviews: null })).toBe("failure");
const api = new GitHubApi(basePR, { logger, finishProcessReviews: null, octokit });
expect(await runChecks({ pr: basePR, octokit, logger, finishProcessReviews: null }, api)).toBe("failure");

expect(logHistory).toMatchSnapshot();
});
Expand Down
Loading