Skip to content

Commit

Permalink
Created basic rule (#21)
Browse files Browse the repository at this point in the history
Created object for basic rule.

First step towards #9

By using Types in typescript we can differ what is the type of rule
easily. I also added checks to Joi to ensure that the rule has
everything required before individually validating it.

Added tests to evaluate the process thoroughly.
  • Loading branch information
Bullrich committed Jul 18, 2023
1 parent 1f41374 commit 469de22
Show file tree
Hide file tree
Showing 5 changed files with 221 additions and 15 deletions.
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

0 comments on commit 469de22

Please sign in to comment.