Skip to content

Commit

Permalink
GH Action requiring multiple internal approvers
Browse files Browse the repository at this point in the history
Fixes #364 and #361.

This action is meant to replace the workflow of the same name. A GH
Action had to be used to facilitate passing a tokens from
abcxyz/github-token-minter (or any other action that produces a GH token). This
is because of the security ramification of passing secret tokens between GH
Workflow jobs.

**Deprecating the multi-approvers.yml workflow**

The legacy multi-approvers.yml workflow will be deprecated as a
follow-up to this PR. We will first need to migrate any users off of the
legacy workflow before deletion.

**Testing**

This workflow has been manually tested using the abcxyz/actions-testing
repository. Both the pull_request and pull_request_review workflows have
been tested.

Automated tests are included in this PR testing input validation and the
multi-approver logic. Run `npm run tests` to execute the tests.

A mix of Jest's mocking capabilities as well as a fake Octokit
(`__tests__/main.test.js:FakeOctokit`) are used to facilitate testing. Test
scenarios are stored in `__tests__/data`. Scenarios include three main parts:

1. Inputs
2. Github context
3. Octokit model

Each scenario is stored in a single file (e.g. `__tests__/data/internal-pr.json`).

The logic for re-validating failed runs after a pull_request_review
event is **not tested** in the automated tests (though this has been
tested manually). Testing for this is possible, but would require a fair amount
of extra functionality in the fake octokit (`__tests__/main.test.js:FakeOctokit`)
class. This can be done in a follow-up PR.

Finally, these tests should be added to the repository's CI/CD workflow. This
should be done in a follow-up PR.

**Follow-up items**

Most of these items are mentioned above.

* Deprecate multi-approvers.yml workflow
* Add tests for pull_request_review events
* Add tests to presubmit CI/CD workflow
* Add build-min to CI/CD workflow

An issue will be created for each of these before merging this PR into
main.
  • Loading branch information
ian-shafer committed Feb 25, 2025
1 parent 4ad0ef4 commit 77f6a72
Show file tree
Hide file tree
Showing 20 changed files with 4,966 additions and 30 deletions.
1 change: 1 addition & 0 deletions .github/actions/multi-approvers/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules/
16 changes: 16 additions & 0 deletions .github/actions/multi-approvers/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Multi-approvers GitHub Action

This GitHub Action requires at least 2 internal approvers for pull requests with
external authors.

Internal approvers are GitHub users who are members of the given team. All other
users are external.

## Development

`npm install`: Downloads required node packages.

`npm run build-min`: Generates minimized versions of Javascript source code.
This MUST be run after making changes to code under the `src` directory.

`npm run tests`: runs tests.
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
{
"inputs": {
"token": "fake-token",
"team": "fake-team"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
},
"model": {
"pulls": [
{
"owner": "test-org",
"pull_number": 1,
"repo": "fake-repository",
"user": {
"login": "a-user"
},
"reviews": [
{
"state": "APPROVED",
"submitted_at": 1,
"user": {
"login": "approver-1"
}
},
{
"state": "COMMENTED",
"submitted_at": 2,
"user": {
"login": "approver-2"
}
}
]
}
],
"teams": [
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-1",
"role": "member",
"state": "active"
},
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-2",
"role": "member",
"state": "active"
}
]
}
}
47 changes: 47 additions & 0 deletions .github/actions/multi-approvers/__tests__/data/internal-pr.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"inputs": {
"token": "fake-token",
"team": "fake-team"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
},
"model": {
"pulls": [
{
"owner": "test-org",
"pull_number": 1,
"repo": "fake-repository",
"user": {
"login": "a-user"
}
}
],
"teams": [
{
"org": "test-org",
"team_slug": "fake-team",
"username": "a-user",
"role": "member",
"state": "active"
}
]
}
}
24 changes: 24 additions & 0 deletions .github/actions/multi-approvers/__tests__/data/no-inputs.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"inputs": {
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"inputs": {
"token": "fake-token",
"team": "fake-team"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
},
"model": {
"pulls": [
{
"owner": "test-org",
"pull_number": 1,
"repo": "fake-repository",
"user": {
"login": "a-user"
}
}
],
"teams": [
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
{
"inputs": {
"token": "fake-token",
"team": "fake-team"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
},
"model": {
"pulls": [
{
"owner": "test-org",
"pull_number": 1,
"repo": "fake-repository",
"user": {
"login": "a-user"
},
"reviews": [
{
"state": "APPROVED",
"submitted_at": 1,
"user": {
"login": "approver-1"
}
},
{
"state": "APPROVED",
"submitted_at": 2,
"user": {
"login": "approver-2"
}
}
]
}
],
"teams": [
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-1",
"role": "member",
"state": "active"
},
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-2",
"role": "member",
"state": "pending"
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
{
"inputs": {
"token": "fake-token",
"team": "fake-team"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
},
"model": {
"pulls": [
{
"owner": "test-org",
"pull_number": 1,
"repo": "fake-repository",
"user": {
"login": "a-user"
},
"reviews": [
{
"state": "APPROVED",
"submitted_at": 1,
"user": {
"login": "approver-1"
}
},
{
"state": "APPROVED",
"submitted_at": 2,
"user": {
"login": "approver-2"
}
},
{
"state": "REQUEST_CHANGES",
"submitted_at": 3,
"user": {
"login": "approver-1"
}
}
]
}
],
"teams": [
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-1",
"role": "member",
"state": "active"
},
{
"org": "test-org",
"team_slug": "fake-team",
"username": "approver-2",
"role": "member",
"state": "active"
}
]
}
}
25 changes: 25 additions & 0 deletions .github/actions/multi-approvers/__tests__/data/team-not-set.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"inputs": {
"token": "a-fake-token"
},
"github": {
"context": {
"eventName": "pull_request",
"runId": 1,
"payload": {
"pull_request": {
"number": 1,
"head": {
"ref": "fake-branch"
}
},
"repository": {
"name": "fake-repository",
"owner": {
"login": "test-org"
}
}
}
}
}
}
Loading

0 comments on commit 77f6a72

Please sign in to comment.