Skip to content

Commit

Permalink
Added required score grouping to fellows reviews (#113)
Browse files Browse the repository at this point in the history
Added new optional field to the config named `FellowScores` that has the
score of each fellow per dan. It was done as an object (and not a
collection) because it will never change in the foreseeable future
(added up to dan IX).

When the rule of type `fellows` has the optional field of
`minTotalScore` it will check that the score of the fellows who approved
the PR sums up to that number. If it doesn't sum to that number it will
fail with a custom error listing the score of each fellow for reference
(and saying how many points are missing).

Added `FellowMissingScoreFailure` class which handles the formatting of
errors when the score of fellows is not high enough.

This resolves #110

Downgraded JOI as there was a version mismatch that made the test fails.

This also updated the version to `2.4.0`
  • Loading branch information
Bullrich authored Jan 29, 2024
1 parent 5371807 commit 2800183
Show file tree
Hide file tree
Showing 24 changed files with 377 additions and 71 deletions.
42 changes: 30 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -411,21 +411,37 @@ Meaning that if a user belongs to both `team-abc` and `team-example` their appro

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:
The fellows rule requires an extra collection to be added to the configuration and distinguishes itself from the other rules with some new fields:
```yaml
- name: Fellows review
condition:
include:
- '.*'
exclude:
- 'example'
type: fellows
minRank: 2
minApprovals: 2
rules:
- name: Fellows review
condition:
include:
- '.*'
exclude:
- 'example'
type: fellows
minRank: 2
minApprovals: 2
minTotalScore: 4
scores:
dan1: 1
dan2: 2
dan3: 4
dan4: 8
dan5: 12
dan6: 15
dan7: 20
dan8: 25
dan9: 30
```
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.
The biggest difference in the rule configuration is that it doesn’t have a reviewers type (it doesn’t have a `teams` or `users` field); instead, it has a `minRank` field and `minTotalScore` field.

The `minRank` 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).

The `minTotalScore` field relies on the `scores` collection. In this collection, each rank (dan) receives a score, so, in the example, a fellow dan 3 will have a score of 4. If the field `minTotalScore` is enabled, the conditions to approve a pull request, aside from requiring a given amount of reviews from a given rank, will be that _the total sum of all the scores from the fellows that have approved the pull request are equal or greater to the `minTotalScore` field_. This field is optional.

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).
For *every dan that has not been attributed a score, their score will default to 0*.

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

Expand All @@ -439,6 +455,8 @@ It also has any other field from the [`basic rule`](#basic-rule) (with the excep
- **Optional**: Defaults to `false`.
- **minRank**: Must be a number.
- **Required**
- **minTotalScore**: Must be a number
- **Optional**: Defaults to 0.

##### Note
The fellows rule will never request reviewers, even if `request-reviewers` rule is enabled.
Expand Down
2 changes: 1 addition & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ outputs:

runs:
using: 'docker'
image: 'Dockerfile'
image: 'docker://ghcr.io/paritytech/review-bot/action:2.4.0'
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "review-bot",
"version": "2.3.1",
"version": "2.4.0",
"description": "Have custom review rules for PRs with auto assignment",
"main": "src/index.ts",
"scripts": {
Expand Down Expand Up @@ -39,7 +39,7 @@
"@actions/github": "^6.0.0",
"@eng-automation/js": "^1.0.2",
"@polkadot/api": "^10.11.2",
"joi": "^17.12.0",
"joi": "^17.6.4",
"yaml": "^2.3.4"
}
}
61 changes: 61 additions & 0 deletions src/failures/fellowsRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,64 @@ export class FellowMissingRankFailure extends ReviewFailure {
return { users: [], teams: [] };
}
}

