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 Pull Request number as input #94

Merged
merged 9 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
# we may forget to set it back to main
- name: Validate that action points to main branch
run: |
BRANCH=$(yq '.jobs.review-approvals.steps[1].uses' $FILE_NAME | cut -d "@" -f2)
BRANCH=$(yq '.jobs.review-approvals.steps[2].uses' $FILE_NAME | cut -d "@" -f2)
# If the branch is not the main branch
if [ "$BRANCH" != "$GITHUB_BASE_REF" ]; then
echo "Action points to $BRANCH. It has to point to $GITHUB_BASE_REF instead!"
Expand Down
18 changes: 10 additions & 8 deletions .github/workflows/review-bot.yml
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
name: Review PR
on:
pull_request:
workflow_run:
workflows:
- Review-Trigger
types:
- opened
- reopened
- synchronize
- review_requested
- review_request_removed
- ready_for_review
pull_request_review:
- completed

permissions:
contents: read
Expand All @@ -18,6 +14,11 @@ jobs:
review-approvals:
runs-on: ubuntu-latest
steps:
- name: Extract content of artifact
id: number
uses: Bullrich/extract-text-from-artifact@main
with:
artifact-name: pr_number
- name: Generate token
id: team_token
uses: tibdex/github-app-token@v1
Expand All @@ -32,3 +33,4 @@ jobs:
repo-token: ${{ secrets.GITHUB_TOKEN }}
team-token: ${{ steps.team_token.outputs.token }}
checks-token: ${{ steps.team_token.outputs.token }}
pr-number: ${{ steps.number.outputs.content }}
31 changes: 31 additions & 0 deletions .github/workflows/review-trigger.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: Review-Trigger

on:
pull_request_target:
types:
- opened
- reopened
- synchronize
- review_requested
- review_request_removed
- ready_for_review
pull_request_review:

jobs:
trigger-review-bot:
runs-on: ubuntu-latest
name: trigger review bot
steps:
- name: Get PR number
env:
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
echo "Saving PR number: $PR_NUMBER"
mkdir -p ./pr
echo $PR_NUMBER > ./pr/pr_number
- uses: actions/upload-artifact@v3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about conflicts? What if there are 2 PRs opened at the same time?
They won't interfere? Can we be sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it won't be a problem because the artifact name is extracted specifically from a job.

The code that does that in Bullrich/extract-text-from-artifact is

          let allArtifacts = await github.rest.actions.listWorkflowRunArtifacts({
              owner: context.repo.owner,
              repo: context.repo.repo,
              run_id: context.payload.workflow_run.id,
          });
          let matchArtifact = allArtifacts.data.artifacts.filter((artifact) => {
            return artifact.name == process.env.ARTIFACT_NAME
          })[0];
          let download = await github.rest.actions.downloadArtifact({
              owner: context.repo.owner,
              repo: context.repo.repo,
              artifact_id: matchArtifact.id,
              archive_format: 'zip',
          });

So it makes sure to extract the artifact of the given workflow by id and not by name.

name: Save PR number
with:
name: pr_number
path: pr/
retention-days: 5
54 changes: 48 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ If one user belongs to both teams, their review will satisfy both requirements.
With the `and-distinct` rule, you can request that *two distinct users must review that file*.

## Installation
The installation requires two files for it to work, we first need to create a file (`.github/review-bot.yml` by default).
The installation requires three files for it to work, we first need to create a file (`.github/review-bot.yml` by default).
```yaml
rules:
- name: General
Expand All @@ -39,10 +39,11 @@ rules:
- your-team-name-here
```

And then we must create a second file. This will be `.github/workflows/review-bot.yml`.
The second file is the triggering file. This will be `.github/workflows/review-trigger.yml`:
```yaml
name: Review Bot
on:
name: Review-Trigger

on:
pull_request_target:
types:
- opened
Expand All @@ -53,6 +54,36 @@ on:
- ready_for_review
pull_request_review:

jobs:
trigger-review-bot:
runs-on: ubuntu-latest
name: trigger review bot
steps:
- name: Get PR number
env:
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
echo "Saving PR number: $PR_NUMBER"
mkdir -p ./pr
echo $PR_NUMBER > ./pr/pr_number
- uses: actions/upload-artifact@v3
name: Save PR number
with:
name: pr_number
path: pr/
retention-days: 5
```

