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.

Sign up for GitHub

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 more details in the rule output #105

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Added more details in the rule output #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c64ff2
added a list of approvals
Bullrich Nov 29, 2023
6c53b5f
added explanation on how rules work
Bullrich Nov 29, 2023
61c42a8
fixed tests
Bullrich Nov 29, 2023
9348348
converted rule explanation into a detail
Bullrich Nov 29, 2023
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
79 changes: 69 additions & 10 deletions src/runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ type ReviewReport = {
usersToRequest?: string[];
/** If applicable, the missing minimum fellows rank required to review */
missingRank?: number;
/** If applicable, reviews that count towards this rule */
countingReviews: string[];
};

export type RuleReport = { name: string } & ReviewReport;
export type RuleReport = { name: string; type: RuleTypes } & ReviewReport;

type ReviewState = [true] | [false, ReviewReport];

Expand Down Expand Up @@ -96,7 +98,7 @@ export class ActionRunner {
const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor);
if (!result) {
this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
errorReports.push({ ...missingData, name: rule.name });
errorReports.push({ ...missingData, name: rule.name, type: rule.type });
}

break;
Expand All @@ -112,7 +114,7 @@ export class ActionRunner {
}
}
if (reports.length > 0) {
const finalReport = unifyReport(reports, rule.name);
const finalReport = unifyReport(reports, rule.name, rule.type);
this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`);
errorReports.push(finalReport);
}
Expand All @@ -139,7 +141,7 @@ export class ActionRunner {
.reduce((a, b) => (a < b ? a : b), 999);
// We get the lowest rank required
// We unify the reports
const finalReport = unifyReport(reports, rule.name);
const finalReport = unifyReport(reports, rule.name, rule.type);
// We set the value to the minimum neccesary
finalReport.missingReviews = lowerAmountOfReviewsNeeded;
this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`);
Expand All @@ -152,15 +154,15 @@ export class ActionRunner {
const [result, missingData] = await this.andDistinctEvaluation(rule);
if (!result) {
this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
errorReports.push({ ...missingData, name: rule.name });
errorReports.push({ ...missingData, name: rule.name, type: rule.type });
}
break;
}
case RuleTypes.Fellows: {
const [result, missingData] = await this.fellowsEvaluation(rule);
if (!result) {
this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`);
errorReports.push({ ...missingData, name: rule.name });
errorReports.push({ ...missingData, name: rule.name, type: rule.type });
}
break;
}
Expand All @@ -177,21 +179,27 @@ export class ActionRunner {
return { files: modifiedFiles, reports: errorReports };
}

/** WIP - Class that will assign the requests for review */
async requestReviewers(
reports: RuleReport[],
preventReviewRequests: ConfigurationFile["preventReviewRequests"],
): Promise<void> {
if (reports.length === 0) {
return;
}
const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [] };
const finalReport: ReviewReport = {
missingReviews: 0,
missingUsers: [],
teamsToRequest: [],
usersToRequest: [],
countingReviews: [],
};

for (const report of reports) {
finalReport.missingReviews += report.missingReviews;
finalReport.missingUsers = concatArraysUniquely(finalReport.missingUsers, report.missingUsers);
finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, report.teamsToRequest);
finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest);
finalReport.countingReviews = concatArraysUniquely(finalReport.countingReviews, report.countingReviews);
}

this.logger.debug(`Request data: ${JSON.stringify(finalReport)}`);
Expand Down Expand Up @@ -243,11 +251,38 @@ export class ActionRunner {
}

for (const report of reports) {
const ruleExplanation = (type: RuleTypes) => {
switch (type) {
case RuleTypes.Basic:
return "Rule 'Basic' requires a given amount of reviews from users/teams";
case RuleTypes.And:
return "Rule 'And' has many required reviewers/teams and requires all of them to be fulfilled.";
case RuleTypes.Or:
return "Rule 'Or' has many required reviewers/teams and requires at least one of them to be fulfilled.";
case RuleTypes.AndDistinct:
return (
"Rule 'And Distinct' has many required reviewers/teams and requires all of them to be fulfilled **by different users**.\n\n" +
"The approval of one user that belongs to _two teams_ will count only towards one team."
);
case RuleTypes.Fellows:
return "Rule 'Fellows' requires a given amount of reviews from users whose Fellowship ranking is the required rank or great.";
default:
console.error("Out of range for rule type", type);
return "Unhandled rule";
}
};

check.output.summary += `- **${report.name}**\n`;
let text = summary
.emptyBuffer()
.addHeading(report.name, 2)
.addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4);
.addHeading(`Missing ${report.missingReviews} review${report.missingReviews > 1 ? "s" : ""}`, 4)
.addDetails(
"Rule explanation",
`${ruleExplanation(
report.type,
)}\n\nFor more info found out how the rules work in [Review-bot types](https://github.com/paritytech/review-bot#types)`,
);
if (report.usersToRequest && report.usersToRequest.length > 0) {
text = text
.addHeading("Missing users", 3)
Expand All @@ -263,6 +298,13 @@ export class ActionRunner {
.addRaw(`Missing reviews from rank \`${report.missingRank}\` or above`)
.addEOL();
}
if (report.countingReviews.length > 0) {
text = text
.addHeading("Users approvals that counted towards this rule", 3)
.addEOL()
.addList(report.countingReviews)
.addEOL();
}

check.output.text += text.stringify() + "\n";
}
Expand All @@ -289,6 +331,8 @@ export class ActionRunner {
// We get the list of users that approved the PR
const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false);

let countingReviews: string[] = [];

// Utility method used to generate error
const generateErrorReport = (): ReviewReport => {
const filterMissingUsers = (reviewData: { users?: string[] }[]): string[] =>
Expand All @@ -301,6 +345,7 @@ export class ActionRunner {
missingUsers: filterMissingUsers(requirements),
teamsToRequest: rule.reviewers.flatMap((r) => r.teams ?? []),
usersToRequest: filterMissingUsers(rule.reviewers),
countingReviews,
};
};

Expand All @@ -327,6 +372,8 @@ export class ActionRunner {
}
this.logger.debug(`Matching approvals: ${JSON.stringify(conditionApprovals)}`);

countingReviews = [...new Set(conditionApprovals.flatMap(({ matchingUsers }) => matchingUsers))];

// If one of the rules doesn't have the required approval we fail the evaluation
if (conditionApprovals.some((cond) => cond.matchingUsers.length === 0)) {
this.logger.warn("One of the groups does not have any approvals");
Expand Down Expand Up @@ -421,12 +468,16 @@ export class ActionRunner {
const approvals = await this.prApi.listApprovedReviewsAuthors(countAuthor ?? false);
this.logger.info(`Found ${approvals.length} approvals.`);

// List of user reviews which fulfill this rule
const countingReviews: string[] = [];

// This is the amount of reviews required. To succeed this should be 0 or lower
let missingReviews = rule.minApprovals;
for (const requiredUser of requiredUsers) {
// We check for the approvals, if it is a required reviewer we lower the amount of missing reviews
if (approvals.indexOf(requiredUser) > -1) {
missingReviews--;
countingReviews.push(requiredUser);
}
}

Expand All @@ -444,6 +495,7 @@ export class ActionRunner {
missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author),
teamsToRequest: rule.teams ? rule.teams : undefined,
usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined,
countingReviews,
},
];
} else {
Expand Down Expand Up @@ -472,12 +524,16 @@ export class ActionRunner {
const approvals = await this.prApi.listApprovedReviewsAuthors(rule.countAuthor ?? false);
this.logger.info(`Found ${approvals.length} approvals.`);

// List of user reviews which fulfill this rule
const countingReviews: string[] = [];

// This is the amount of reviews required. To succeed this should be 0 or lower
let missingReviews = rule.minApprovals;
for (const requiredUser of requiredUsers) {
// We check for the approvals, if it is a required reviewer we lower the amount of missing reviews
if (approvals.indexOf(requiredUser) > -1) {
missingReviews--;
countingReviews.push(requiredUser);
}
}

Expand All @@ -494,6 +550,7 @@ export class ActionRunner {
// Remove all the users who approved the PR + the author (if he belongs to the group)
missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author),
missingRank: rule.minRank,
countingReviews,
},
];
} else {
Expand Down Expand Up @@ -589,7 +646,7 @@ const getRequiredRanks = (reports: Pick<ReviewReport, "missingRank">[]): number[
}
};

