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

ci: add require multiple reviewers action #204

Merged
merged 5 commits into from
Jan 2, 2025
Merged
Show file tree
Hide file tree
Changes from 2 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
11 changes: 10 additions & 1 deletion .github/actions/require-multiple-reviewers-v1/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ A GitHub Action that requires multiple reviewers for important files
```yaml
name: Require multiple reviewers

zlayaAvocado marked this conversation as resolved.
Show resolved Hide resolved
# This must be both pull_request and pull_request_review.
# pull_request - for when the PR is initially created (or files are changed)
# pull_request_review - for when the count of reviews on a PR changes
on:
pull_request:
pull_request_review:

jobs:
require-multiple-reviewers:
runs-on: ubuntu-latest
Expand All @@ -24,10 +31,12 @@ jobs:
contents: read
zlayaAvocado marked this conversation as resolved.
Show resolved Hide resolved
checks: write
steps:
zlayaAvocado marked this conversation as resolved.
Show resolved Hide resolved
- name: Checkout repository
uses: actions/checkout@v4
- name: Require two reviewers for important files
uses: dequelabs/axe-api-team-public/.github/actions/require-multiple-reviewers-v1@main
with:
token: ${{ secrets.GITHUB_TOKEN }}
token: ${{ github.token }}
number-of-reviewers: 2
important-files-path: .github/important-files.txt
```
Expand Down
32 changes: 26 additions & 6 deletions .github/actions/require-multiple-reviewers-v1/dist/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ describe('run()', () => {
.stub(utils, 'getImportantFilesChanged')
.returns(['file1', 'file2'])

const getApproversCount = sinon.stub(utils, 'getApproversCount').returns(2)

await run(core as unknown as Core, github as unknown as GitHub)

assert.isTrue(core.setFailed.notCalled)
Expand All @@ -120,6 +122,7 @@ describe('run()', () => {
)

getImportantFilesChanged.restore()
getApproversCount.restore()
})

it('should fail if not enough reviewers', async () => {
Expand Down Expand Up @@ -180,6 +183,8 @@ describe('run()', () => {
.stub(utils, 'getImportantFilesChanged')
.returns(['file1', 'file2'])

const getApproversCount = sinon.stub(utils, 'getApproversCount').returns(0)

await run(core as unknown as Core, github as unknown as GitHub)

assert.isTrue(core.setFailed.notCalled)
Expand Down Expand Up @@ -211,6 +216,7 @@ describe('run()', () => {
)

getImportantFilesChanged.restore()
getApproversCount.restore()
})

it('should fail if number-of-reviewers is not a number', async () => {
Expand Down
10 changes: 7 additions & 3 deletions .github/actions/require-multiple-reviewers-v1/src/run.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { getAnnotations, getImportantFilesChanged } from './utils'
import {
getAnnotations,
getImportantFilesChanged,
getApproversCount
} from './utils'
import type { Conclusion, Core, GitHub } from './types'

export default async function run(core: Core, github: GitHub): Promise<void> {
Expand Down Expand Up @@ -48,8 +52,8 @@ export default async function run(core: Core, github: GitHub): Promise<void> {
pull_number: github.context.payload.pull_request!.number
})
zlayaAvocado marked this conversation as resolved.
Show resolved Hide resolved

const approvals = reviews.filter(review => review.state === 'APPROVED')
conclusion = approvals.length < numberOfReviewers ? 'failure' : 'success'
const approvals = getApproversCount(reviews)
conclusion = approvals < numberOfReviewers ? 'failure' : 'success'
}

await octokit.request('POST /repos/{owner}/{repo}/check-runs', {
Expand Down
2 changes: 2 additions & 0 deletions .github/actions/require-multiple-reviewers-v1/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ type Output = NonNullable<
export type Annotation = NonNullable<Output['annotations']>[number]
export type Conclusion =
Endpoints['POST /repos/{owner}/{repo}/check-runs']['parameters']['conclusion']
export type Review =
Endpoints['GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews']['response']['data'][0]
105 changes: 103 additions & 2 deletions .github/actions/require-multiple-reviewers-v1/src/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { expect } from 'chai'
import fs from 'fs'
import sinon from 'sinon'
import { Annotation } from './types'
import { getAnnotations, getImportantFilesChanged } from './utils'
import { Annotation, Review } from './types'
import {
getAnnotations,
getImportantFilesChanged,
getApproversCount
} from './utils'

describe('utils', () => {
describe('getAnnotations', () => {
Expand Down Expand Up @@ -177,4 +181,101 @@ describe('utils', () => {
expect(result).to.deep.equal([])
})
})

describe('getApproversCount', () => {
it('should return the correct number of approvers', () => {
const reviews = [
{
user: { login: 'user1' },
state: 'APPROVED',
submitted_at: '2023-01-01T00:00:00Z'
},
{
user: { login: 'user2' },
state: 'APPROVED',
submitted_at: '2023-01-02T00:00:00Z'
},
{
user: { login: 'user3' },
state: 'CHANGES_REQUESTED',
submitted_at: '2023-01-03T00:00:00Z'
}
]

const result = getApproversCount(reviews as Array<Review>)

expect(result).to.equal(2)
})

it('should handle multiple reviews from the same user and count only the latest approval', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

const reviews = [
{
user: { login: 'user1' },
state: 'APPROVED',
submitted_at: '2023-01-01T00:00:00Z'
},
{
user: { login: 'user1' },
state: 'CHANGES_REQUESTED',
submitted_at: '2023-01-02T00:00:00Z'
},
{
user: { login: 'user2' },
state: 'APPROVED',
submitted_at: '2023-01-03T00:00:00Z'
},
{
user: { login: 'user2' },
state: 'APPROVED',
submitted_at: '2023-01-04T00:00:00Z'
}
]

const result = getApproversCount(reviews as Array<Review>)

expect(result).to.equal(1)
})

it('should return 0 if there are no approvals', () => {
const reviews = [
{
user: { login: 'user1' },
state: 'CHANGES_REQUESTED',
submitted_at: '2023-01-01T00:00:00Z'
},
{
user: { login: 'user2' },
state: 'COMMENTED',
submitted_at: '2023-01-02T00:00:00Z'
}
]

const result = getApproversCount(reviews as Array<Review>)

expect(result).to.equal(0)
})

it('should handle an empty array of reviews', () => {
const reviews: Array<Review> = []

const result = getApproversCount(reviews)

expect(result).to.equal(0)
})

it('should handle reviews with missing user information', () => {
const reviews = [
{ user: null, state: 'APPROVED', submitted_at: '2023-01-01T00:00:00Z' },
{
user: { login: 'user2' },
state: 'APPROVED',
submitted_at: '2023-01-02T00:00:00Z'
}
]

const result = getApproversCount(reviews as Array<Review>)

expect(result).to.equal(1)
})
})
})
46 changes: 39 additions & 7 deletions .github/actions/require-multiple-reviewers-v1/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fs from 'fs'
import ignore from 'ignore'
import { Annotation } from './types'
import { Annotation, Review } from './types'

/**
* Returns an array of annotations for the important files that require multiple reviewers.
Expand Down Expand Up @@ -30,11 +30,43 @@ export function getImportantFilesChanged(
IMPORTANT_FILES_PATH: string,
changedFiles: string[]
): string[] {
// Since this is the ignore package, it will filter out files that match the patterns
const notImportantFiles = ignore()
.add(fs.readFileSync(IMPORTANT_FILES_PATH, 'utf-8').toString())
.filter(changedFiles)
// Use the ignore package to create a filter for the important files.
const i = ignore().add(
fs.readFileSync(IMPORTANT_FILES_PATH, 'utf-8').toString()
)

// Get all the files that were filtered out earlier - those are the important files
return changedFiles.filter(file => !notImportantFiles.includes(file))
// Return the files that have changed and are important, since they will be "ignored" if this was a gitignore file.
return changedFiles.filter(file => i.ignores(file))
}

/**
* Returns the number of approvals after filtering to the latest review of each reviewer.
* @param reviews
* @returns number
*/
export function getApproversCount(reviews: Array<Review>): number {
// Get the latest review from each reviewer
const latestReviews = reviews.reduce((acc, review) => {
if (!review.user) {
return acc
}
if (!acc.has(review.user.login)) {
acc.set(review.user.login, review)
} else {
const existingReview = acc.get(review.user.login)
if (
review.submitted_at &&
existingReview!.submitted_at &&
new Date(review.submitted_at) > new Date(existingReview!.submitted_at)
) {
acc.set(review.user.login, review)
}
}
return acc
}, new Map<string, Review>())
// Filter only approvals
const approvals = Array.from(latestReviews.values()).filter(
review => review.state === 'APPROVED'
)
return approvals.length
}
Loading