And then we must create a final file. This will be `.github/workflows/review-bot.yml`.
```yaml
name: Review Bot
on:
workflow_run:
workflows:
- Review-Trigger
types:
- completed

permissions:
contents: read
checks: write
Expand All @@ -61,13 +92,18 @@ jobs:
review-approvals:
runs-on: ubuntu-latest
steps:
- name: Extract content of artifact
id: number
uses: Bullrich/extract-text-from-artifact@main
with:
artifact-name: pr_number
- name: "Evaluates PR reviews"
uses: paritytech/review-bot@main
with:
repo-token: ${{ github.token }}
team-token: ${{ secrets.TEAM_TOKEN }}
checks-token: ${{ secrets.CHECKS_TOKEN }}

pr-number: ${{ steps.number.outputs.content }}
```
Create a new PR and see if it is working.

Expand All @@ -84,6 +120,10 @@ After this, go to your branch protection rules and make sure that you have the f
### Important
Use [`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target) for the event, not `pull_request`.
- This is a security measure so that an attacker doesn’t have access to our secrets.

We use [`worflow_run`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run) to let the action be triggered at all times (If we don’t use this, GitHub will stop it if it comes from a fork).

By chaining events we are able to safely execute our action without jeopardizing our secrets. You can even use [`environment`](https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment) in the final file if you want to have it extra secure.
## Workflow Configuration
Review bot has multiple configurations available. It has available inputs and outputs. It also has rule configurations, but you can find that in the [Rule Configuration](#rule-configuration-file) section.

Expand Down Expand Up @@ -136,8 +176,10 @@ Because this project is intended to be used with a token, we need to do an extra
repo-token: ${{ github.token }}
# The previous step generates a token which is used as the input for this action
team-token: ${{ steps.generate_token.outputs.token }
checks-token: ${{ steps.generate_token.outputs.token }
checks-token: ${{ steps.generate_token.outputs.token }}
pr-number: ${{ steps.number.outputs.content }}
```

### Outputs
Outputs are needed for your chained actions. If you want to use this information, remember to set an `id` field in the step, so you can access it.

Expand Down
3 changes: 3 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ inputs:
description: 'Location of the configuration file'
required: false
default: '.github/review-bot.yml'
pr-number:
description: 'The number of the pull request to review. Required if event is `workflow_run`'
required: false
outputs:
repo:
description: 'The name of the repo in owner/repo pattern'
Expand Down
18 changes: 9 additions & 9 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@
"homepage": "https://github.com/paritytech/review-bot#readme",
"devDependencies": {
"@eng-automation/js-style": "^2.3.0",
"@octokit/webhooks-types": "^7.2.0",
"@types/jest": "^29.5.4",
"@vercel/ncc": "^0.36.1",
"jest": "^29.6.4",
"@octokit/webhooks-types": "^7.3.1",
"@types/jest": "^29.5.5",
"@vercel/ncc": "^0.38.0",
"jest": "^29.7.0",
"jest-mock-extended": "^3.0.5",
"ts-jest": "^29.1.1",
"typescript": "^5.2.2"
},
"dependencies": {
"@actions/core": "^1.10.0",
"@actions/github": "^5.1.1",
"@actions/core": "^1.10.1",
"@actions/github": "^6.0.0",
"@eng-automation/js": "^1.0.2",
"@polkadot/api": "^10.9.1",
"joi": "^17.10.0",
"yaml": "^2.3.2"
"@polkadot/api": "^10.10.1",
"joi": "^17.11.0",
"yaml": "^2.3.3"
}
}
1 change: 0 additions & 1 deletion src/github/pullRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ export class PullRequestApi {
private readonly api: GitHubClient,
private readonly pr: PullRequest,
private readonly logger: ActionLogger,
private readonly detailsUrl: string,
) {
this.number = pr.number;
this.repoInfo = { owner: this.pr.base.repo.owner.login, repo: this.pr.base.repo.name };
Expand Down
51 changes: 34 additions & 17 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { PullRequest } from "@octokit/webhooks-types";
import { GitHubChecksApi } from "./github/check";
import { PullRequestApi } from "./github/pullRequest";
import { GitHubTeamsApi } from "./github/teams";
import { CheckData } from "./github/types";
import { PolkadotFellows } from "./polkadot/fellows";
import { ActionRunner } from "./runner";
import { generateCoreLogger } from "./util";
Expand All @@ -16,6 +17,8 @@ export interface Inputs {
repoToken: string;
/** 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 */
prNumber: number | null;
}

const getRepo = (ctx: Context) => {
Expand All @@ -36,43 +39,57 @@ const getInputs = (): Inputs => {
const configLocation = getInput("config-file");
const repoToken = getInput("repo-token", { required: true });
const teamApiToken = getInput("team-token", { required: true });
const prNumber = getInput("pr-number");

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

const repo = getRepo(context);

setOutput("repo", `${repo.owner}/${repo.repo}`);

if (!context.payload.pull_request) {
throw new Error("No pull request event");
}

debug("Got payload:" + JSON.stringify(context.payload.pull_request));

const inputs = getInputs();
const runAction = async (): Promise<Pick<CheckData, "conclusion">> => {
const inputs = getInputs();

const actionId = `${context.serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${context.runId}`;

const actionInstance = getOctokit(inputs.repoToken);

const actionId = `${context.serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${context.runId}`;
let pr: PullRequest;
if (context.payload.pull_request) {
pr = context.payload.pull_request as PullRequest;
} else if (inputs.prNumber) {
debug(`Fetching pull request number #${inputs.prNumber}`);
const { data } = await actionInstance.rest.pulls.get({ ...repo, pull_number: inputs.prNumber });
pr = data as PullRequest;
} else {
throw new Error("Payload is not `pull_request` and PR number wasn't provided");
}

const api = new PullRequestApi(actionInstance, pr, generateCoreLogger());

const pr = context.payload.pull_request as PullRequest;
const logger = generateCoreLogger();

const api = new PullRequestApi(getOctokit(inputs.repoToken), pr, generateCoreLogger(), actionId);
const teamApi = new GitHubTeamsApi(getOctokit(inputs.teamApiToken), repo.owner, logger);

const logger = generateCoreLogger();
const checks = new GitHubChecksApi(getOctokit(inputs.teamApiToken), pr, logger, actionId);

const teamApi = new GitHubTeamsApi(getOctokit(inputs.teamApiToken), repo.owner, logger);
const fellows = new PolkadotFellows(logger);

const checks = new GitHubChecksApi(getOctokit(inputs.teamApiToken), pr, logger, actionId);
const runner = new ActionRunner(api, teamApi, fellows, checks, logger);

const fellows = new PolkadotFellows(logger);
const result = await runner.runAction(inputs);

const runner = new ActionRunner(api, teamApi, fellows, checks, logger);
setOutput("report", JSON.stringify(result));

return result;
};

runner
.runAction(inputs)
runAction()
.then((result) => {
info(`Action run without problem. Evaluation result was '${result.conclusion}'`);
setOutput("report", JSON.stringify(result));
})
.catch((error) => {
console.error(error);
Expand Down
2 changes: 1 addition & 1 deletion src/test/github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe("Pull Request API Tests", () => {
pr.number = 99;
pr.base.repo.owner.login = "org";

api = new PullRequestApi(client, pr, logger, "");
api = new PullRequestApi(client, pr, logger);
});

describe("Approvals", () => {
Expand Down
2 changes: 1 addition & 1 deletion src/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe("Integration testing", () => {
pr.number = 99;
pr.base.repo.owner.login = "org";

api = new PullRequestApi(client, pr, logger, "");
api = new PullRequestApi(client, pr, logger);
teams = new GitHubTeamsApi(client, "org", logger);
checks = new GitHubChecksApi(client, pr, logger, "example");
runner = new ActionRunner(api, teams, mock<TeamApi>(), checks, logger);
Expand Down
Loading
Loading