Skip to content

Commit

Permalink
feat(manage-merge-queue): enforce required approvals (#314)
Browse files Browse the repository at this point in the history
* feat(manage-merge-queue): check for required approvals when pr added to queue

* update docs

* add support for teams input also
  • Loading branch information
danadajian authored Jan 18, 2023
1 parent 1ab935d commit 23d029a
Show file tree
Hide file tree
Showing 15 changed files with 378 additions and 20 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ Additionally, the following parameters can be used for additional control over t
* Merging a PR will update the positions of all PRs in the queue.
* Adding the `JUMP THE QUEUE` label to a PR will make that PR first in the queue immediately.
* When a PR is merged, it automatically updates the first-queued PR with the default branch.
* If a PR does not have all required approvals (as determined by `CODEOWNERS` if present or `teams` if provided), it will be denied entrance to the merge queue.
* You can also pass `login` and `slack_webhook_url` to notify the PR author when they are in the 1st position of the merge queue.

### [move-project-card](.github/workflows/move-project-card.yml)
Expand Down
4 changes: 2 additions & 2 deletions dist/153.index.js

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

2 changes: 1 addition & 1 deletion dist/153.index.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/499.index.js

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

2 changes: 1 addition & 1 deletion dist/499.index.js.map

Large diffs are not rendered by default.

283 changes: 279 additions & 4 deletions dist/676.index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/676.index.js.map

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/905.index.js

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

2 changes: 1 addition & 1 deletion dist/905.index.js.map

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20097,13 +20097,15 @@ var map = {
7473,
832,
710,
445,
700,
676
],
"./manage-merge-queue.ts": [
7473,
832,
710,
445,
700,
676
],
Expand Down Expand Up @@ -20133,13 +20135,15 @@ var map = {
1004,
832,
710,
445,
700,
676
],
"./prepare-queued-pr-for-merge.ts": [
1004,
832,
710,
445,
700,
676
],
Expand Down
2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

20 changes: 17 additions & 3 deletions src/helpers/manage-merge-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,38 @@ import { removeLabelIfExists } from './remove-label';
import { setCommitStatus } from './set-commit-status';
import { updateMergeQueue } from '../utils/update-merge-queue';
import { paginateAllOpenPullRequests } from '../utils/paginate-open-pull-requests';
import { approvalsSatisfied } from './approvals-satisfied';
import { createPrComment } from './create-pr-comment';

export class ManageMergeQueue extends HelperInputs {
login?: string;
slack_webhook_url?: string;
teams?: string;
}

export const manageMergeQueue = async ({ login, slack_webhook_url }: ManageMergeQueue = {}) => {
export const manageMergeQueue = async ({ login, slack_webhook_url, teams }: ManageMergeQueue = {}) => {
const { data: pullRequest } = await octokit.pulls.get({ pull_number: context.issue.number, ...context.repo });
if (pullRequest.merged || !pullRequest.labels.find(label => label.name === READY_FOR_MERGE_PR_LABEL)) {
core.info('This PR is not in the merge queue.');
return removePrFromQueue(pullRequest);
}
const queuedPrs = await getQueuedPullRequests();
const queuePosition = queuedPrs.length;
if (pullRequest.labels.find(label => label.name === JUMP_THE_QUEUE_PR_LABEL)) {
const jumpQueueRequested = pullRequest.labels.find(label => label.name === JUMP_THE_QUEUE_PR_LABEL);
if (jumpQueueRequested) {
return updateMergeQueue(queuedPrs);
}
if (!pullRequest.labels.find(label => label.name?.startsWith(QUEUED_FOR_MERGE_PREFIX))) {
const prAlreadyInQueue = pullRequest.labels.find(label => label.name?.startsWith(QUEUED_FOR_MERGE_PREFIX));
if (!prAlreadyInQueue) {
const allRequiredApprovalsAreMet = await approvalsSatisfied({ teams });
if (!allRequiredApprovalsAreMet) {
core.info('This PR is missing required approvals.');
await removeLabelIfExists(READY_FOR_MERGE_PR_LABEL, pullRequest.number);
return createPrComment({
sha: pullRequest.head.sha,
body: 'This PR is missing required approvals. Please obtain all required approvals prior to entering the merge queue!'
});
}
await addPrToQueue(pullRequest, queuePosition);
}

Expand Down
4 changes: 2 additions & 2 deletions src/utils/get-core-member-logins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export const getCoreTeamsAndLogins = async (pull_number: number, teams?: string[
const codeOwners = teams ?? (await getCodeOwners(pull_number));

if (!codeOwners?.length) {
core.setFailed('No code owners found. Please provide a "teams" input or set up a CODEOWNERS file in your repo.');
throw new Error();
core.warning('No code owners found. Please provide a "teams" input or set up a CODEOWNERS file in your repo.');
return [];
}

const teamsAndLogins = await map(codeOwners, async team =>
Expand Down
22 changes: 22 additions & 0 deletions test/helpers/approvals-satisfied.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,26 @@ describe('approvalsSatisfied', () => {
const result = await approvalsSatisfied({ teams: 'team1\nteam2\nteam3' });
expect(result).toBe(true);
});

it('should return true when no teams input provided and no CODEOWNERS found', async () => {
(getCoreTeamsAndLogins as jest.Mock).mockResolvedValue([]);
(octokit.pulls.listReviews as unknown as Mocktokit).mockImplementation(async () => ({
data: [
{
state: 'APPROVED',
user: { login: 'user1' }
},
{
state: 'APPROVED',
user: { login: 'user2' }
},
{
state: 'CHANGES_REQUESTED',
user: { login: 'user3' }
}
]
}));
const result = await approvalsSatisfied();
expect(result).toBe(true);
});
});
42 changes: 42 additions & 0 deletions test/helpers/manage-merge-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ import { octokit, octokitGraphql } from '../../src/octokit';
import { removeLabelIfExists } from '../../src/helpers/remove-label';
import { setCommitStatus } from '../../src/helpers/set-commit-status';
import { updateMergeQueue } from '../../src/utils/update-merge-queue';
import { approvalsSatisfied } from '../../src/helpers/approvals-satisfied';
import { createPrComment } from '../../src/helpers/create-pr-comment';

jest.mock('../../src/helpers/approvals-satisfied');
jest.mock('../../src/helpers/create-pr-comment');
jest.mock('../../src/helpers/remove-label');
jest.mock('../../src/helpers/set-commit-status');
jest.mock('../../src/utils/notify-user');
Expand All @@ -40,6 +44,10 @@ jest.mock('@actions/github', () => ({
}));

describe('manageMergeQueue', () => {
beforeEach(() => {
(approvalsSatisfied as jest.Mock).mockResolvedValue(true);
});

describe('pr merged case', () => {
const queuedPrs = [{ labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }, { labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }];
beforeEach(async () => {
Expand Down Expand Up @@ -103,6 +111,40 @@ describe('manageMergeQueue', () => {
});
});

describe('pr missing required approvals case', () => {
const queuedPrs = [{ labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }, { labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }];
beforeEach(async () => {
(octokit.pulls.list as unknown as Mocktokit).mockImplementation(async ({ page }) => ({
data: page === 1 ? queuedPrs : []
}));
(octokit.pulls.get as unknown as Mocktokit).mockImplementation(async () => ({
data: {
merged: false,
head: { sha: 'sha' },
labels: [{ name: READY_FOR_MERGE_PR_LABEL }],
number: 123
}
}));
(approvalsSatisfied as jest.Mock).mockResolvedValue(false);
await manageMergeQueue();
});

it('should call removeLabel with correct params', () => {
expect(removeLabelIfExists).toHaveBeenCalledWith(READY_FOR_MERGE_PR_LABEL, 123);
});

it('should call createPrComment with correct params', () => {
expect(createPrComment).toHaveBeenCalledWith({
sha: 'sha',
body: 'This PR is missing required approvals. Please obtain all required approvals prior to entering the merge queue!'
});
});

it('should not enable auto-merge', () => {
expect(octokitGraphql).not.toHaveBeenCalled();
});
});

describe('pr ready for merge case', () => {
const queuedPrs = [{ labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }, { labels: [{ name: READY_FOR_MERGE_PR_LABEL }] }];
beforeEach(async () => {
Expand Down

0 comments on commit 23d029a

Please sign in to comment.