Skip to content

Commit

Permalink
Added rule type 'fellows' (#85)
Browse files Browse the repository at this point in the history
Created a new type of rule named `fellows`. 

This rule has its own evaluation (currently, it is mostly a copy of the
`basic` evaluation). This is done so we can add special cases for rule,
as _suggested by @bkchr in
polkadot-fellows/runtimes#31 (review).

Removed all the ability to request a `rank` in normal rules. I believe
that this is better as we now separate the concerns and simplify logic.
The `or`, `and` and `and-distinct` rule didn't really apply on fellows
because a higher ranking one belongs also to the lower echelons.

This resolves #84 and allows us to experiment better.

Also, this is technically a breaking change, but it has never been
applied on any system, so it wouldn't be breaking an existing system.
  • Loading branch information
Bullrich committed Sep 21, 2023
1 parent 6addf3a commit bb670b9
Show file tree
Hide file tree
Showing 13 changed files with 559 additions and 394 deletions.
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;
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

0 comments on commit bb670b9

Please sign in to comment.