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

Added rule type 'fellows' #85

Merged
merged 9 commits into from
Sep 21, 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
47 changes: 33 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,6 @@ rules:
users:
- user-1
- user-2
minFellowsRank: 2
countAuthor: true
allowedToSkipRule:
teams:
Expand All @@ -257,13 +256,9 @@ It has the same parameters as a normal rule:
- **Optional**: Defaults to 1.
- Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users)
- **teams**: An array of team *slugs* that need to review this file.
- *Optional if **minFellowsRank** or **users** are defined*.
- *Optional if **users** is defined*.
- **users**: An array of the GitHub usernames of the users who need to review this file.
- *Optional if **teams** or **minFellowsRank** are defined*.
- **minFellowsRank**: A number defining the minimum [minFellowsRank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR.
- *Optional if **teams** or **users** are defined*.
- If it is set to minFellowsRank 3, it will need the approval of a fellow of minFellowsRank 3 or higher to pass.
- The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval.
- *Optional if **teams** is defined*.
- **countAuthor**: If the pull request author should be considered as an approval.
- If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total).
- ** Optional**: Defaults to `false`
Expand Down Expand Up @@ -308,13 +303,9 @@ rules:
- **Optional**: Defaults to 1.
- Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users)
- **teams**: An array of team *slugs* that need to review this file.
- *Optional if **minFellowsRank** or **users** are defined*.
- *Optional if **users** is defined*.
- **users**: An array of the GitHub usernames of the users who need to review this file.
- *Optional if **teams** or **minFellowsRank** are defined*.
- **minFellowsRank**: A number defining the minimum [minFellowsRank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR.
- *Optional if **teams** or **users** are defined*.
- If it is set to minFellowsRank 3, it will need the approval of a fellow of minFellowsRank 3 or higher to pass.
- The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval.
- *Optional if **teams** is defined*.
##### Or rule logic
This is a rule that has at least two available options of reviewers and needs **at least one group to approve**.

Expand Down Expand Up @@ -365,7 +356,35 @@ The logic in this rule is the *same as the `and` rule **but** with one exception
Meaning that if a user belongs to both `team-abc` and `team-example` their approval will count only towards one of the available options *even if they fulfill both needs*.

This rule is useful when you need to make sure that at leasts two sets of eyes of two different teams review a Pull Request.

#### Fellows rule
The fellows rule has a slight difference to all of the rules:
```yaml
- name: Fellows review
condition:
include:
- '.*'
exclude:
- 'example'
type: fellows
minRank: 2
min_approvals: 2
```
The biggest difference is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field.

This field receives a number, which will be the lowest rank required to evaluate the PR, and then it fetches [all the fellows from the chain data](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship), filters only the one to belong to that rank or above and then [looks into their metadata for a field name `github` and the handle there](https://github.com/polkadot-fellows/runtimes/issues/7).

After this is done, the resulting handles will be treated like a normal list of required users.

It also has any other field from the [`basic rule`](#basic-rule) (with the exception of `users` and `teams`):
- **name**
- **conditions**:
- **include** is **required**.
- **exclude** is **optional**.
- **type**: Must be `fellows`.
- **countAuthor**: If the pull request author should be considered as an approval.
- **Optional**: Defaults to `false`.
- **minRank**: Must be a number.
- **Required**
### Evaluating config

If you want to evaluate the config file to find problems before merging it, we have a simple `cli` to do so.
Expand Down
11 changes: 9 additions & 2 deletions src/rules/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ export enum RuleTypes {
And = "and",
Or = "or",
AndDistinct = "and-distinct",
Fellows = "fellows",
}

export type Reviewers = { users?: string[]; teams?: string[]; minFellowsRank?: number; min_approvals: number };
export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: number };

export interface Rule {
name: string;
Expand Down Expand Up @@ -33,11 +34,17 @@ export interface AndDistinctRule extends Rule {
reviewers: Reviewers[];
}

export interface FellowsRule extends Rule {
type: RuleTypes.Fellows;
minRank: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to make this consistent - either camelCase or snake for config or there's some purpose?

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 would prefer camelCase. Would it be fine if I make a second PR moments after this renaming min_approvals to minApprovals?

min_approvals: number;
}

export interface ConfigurationFile {
/** Based on the `type` parameter, Typescript converts the object to the correct type
* @see {@link Rules}
*/
rules: (BasicRule | AndRule | OrRule | AndDistinctRule)[];
rules: (BasicRule | AndRule | OrRule | AndDistinctRule | FellowsRule)[];
preventReviewRequests?: {
teams?: string[];
users?: string[];
Expand Down
22 changes: 16 additions & 6 deletions src/rules/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@ import { validate } from "@eng-automation/js";
import Joi from "joi";

import { ActionLogger } from "../github/types";
import { AndRule, BasicRule, ConfigurationFile, Reviewers, Rule, RuleTypes } from "./types";
import { AndRule, BasicRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./types";

/** For the users or team schema. Will be recycled A LOT
* Remember to add `.or("users", "teams")` to force at least one of the two to be defined
*/
const reviewersObj = {
users: Joi.array().items(Joi.string()).optional().empty(null),
teams: Joi.array().items(Joi.string()).optional().empty(null),
minFellowsRank: Joi.number().optional().min(1).empty(null),
};

const reviewerConditionObj = { ...reviewersObj, min_approvals: Joi.number().min(1).default(1) };
Expand All @@ -26,7 +25,9 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
exclude: Joi.array().items(Joi.string()).optional().allow(null),
}),
allowedToSkipRule: Joi.object<Omit<Reviewers, "min_approvals">>().keys(reviewersObj).optional().or("users", "teams"),
type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(),
type: Joi.string()
.valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct, RuleTypes.Fellows)
.required(),
});

/** General Configuration schema.
Expand All @@ -35,25 +36,31 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
*/
export const generalSchema = Joi.object<ConfigurationFile>().keys({
rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).unique("name").required(),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams", "minFellowsRank"),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("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({ ...reviewerConditionObj, countAuthor: Joi.boolean().default(false) })
.or("users", "teams", "minFellowsRank");
.or("users", "teams");

/** As, with the exception of basic, every other schema has the same structure, we can recycle this */
export const otherRulesSchema = Joi.object<AndRule>().keys({
reviewers: Joi.array<AndRule["reviewers"]>()
.items(Joi.object<Reviewers>().keys(reviewerConditionObj).or("users", "teams", "minFellowsRank"))
.items(Joi.object<Reviewers>().keys(reviewerConditionObj).or("users", "teams"))
.min(2)
.required(),
countAuthor: Joi.boolean().default(false),
});

export const fellowsRuleSchema = Joi.object<FellowsRule>().keys({
countAuthor: Joi.boolean().default(false),
minRank: Joi.number().required().min(1).empty(null),
min_approvals: Joi.number().min(1).default(1),
});

/**
* Evaluates a config thoroughly. If there is a problem with it, it will throw.
*
Expand All @@ -77,6 +84,9 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n
// Aside from the type, every other field in this rules is identical so we can
// use any of these rules to validate the other fields.
validatedConfig.rules[i] = validate<AndRule>(rule, otherRulesSchema, { message });
} else if (type === "fellows") {
// Fellows have a specific config that uses ranks instead of usernames
validatedConfig.rules[i] = validate<FellowsRule>(rule, fellowsRuleSchema, { message });
} else {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
throw new Error(`Rule ${name} has an invalid type: ${type}`);
Expand Down
Loading
Loading