export class FellowMissingScoreFailure extends ReviewFailure {
public readonly currentScore: number;
constructor(
ruleInfo: Omit<RuleFailedSummary, "missingUsers" | "countingReviews" | "missingReviews">,
public readonly requiredScore: number,
approvalsWithScores: [string, number][],
missingFellowsWithScore: [string, number][],
) {
const unifyFellowWithScore = ([handle, score]: [string, number]) => `${handle} -> <b>${score}</b>`;
super({
...ruleInfo,
countingReviews: approvalsWithScores.map(unifyFellowWithScore),
missingUsers: missingFellowsWithScore.map(unifyFellowWithScore),
missingReviews: 1,
});

this.currentScore = approvalsWithScores.reduce((n, [_, score]) => n + score, 0);
}

generateSummary(): typeof summary {
let text = summary
.emptyBuffer()
.addHeading(this.name, 2)
.addHeading("Missing minimum required score from Fellows", 4)
.addDetails(
"Rule explanation",
"Rule 'Fellows' gives every fellow a score based on their rank, and required that the sum of all the scores is greater than the required score." +
"\n\n" +
"For more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)",
);

text = text
.addHeading(`Missing a score of ${this.requiredScore}`, 3)
.addEOL()
.addRaw(`Missing ${this.requiredScore - this.currentScore} in the required score.`)
.addEOL()
.addRaw(`Current score is ${this.currentScore}/${this.requiredScore}`);
if (this.missingUsers.length > 0)
text = text.addDetails(
"GitHub users whose approval counts",
`This is a list of all the Fellows that have not reviewed with their current scores:\n\n - ${this.missingUsers
.map(toHandle)
.join("\n - ")}`,
);

if (this.countingReviews.length > 0) {
text = text
.addHeading("Users approvals that counted towards this rule with their scores", 3)
.addEOL()
.addList(this.countingReviews.map(toHandle))
.addEOL();
}

return text;
}

getRequestLogins(): { users: string[]; teams: string[] } {
return { users: [], teams: [] };
}
}
14 changes: 13 additions & 1 deletion src/polkadot/fellows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export class PolkadotFellows implements TeamApi {

constructor(private readonly logger: ActionLogger) {}

async fetchAllFellows(): Promise<Map<string, number>> {
private async fetchAllFellows(): Promise<Map<string, number>> {
let api: ApiPromise;
this.logger.debug("Connecting to collective parachain");
// we connect to the collective rpc node
Expand Down Expand Up @@ -91,6 +91,18 @@ export class PolkadotFellows implements TeamApi {
}
}

/** Returns all the fellows with their rankings */
async listFellows(): Promise<[string, number][]> {
this.logger.info("Fetching all fellows with their ranks");

if (this.fellowsCache.size < 1) {
this.logger.debug("Cache not found. Fetching fellows.");
this.fellowsCache = await this.fetchAllFellows();
}

return Array.from(this.fellowsCache.entries());
}

async getTeamMembers(ranking: string): Promise<string[]> {
const requiredRank = Number(ranking);
this.logger.info(`Fetching members of rank '${requiredRank}' or higher`);
Expand Down
14 changes: 14 additions & 0 deletions src/rules/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,19 @@ export interface FellowsRule extends Rule {
type: RuleTypes.Fellows;
minRank: number;
minApprovals: number;
minTotalScore?: number;
}

export interface FellowsScore {
dan1: number;
dan2: number;
dan3: number;
dan4: number;
dan5: number;
dan6: number;
dan7: number;
dan8: number;
dan9: number;
}

export interface ConfigurationFile {
Expand All @@ -49,4 +62,5 @@ export interface ConfigurationFile {
teams?: string[];
users?: string[];
};
score?: FellowsScore;
}
17 changes: 16 additions & 1 deletion src/rules/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js";
import Joi from "joi";

import { ActionLogger } from "../github/types";
import { AndRule, BasicRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./types";
import { AndRule, BasicRule, ConfigurationFile, FellowsRule, FellowsScore, 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
Expand Down Expand Up @@ -30,13 +30,27 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
.required(),
});

/** Schema for ensuring that all the dan ranks are set properly */
export const fellowScoreSchema = Joi.object<FellowsScore>().keys({
dan1: Joi.number().default(0),
dan2: Joi.number().default(0),
dan3: Joi.number().default(0),
dan4: Joi.number().default(0),
dan5: Joi.number().default(0),
dan6: Joi.number().default(0),
dan7: Joi.number().default(0),
dan8: Joi.number().default(0),
dan9: Joi.number().default(0),
});

/** 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).unique("name").required(),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"),
score: fellowScoreSchema,
});

/** Basic rule schema
Expand All @@ -59,6 +73,7 @@ export const fellowsRuleSchema = Joi.object<FellowsRule>().keys({
countAuthor: Joi.boolean().default(false),
minRank: Joi.number().required().min(1).empty(null),
minApprovals: Joi.number().min(1).default(1),
minTotalScore: Joi.number().min(1).optional(),
});

/**
Expand Down
76 changes: 65 additions & 11 deletions src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Inputs } from ".";
import {
CommonRuleFailure,
FellowMissingRankFailure,
FellowMissingScoreFailure,
RequiredReviewersData,
ReviewFailure,
RuleFailedReport,
Expand All @@ -13,9 +14,18 @@ import {
import { GitHubChecksApi } from "./github/check";
import { PullRequestApi } from "./github/pullRequest";
import { ActionLogger, CheckData, TeamApi } from "./github/types";
import { AndDistinctRule, ConfigurationFile, FellowsRule, Reviewers, Rule, RuleTypes } from "./rules/types";
import { PolkadotFellows } from "./polkadot/fellows";
import {
AndDistinctRule,
ConfigurationFile,
FellowsRule,
FellowsScore,
Reviewers,
Rule,
RuleTypes,
} from "./rules/types";
import { validateConfig, validateRegularExpressions } from "./rules/validator";
import { concatArraysUniquely } from "./util";
import { concatArraysUniquely, rankToScore } from "./util";

type BaseRuleReport = RuleFailedReport & RequiredReviewersData;

Expand All @@ -31,7 +41,7 @@ export class ActionRunner {
constructor(
private readonly prApi: PullRequestApi,
private readonly teamApi: TeamApi,
private readonly polkadotApi: TeamApi,
private readonly polkadotApi: PolkadotFellows,
private readonly checks: GitHubChecksApi,
private readonly logger: ActionLogger,
) {}
Expand Down Expand Up @@ -61,7 +71,7 @@ export class ActionRunner {
* The action evaluates if the rules requirements are meet for a PR
* @returns an array of error reports for each failed rule. An empty array means no errors
*/
async validatePullRequest({ rules }: ConfigurationFile): Promise<PullRequestReport> {
async validatePullRequest({ rules, score }: ConfigurationFile): Promise<PullRequestReport> {
const modifiedFiles = await this.prApi.listModifiedFiles();

const errorReports: ReviewFailure[] = [];
Expand Down Expand Up @@ -150,7 +160,7 @@ export class ActionRunner {
break;
}
case RuleTypes.Fellows: {
const fellowReviewError = await this.fellowsEvaluation(rule);
const fellowReviewError = await this.fellowsEvaluation(rule, score);
if (fellowReviewError) {
this.logger.error(`Missing the reviews from ${JSON.stringify(fellowReviewError.missingReviews)}`);
// errorReports.push({ ...missingData, name: rule.name, type: rule.type });
Expand Down Expand Up @@ -440,7 +450,7 @@ export class ActionRunner {
}
}

async fellowsEvaluation(rule: FellowsRule): Promise<ReviewFailure | null> {
async fellowsEvaluation(rule: FellowsRule, scores?: FellowsScore): Promise<ReviewFailure | null> {
// This is a list of all the users that need to approve a PR
const requiredUsers: string[] = await this.polkadotApi.getTeamMembers(rule.minRank.toString());

Expand Down Expand Up @@ -472,9 +482,10 @@ export class ActionRunner {
}
}

const author = this.prApi.getAuthor();

// Now we verify if we have any remaining missing review.
if (missingReviews > 0) {
const author = this.prApi.getAuthor();
this.logger.warn(`${missingReviews} reviews are missing.`);
// If we have at least one missing review, we return an object with the list of missing reviewers, and
// which users/teams we should request to review
Expand All @@ -487,11 +498,54 @@ export class ActionRunner {
},
rule.minRank,
);
} else {
this.logger.info("Rule requirements fulfilled");
// If we don't have any missing reviews, we return no error
return null;
// Then we verify if we need to have a minimum score
} else if (rule.minTotalScore && scores) {
this.logger.debug("Validating required minimum score");
// We get all the fellows with their ranks and convert them to their score
const fellows: [string, number][] = (await this.polkadotApi.listFellows()).map(([handle, rank]) => [
handle,
rankToScore(rank, scores),
]);

const maximumScore = fellows.reduce((a, [_, score]) => a + score, 0);
if (rule.minTotalScore > maximumScore) {
throw new Error(
`Minimum score of ${rule.minTotalScore} is higher that the obtainable score of ${maximumScore}!`,
);
}

let score = 0;

const countingFellows: [string, number][] = [];

// We iterate over all the approvals and convert their rank to their score
for (const [handle, fellowScore] of fellows) {
// We filter fellows whose score is 0
if (approvals.indexOf(handle) > -1 && fellowScore > 0) {
score += fellowScore;
countingFellows.push([handle, fellowScore]);
}
}

this.logger.debug(`Current score is ${score} and the minimum required score is ${rule.minTotalScore}`);

if (rule.minTotalScore > score) {
const missingUsers = fellows
// Remove all the fellows who score is worth 0
.filter(([_, fellowScore]) => fellowScore > 0)
// Remove the author
.filter(([handle]) => handle != author)
// Remove the approvals
.filter(([handle]) => approvals.indexOf(handle) < 0);

this.logger.warn(`Missing score of ${rule.minTotalScore} by ${score - rule.minTotalScore}`);

return new FellowMissingScoreFailure(rule, rule.minTotalScore, countingFellows, missingUsers);
}
}
this.logger.info("Rule requirements fulfilled");
// If we don't have any missing reviews, we return no error
return null;
}

/** Using the include and exclude condition, it returns a list of all the files in a PR that matches the criteria */
Expand Down
2 changes: 1 addition & 1 deletion src/test/fellows.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { mock, mockClear, MockProxy } from "jest-mock-extended";
import { ActionLogger, TeamApi } from "../github/types";
import { PolkadotFellows } from "../polkadot/fellows";

const timeout = 15_000;
const timeout = 25_000;

describe("CAPI test", () => {
let fellows: TeamApi;
Expand Down
Loading

0 comments on commit 2800183

Please sign in to comment.