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 feature to request reviewers #97

Merged
merged 11 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 10 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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ on:
permissions:
contents: read
checks: write
pull-requests: write

jobs:
review-approvals:
Expand All @@ -103,6 +104,7 @@ jobs:
repo-token: ${{ github.token }}
team-token: ${{ secrets.TEAM_TOKEN }}
checks-token: ${{ secrets.CHECKS_TOKEN }}
request-reviewers: false
pr-number: ${{ steps.number.outputs.content }}
```
Create a new PR and see if it is working.
Expand Down Expand Up @@ -149,6 +151,10 @@ You can find all the inputs in [the action file](./action.yml), but let's walk t
- You can use the same GitHub app for `checks-token` and `team-token`.
- `config-file`: The location of the config file.
- **default**: `.github/review-bot.yml`
- `request-reviewers`: If the system should automatically request the required reviewers.
- **default**: false.
- If enabled, when there are missing reviews, the system will request the appropriate users and/or team to review.
Bullrich marked this conversation as resolved.
Show resolved Hide resolved
- If enabled, and using teams, this requires a GitHub action with `write` permission for `pull request`.

#### Using a GitHub app instead of a PAT
In some cases, specially in big organizations, it is more organized to use a GitHub app to authenticate, as it allows us to give it permissions per repository, and we can fine-grain them even better. If you wish to do that, you need to create a GitHub app with the following permissions:
Expand Down
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ inputs:
pr-number:
description: 'The number of the pull request to review. Required if event is `workflow_run`'
required: false
request-reviewers:
description: If the system should automatically request the required reviewers.
required: false
outputs:
repo:
description: 'The name of the repo in owner/repo pattern'
Expand Down
27 changes: 26 additions & 1 deletion src/github/pullRequest.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { PullRequest, PullRequestReview } from "@octokit/webhooks-types";

import { Reviewers } from "../rules/types";
import { caseInsensitiveEqual } from "../util";
import { ActionLogger, GitHubClient } from "./types";

/** API class that uses the default token to access the data from the pull request and the repository */
/** API class that uses the default token to access the data from the pull request and the repository
* If we are using the assign reviewers features with teams, it requires a GitHub app
* (Action token doesn't have permission to assign teams)
*/
export class PullRequestApi {
private readonly number: number;
private readonly repoInfo: { repo: string; owner: string };
Expand Down Expand Up @@ -107,6 +111,27 @@ export class PullRequestApi {
return approvals;
}

async requestReview({ users, teams }: Pick<Reviewers, "users" | "teams">): Promise<void> {
if (users || teams) {
const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0;
const reviewersLog = [
validArray(users) ? `Users: ${JSON.stringify(users)}` : undefined,
validArray(teams) ? `Teams: ${JSON.stringify(teams)}` : undefined,
]
.filter((e) => !!e)
.join(" - ");

this.logger.info(`Requesting reviews from ${reviewersLog}`);

await this.api.rest.pulls.requestReviewers({
...this.repoInfo,
pull_number: this.number,
reviewers: users,
team_reviewers: teams,
});
}
}

/** Returns the login of the PR's author */
getAuthor(): string {
return this.pr.user.login;
Expand Down
5 changes: 4 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export interface Inputs {
configLocation: string;
/** GitHub's action default secret */
repoToken: string;
/** Should automatically request missing reviewers */
requestReviewers?: boolean;
/** A custom access token with the read:org access */
teamApiToken: string;
/** Number of the PR to analyze. Optional when it is triggered by `pull_request` event */
Expand All @@ -38,10 +40,11 @@ const getRepo = (ctx: Context) => {
const getInputs = (): Inputs => {
const configLocation = getInput("config-file");
const repoToken = getInput("repo-token", { required: true });
const requestReviewers = !!getInput("request-reviewers", { required: false });
const teamApiToken = getInput("team-token", { required: true });
const prNumber = getInput("pr-number");

return { configLocation, repoToken, teamApiToken, prNumber: prNumber ? parseInt(prNumber) : null };
return { configLocation, requestReviewers, repoToken, teamApiToken, prNumber: prNumber ? parseInt(prNumber) : null };
};

const repo = getRepo(context);
Expand Down
25 changes: 15 additions & 10 deletions src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,10 @@ export class ActionRunner {
}

/** WIP - Class that will assign the requests for review */
requestReviewers(reports: RuleReport[], preventReviewRequests: ConfigurationFile["preventReviewRequests"]): void {
async requestReviewers(
reports: RuleReport[],
preventReviewRequests: ConfigurationFile["preventReviewRequests"],
): Promise<void> {
if (reports.length === 0) {
return;
}
Expand All @@ -191,6 +194,8 @@ export class ActionRunner {
finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest);
}

this.logger.debug(`Request data: ${JSON.stringify(finalReport)}`);

let { teamsToRequest, usersToRequest } = finalReport;

/**
Expand All @@ -214,13 +219,7 @@ export class ActionRunner {
}
}

const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0;
const reviewersLog = [
validArray(teamsToRequest) ? `Teams: ${JSON.stringify(teamsToRequest)}` : "",
validArray(usersToRequest) ? `Users: ${JSON.stringify(usersToRequest)}` : "",
].join(" - ");

this.logger.info(`Need to request reviews from ${reviewersLog}`);
await this.prApi.requestReview({ users: usersToRequest, teams: teamsToRequest });
}

/** Aggregates all the reports and generate a status report
Expand Down Expand Up @@ -556,7 +555,9 @@ export class ActionRunner {
* 3. It generates a status check in the Pull Request
* 4. WIP - It assigns the required reviewers to review the PR
*/
async runAction(inputs: Pick<Inputs, "configLocation">): Promise<Pick<CheckData, "conclusion"> & PullRequestReport> {
async runAction(
inputs: Pick<Inputs, "configLocation" | "requestReviewers">,
): Promise<Pick<CheckData, "conclusion"> & PullRequestReport> {
const config = await this.getConfigFile(inputs.configLocation);

const prValidation = await this.validatePullRequest(config);
Expand All @@ -567,7 +568,11 @@ export class ActionRunner {
const checkRunData = this.generateCheckRunData(reports);
await this.checks.generateCheckRun(checkRunData);

this.requestReviewers(reports, config.preventReviewRequests);
if (inputs.requestReviewers) {
await this.requestReviewers(reports, config.preventReviewRequests);
} else {
this.logger.info("'request-reviewers' is disabled. Skipping the request.");
}

setOutput("report", JSON.stringify(prValidation));

Expand Down
11 changes: 11 additions & 0 deletions src/test/github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,15 @@ describe("Pull Request API Tests", () => {
expect(client.rest.pulls.listFiles).toHaveBeenCalledTimes(1);
});
});

test("Request reviewers", async () => {
await api.requestReview({ users: ["abc"], teams: ["team-abc"] });
expect(client.rest.pulls.requestReviewers).toHaveBeenCalledWith({
pull_number: 99,
owner: "org",
repo: pr.base.repo.name,
reviewers: ["abc"],
team_reviewers: ["team-abc"],
});
});
});
29 changes: 13 additions & 16 deletions src/test/runner/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,30 +102,27 @@ describe("Shared validations", () => {
usersToRequest: ["user-1"],
};

test("should request reviewers if object is not defined", () => {
runner.requestReviewers([exampleReport], undefined);
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
test("should request reviewers if object is not defined", async () => {
await runner.requestReviewers([exampleReport], undefined);
expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] });
});

test("should not request user if he is defined", () => {
runner.requestReviewers([exampleReport], { users: ["user-1"] });
test("should not request user if he is defined", async () => {
await runner.requestReviewers([exampleReport], { users: ["user-1"] });

expect(logger.info).toHaveBeenCalledWith("Filtering users to request a review from.");
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
expect(api.requestReview).toHaveBeenCalledWith({ teams: ["team-1"], users: [] });
});

test("should not request team if it is defined", () => {
runner.requestReviewers([exampleReport], { teams: ["team-1"] });
test("should not request team if it is defined", async () => {
await runner.requestReviewers([exampleReport], { teams: ["team-1"] });
expect(logger.info).toHaveBeenCalledWith("Filtering teams to request a review from.");
expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
expect(api.requestReview).toHaveBeenCalledWith({ teams: [], users: ["user-1"] });
});

test("should request reviewers if the team and user are not the same", () => {
runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] });
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
test("should request reviewers if the team and user are not the same", async () => {
await runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] });
expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] });
});
});
});
Loading