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 #21

Merged
merged 9 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/file/types.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,31 @@
enum Rules {
Basic = "basic",
Debug = "debug",
}

type Reviewers = { users?: string[]; teams?: string[] };

export interface Rule {
name: string;
condition: { include: string[]; exclude?: string[] };
}

// TODO: Delete this once we add a second type of rule
export interface DebugRule extends Rule {
type: Rules.Debug;
size: number;
}

export interface BasicRule extends Rule, Reviewers {
type: Rules.Basic;
min_approvals: number;
}

export interface ConfigurationFile {
rules: Rule[];
/** Based on the `type` parameter, Typescript converts the object to the correct type
* @see {@link Rules}
*/
rules: (BasicRule | DebugRule)[];
preventReviewRequests: {
teams?: string[];
users: string[];
Expand Down
68 changes: 58 additions & 10 deletions src/file/validator.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,75 @@
import { validate } from "@eng-automation/js";
import Joi from "joi";

import { ActionLogger } from "../github/types";
import { ConfigurationFile, Rule } from "./types";
import { BasicRule, ConfigurationFile, Rule } from "./types";

const ruleSchema = Joi.object<Rule>().keys({
/** For the users or team schema. Will be recycled A LOT
* Remember to add `.xor("users", "teams")` to force one of the two to be picked up
*/
const reviewersObj = {
users: Joi.array().items(Joi.string()).optional().empty(null),
teams: Joi.array().items(Joi.string()).optional().empty(null),
};

/** Base rule condition.
* This are the minimum requirements that all the rules must have.
* After we evaluated this, we can run a custom evaluation per rule
*/
const ruleSchema = Joi.object<Rule & { type: string }>().keys({
name: Joi.string().required(),
condition: Joi.object<Rule["condition"]>().keys({
include: Joi.array().items(Joi.string()).required(),
exclude: Joi.array().items(Joi.string()).optional().allow(null),
}),
type: Joi.string().required(),
});

export const schema = Joi.object<ConfigurationFile>().keys({
/** General Configuration schema.
* Evaluates all the upper level field plus the generic rules fields.
* Remember to evaluate the rules with their custom rules
*/
export const generalSchema = Joi.object<ConfigurationFile>().keys({
rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).required(),
preventReviewRequests: Joi.object<ConfigurationFile["preventReviewRequests"]>()
.keys({
users: Joi.array().items(Joi.string()).optional().allow(null),
teams: Joi.array().items(Joi.string()).optional().allow(null),
})
.optional()
.allow(null),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().xor("users", "teams"),
});

/** Basic rule schema
* This rule is quite simple as it only has the min_approvals field and the required reviewers
*/
export const basicRuleSchema = Joi.object<BasicRule>()
.keys({ min_approvals: Joi.number().empty(1), ...reviewersObj })
.xor("users", "teams");

/**
* Evaluates a config thoroughly. If there is a problem with it, it will throw.
*
* It first evaluates the configuration on a higher level and then runs individually per rule
* @see-also {@link generalSchema}
* @param config The configuration object to be validated. Usually parsed directly from a yaml or json
* @returns The configuration file post validation, or it throws an error.
*/
export const validateConfig = (config: ConfigurationFile): ConfigurationFile | never => {
const validatedConfig = validate<ConfigurationFile>(config, generalSchema, {
message: "Configuration file is invalid",
});

for (const rule of validatedConfig.rules) {
const { name, type } = rule;
const message = `Configuration for rule '${rule.name}' is invalid`;
if (type === "basic") {
validate<BasicRule>(rule, basicRuleSchema, { message });
} else if (type === "debug") {
validate<Rule>(rule, ruleSchema, { message });
} else {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
throw new Error(`Rule ${name} has an invalid type: ${type}`);
}
}

return validatedConfig;
};

/** Evaluate if the regex expression inside a configuration are valid.
* @returns a tuple of type [boolean, string]. If the boolean is false, the string will contain an error message
* @example
Expand Down
7 changes: 3 additions & 4 deletions src/runner.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { validate } from "@eng-automation/js";
import { parse } from "yaml";

import { Inputs } from ".";
import { ConfigurationFile } from "./file/types";
import { schema, validateRegularExpressions } from "./file/validator";
import { validateConfig, validateRegularExpressions } from "./file/validator";
import { PullRequestApi } from "./github/pullRequest";
import { ActionLogger } from "./github/types";

Expand All @@ -18,11 +17,11 @@ export class ActionRunner {
async getConfigFile(configLocation: string): Promise<ConfigurationFile> {
const content = await this.prApi.getConfigFile(configLocation);
this.logger.debug(content);
const config: unknown = parse(content);
const config = parse(content) as ConfigurationFile;

this.logger.info(`Obtained config at ${configLocation}`);

const configFile = validate<ConfigurationFile>(config, schema, { message: "Configuration file is invalid" });
const configFile = validateConfig(config);

const [result, error] = validateRegularExpressions(configFile, this.logger);
if (!result) {
Expand Down
88 changes: 88 additions & 0 deletions src/test/runner/basicRule.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/* eslint-disable @typescript-eslint/unbound-method */
/* eslint-disable @typescript-eslint/no-unsafe-assignment */
/* eslint-disable @typescript-eslint/no-unsafe-call */
import { mock, MockProxy } from "jest-mock-extended";

import { BasicRule } from "../../file/types";
import { PullRequestApi } from "../../github/pullRequest";
import { ActionRunner } from "../../runner";
import { TestLogger } from "../logger";

describe("Basic rule parsing", () => {
let api: MockProxy<PullRequestApi>;
let runner: ActionRunner;
let logger: TestLogger;
beforeEach(() => {
logger = new TestLogger();
api = mock<PullRequestApi>();
runner = new ActionRunner(api, logger);
});
test("should get minimal config", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
- name: Test review
condition:
include:
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example
`);
const config = await runner.getConfigFile("");
expect(config.rules[0].name).toEqual("Test review");
expect(config.rules[0].type).toEqual("basic");
});

test("should require teams", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
- name: Test review
condition:
include:
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example
`);
const config = await runner.getConfigFile("");
const rule = config.rules[0] as BasicRule;
expect(rule.teams).toContainEqual("team-example");
expect(rule.users).toBeUndefined();
});
test("should require users", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
- name: Test review
condition:
include:
- '.*'
exclude:
- 'example'
type: basic
users:
- user-example
`);
const config = await runner.getConfigFile("");
const rule = config.rules[0] as BasicRule;
expect(rule.users).toContainEqual("user-example");
expect(rule.teams).toBeUndefined();
});

test("should fail without reviewers", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
- name: Test review
condition:
include:
- '.*'
exclude:
- 'example'
type: basic
`);
await expect(runner.getConfigFile("")).rejects.toThrowError('"value" must contain at least one of [users, teams]');
});
});
50 changes: 50 additions & 0 deletions src/test/runner/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ describe("Config Parsing", () => {
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example
`);
const config = await runner.getConfigFile("");
expect(config.preventReviewRequests).toBeUndefined();
Expand All @@ -44,6 +47,9 @@ describe("Config Parsing", () => {
condition:
include:
- '${invalidRegex}'
type: basic
teams:
- team-example
`);
await expect(runner.getConfigFile("")).rejects.toThrowError(
`Regular expression is invalid: Include condition '${invalidRegex}' is not a valid regex`,
Expand All @@ -62,6 +68,9 @@ describe("Config Parsing", () => {
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example

preventReviewRequests:
teams:
Expand All @@ -81,6 +90,9 @@ describe("Config Parsing", () => {
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example

preventReviewRequests:
users:
Expand All @@ -91,6 +103,32 @@ describe("Config Parsing", () => {
expect(config.preventReviewRequests.users).toEqual(["user-a", "user-b"]);
});

test("should fail with both users and teams", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
- name: Default review
condition:
include:
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example

preventReviewRequests:
users:
- user-a
- user-b
teams:
- team-a
- team-b
`);
await expect(runner.getConfigFile("")).rejects.toThrowError(
'"preventReviewRequests" contains a conflict between exclusive peers [users, teams]',
);
});

test("should pass if preventReviewRequests is not assigned", async () => {
api.getConfigFile.mockResolvedValue(`
rules:
Expand All @@ -100,6 +138,9 @@ describe("Config Parsing", () => {
- '.*'
exclude:
- 'example'
type: basic
teams:
- team-example
`);
const config = await runner.getConfigFile("");
expect(config.preventReviewRequests).toBeUndefined();
Expand All @@ -116,6 +157,9 @@ describe("Config Parsing", () => {
- 'example-include-rule-2'
exclude:
- 'example-exclude-rule'
type: basic
teams:
- team-example
`;
it("should parse include conditions", async () => {
api.getConfigFile.mockResolvedValue(exampleConfig);
Expand All @@ -132,6 +176,9 @@ describe("Config Parsing", () => {
condition:
exclude:
- 'example'
type: basic
teams:
- team-example
`);
await expect(runner.getConfigFile("")).rejects.toThrowError('"rules[0].condition.include" is required');
});
Expand All @@ -151,6 +198,9 @@ describe("Config Parsing", () => {
condition:
include:
- '.*'
type: basic
teams:
- team-1
`);
const config = await runner.getConfigFile("");
expect(config.rules[0].condition.exclude).toBeUndefined();
Expand Down