From f253d2c9afce728d150199f0738e8e53b62589e7 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 25 Jul 2023 21:19:08 +0200 Subject: [PATCH] Handle not enough reviewers (#29) Added validation that the amount of available users must not be smaller than the amount of required reviews. Closes #27 Also fixed a small security vulnerability in a dependency. --- src/runner.ts | 45 +++++++++++++++++++++--------- src/test/runner/conditions.test.ts | 23 ++++++++++++++- yarn.lock | 7 +---- 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index bdbabae..276cb2e 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -54,20 +54,26 @@ export class ActionRunner { */ async validatePullRequest({ rules }: ConfigurationFile): Promise { for (const rule of rules) { - // We get all the files that were modified and match the rules condition - const files = await this.listFilesThatMatchRuleCondition(rule); - // We check if there are any matches - if (files.length === 0) { - this.logger.debug(`Skipping rule ${rule.name} as no condition matched`); - // If there are no matches, we simply skip the check - continue; - } - if (rule.type === "basic") { - const [result, missingData] = await this.evaluateCondition(rule); - if (!result) { - this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - return false; + try { + // We get all the files that were modified and match the rules condition + const files = await this.listFilesThatMatchRuleCondition(rule); + // We check if there are any matches + if (files.length === 0) { + this.logger.debug(`Skipping rule ${rule.name} as no condition matched`); + // If there are no matches, we simply skip the check + continue; + } + if (rule.type === "basic") { + const [result, missingData] = await this.evaluateCondition(rule); + if (!result) { + this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); + return false; + } } + } catch (error: unknown) { + // We only throw if there was an unexpected error, not if the check fails + this.logger.error(`Rule ${rule.name} failed with error`); + throw error; } } @@ -102,6 +108,19 @@ export class ActionRunner { throw new Error("Teams and Users field are not set for rule."); } + if (requiredUsers.length < rule.min_approvals) { + this.logger.error( + `${rule.min_approvals} approvals are required but only ${requiredUsers.length} user's approval count.`, + ); + if (rule.teams) { + this.logger.error(`Allowed teams: ${JSON.stringify(rule.teams)}`); + } + if (rule.users) { + this.logger.error(`Allowed users: ${JSON.stringify(rule.users)}`); + } + throw new Error("The amount of required approvals is smaller than the amount of available users."); + } + // We get the list of users that approved the PR const approvals = await this.prApi.listApprovedReviewsAuthors(); diff --git a/src/test/runner/conditions.test.ts b/src/test/runner/conditions.test.ts index 7a36b7b..3a3b71b 100644 --- a/src/test/runner/conditions.test.ts +++ b/src/test/runner/conditions.test.ts @@ -23,6 +23,27 @@ describe("evaluateCondition tests", () => { ); }); + test("should throw if not enough users are available", async () => { + await expect(runner.evaluateCondition({ min_approvals: 5, users: ["one-user"] })).rejects.toThrow( + "The amount of required approvals is smaller than the amount of available users.", + ); + }); + + test("should throw if not enough users in teams are available", async () => { + teamsApi.getTeamMembers.mockResolvedValue(["1", "2", "3"]); + await expect(runner.evaluateCondition({ min_approvals: 4, teams: ["etcetera"] })).rejects.toThrow( + "The amount of required approvals is smaller than the amount of available users.", + ); + }); + + test("should throw if not enough users in teams are available and find duplicates", async () => { + teamsApi.getTeamMembers.calledWith("a").mockResolvedValue(["1", "2", "3"]); + teamsApi.getTeamMembers.calledWith("b").mockResolvedValue(["2", "3", "4"]); + await expect(runner.evaluateCondition({ min_approvals: 5, teams: ["a", "b"] })).rejects.toThrow( + "The amount of required approvals is smaller than the amount of available users.", + ); + }); + describe("users tests", () => { const users = ["user-1", "user-2", "user-3"]; beforeEach(() => { @@ -104,7 +125,7 @@ describe("evaluateCondition tests", () => { teamsApi.getTeamMembers.calledWith(team1.name).mockResolvedValue(team1.users); teamsApi.getTeamMembers.calledWith(team2.name).mockResolvedValue(team2.users); api.listApprovedReviewsAuthors.mockResolvedValue([]); - const [result, report] = await runner.evaluateCondition({ min_approvals: 4, teams: [team1.name, team2.name] }); + const [result, report] = await runner.evaluateCondition({ min_approvals: 3, teams: [team1.name, team2.name] }); expect(result).toBeFalsy(); // Should not send required users more than once expect(report?.missingUsers).toEqual([...team1.users, team2.users[0]]); diff --git a/yarn.lock b/yarn.lock index 02e060a..e8ba85d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3315,16 +3315,11 @@ safe-regex-test@^1.0.0: get-intrinsic "^1.1.3" is-regex "^1.1.4" -semver@^6.0.0: +semver@^6.0.0, semver@^6.3.0: version "6.3.1" resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.1.tgz#556d2ef8689146e46dcea4bfdd095f3434dffcb4" integrity sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA== -semver@^6.3.0: - version "6.3.0" - resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d" - integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw== - semver@^7.3.7, semver@^7.5.3: version "7.5.4" resolved "https://registry.yarnpkg.com/semver/-/semver-7.5.4.tgz#483986ec4ed38e1c6c48c34894a9182dbff68a6e"