Skip to content

Commit

Permalink
Revert "feat(manage-merge-queue): enforce required approvals (#314)" (#…
Browse files Browse the repository at this point in the history
…315)

This reverts commit 23d029a.
  • Loading branch information
danadajian authored Jan 18, 2023
1 parent 23d029a commit 66f0217
Show file tree
Hide file tree
Showing 15 changed files with 20 additions and 378 deletions.
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ 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: 4 additions & 279 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: 0 additions & 4 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20097,15 +20097,13 @@ var map = {
7473,
832,
710,
445,
700,
676
],
"./manage-merge-queue.ts": [
7473,
832,
710,
445,
700,
676
],
Expand Down Expand Up @@ -20135,15 +20133,13 @@ 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: 3 additions & 17 deletions src/helpers/manage-merge-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,38 +29,24 @@ 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, teams }: ManageMergeQueue = {}) => {
export const manageMergeQueue = async ({ login, slack_webhook_url }: 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;
const jumpQueueRequested = pullRequest.labels.find(label => label.name === JUMP_THE_QUEUE_PR_LABEL);
if (jumpQueueRequested) {
if (pullRequest.labels.find(label => label.name === JUMP_THE_QUEUE_PR_LABEL)) {
return updateMergeQueue(queuedPrs);
}
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!'
});
}
if (!pullRequest.labels.find(label => label.name?.startsWith(QUEUED_FOR_MERGE_PREFIX))) {
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.warning('No code owners found. Please provide a "teams" input or set up a CODEOWNERS file in your repo.');
return [];
core.setFailed('No code owners found. Please provide a "teams" input or set up a CODEOWNERS file in your repo.');
throw new Error();
}

const teamsAndLogins = await map(codeOwners, async team =>
Expand Down
22 changes: 0 additions & 22 deletions test/helpers/approvals-satisfied.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,26 +145,4 @@ 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: 0 additions & 42 deletions test/helpers/manage-merge-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@ 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 @@ -44,10 +40,6 @@ 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 @@ -111,40 +103,6 @@ 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 66f0217

Please sign in to comment.