const unifyReport = (reports: ReviewReport[], name: string): RuleReport => {
const unifyReport = (reports: ReviewReport[], name: string, type: RuleTypes): RuleReport => {
const ranks = getRequiredRanks(reports);
const missingRank = ranks ? Math.max(...(ranks as number[])) : undefined;

Expand All @@ -599,6 +656,8 @@ const unifyReport = (reports: ReviewReport[], name: string): RuleReport => {
teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))],
usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))],
name,
type,
missingRank,
countingReviews: [...new Set(reports.flatMap((r) => r.countingReviews))],
};
};
6 changes: 4 additions & 2 deletions src/test/runner/runner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { GitHubChecksApi } from "../../github/check";
import { PullRequestApi } from "../../github/pullRequest";
import { ActionLogger, TeamApi } from "../../github/types";
import { ConfigurationFile, Rule, RuleTypes } from "../../rules/types";
import { ActionRunner } from "../../runner";
import { ActionRunner, RuleReport } from "../../runner";

describe("Shared validations", () => {
let api: MockProxy<PullRequestApi>;
Expand Down Expand Up @@ -94,12 +94,14 @@ describe("Shared validations", () => {
});

describe("Validation in requestReviewers", () => {
const exampleReport = {
const exampleReport: RuleReport = {
name: "Example",
missingUsers: ["user-1", "user-2", "user-3"],
missingReviews: 2,
teamsToRequest: ["team-1"],
usersToRequest: ["user-1"],
type: RuleTypes.Basic,
countingReviews: [],
};

test("should request reviewers if object is not defined", async () => {
Expand Down
Loading