Skip to content

Commit 3bffc59

Browse files
erickzhaoerickzhao
authored
feat(api-review): add skip-review label (#198)
1 parent 89db1dd commit 3bffc59

File tree

5 files changed

+182
-11
lines changed

5 files changed

+182
-11
lines changed

‎README.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ If a PR has passed its minimum open time and has the requisite number of approva
5050

5151
For PRs that need to land faster than the minimum open time (e.g. to respond to OS or Chromium updates), the minimum open time can be bypassed by adding a `api-review/skip-delay ⏰` label to the PR. This label may be added to a PR if at least two members of the API WG representing two different employers approve fast-tracking the PR.
5252

53+
For PRs that are labeled as `semver/minor` or `semver/major` but contain no public-facing API surface, WG members may manually add the `api-review/skip-review 🛂` label
54+
to bypass the GitHub check.
55+
5356
## Deprecation Review
5457

5558
The bot controls the deprecation review lifecycle on behalf of the [Releases Working Group](https://github.com/electron/governance/tree/main/wg-releases).

‎spec/api-review-state.spec.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,46 @@ describe('api review', () => {
456456
});
457457
});
458458

459+
it('correctly returns PR ready date when skip-review label is found', async () => {
460+
const payload = loadFixture('api-review-state/pull_request.api-skip-review_label.json');
461+
462+
nock('https://api.github.com')
463+
.get(
464+
`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`,
465+
)
466+
.reply(200, {
467+
check_runs: [
468+
{
469+
name: API_REVIEW_CHECK_NAME,
470+
id: '12345',
471+
},
472+
],
473+
});
474+
475+
const expected = {
476+
name: API_REVIEW_CHECK_NAME,
477+
status: 'completed',
478+
output: {
479+
title: 'Outdated',
480+
summary: 'PR no longer requires API Review',
481+
},
482+
conclusion: CheckRunStatus.NEUTRAL,
483+
};
484+
485+
nock('https://api.github.com')
486+
.patch(`/repos/electron/electron/check-runs/12345`, (body) => {
487+
expect(body).toMatchObject(expected);
488+
return true;
489+
})
490+
.reply(200);
491+
492+
await robot.receive({
493+
id: '123-456',
494+
name: 'pull_request',
495+
payload,
496+
});
497+
});
498+
459499
describe('correctly updates API Review data when a PR review is submitted', () => {
460500
describe('from the base repo', () => {
461501
it('updates the check when there is one API LGTM', async () => {

‎spec/fixtures/api-review-state/pull_request.api-skip-review_label.json‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
{
2+
"action": "opened",
3+
"number": 26876,
4+
"pull_request": {
5+
"url": "https://api.github.com/repos/electron/electron/pulls/26876",
6+
"id": 534054584,
7+
"node_id": "MDExOlB1bGxSZXF1ZXN0NTM0MDU0NTg0",
8+
"html_url": "https://github.com/electron/electron/pull/26876",
9+
"diff_url": "https://github.com/electron/electron/pull/26876.diff",
10+
"patch_url": "https://github.com/electron/electron/pull/26876.patch",
11+
"issue_url": "https://api.github.com/repos/electron/electron/issues/26876",
12+
"number": 26876,
13+
"state": "open",
14+
"locked": false,
15+
"title": "chore: fix JS linting",
16+
"user": {
17+
"login": "MarshallOfSound",
18+
"id": 6634592,
19+
"node_id": "MDQ6VXNlcjY2MzQ1OTI="
20+
},
21+
"body": "* Ensure --fix output is actually written to disk\r\n* Cache bust on lint.js file changes\r\n* Ensure CI does not use the linting cache\r\n\r\nNotes: no-notes",
22+
"created_at": "2020-12-08T01:24:55Z",
23+
"updated_at": "2020-12-08T01:24:55Z",
24+
"closed_at": null,
25+
"merged_at": null,
26+
"merge_commit_sha": null,
27+
"assignee": null,
28+
"labels": [{
29+
"id": 1034512799,
30+
"node_id": "MDU6TGFiZWwxMDM0NTEyNzk5",
31+
"url": "https://api.github.com/repos/electron/electron/labels/api-review/skip-review%20%F0%9F%9B%82",
32+
"name": "api-review/skip-review 🛂",
33+
"color": "6ac2dd",
34+
"default": false,
35+
"description": "skip the API approval process"
36+
}],
37+
"base": {
38+
"repo": {
39+
"owner": {
40+
"login": "electron"
41+
}
42+
}
43+
},
44+
"head": {
45+
"label": "electron:fix-lint-js",
46+
"ref": "fix-lint-js",
47+
"sha": "c6b1b7168ab850a47f856c4a30f7a441bede1117",
48+
"user": {
49+
"login": "electron",
50+
"id": 13409222,
51+
"node_id": "MDEyOk9yZ2FuaXphdGlvbjEzNDA5MjIy",
52+
"avatar_url": "https://avatars.githubusercontent.com/u/13409222?v=4",
53+
"gravatar_id": "",
54+
"url": "https://api.github.com/users/electron",
55+
"html_url": "https://github.com/electron",
56+
"followers_url": "https://api.github.com/users/electron/followers",
57+
"following_url": "https://api.github.com/users/electron/following{/other_user}",
58+
"gists_url": "https://api.github.com/users/electron/gists{/gist_id}",
59+
"starred_url": "https://api.github.com/users/electron/starred{/owner}{/repo}",
60+
"subscriptions_url": "https://api.github.com/users/electron/subscriptions",
61+
"organizations_url": "https://api.github.com/users/electron/orgs",
62+
"repos_url": "https://api.github.com/users/electron/repos",
63+
"events_url": "https://api.github.com/users/electron/events{/privacy}",
64+
"received_events_url": "https://api.github.com/users/electron/received_events",
65+
"type": "Organization",
66+
"site_admin": false
67+
},
68+
"repo": {
69+
"id": 9384267,
70+
"node_id": "MDEwOlJlcG9zaXRvcnk5Mzg0MjY3",
71+
"name": "electron",
72+
"full_name": "electron/electron",
73+
"private": false,
74+
"owner": {
75+
"login": "electron",
76+
"id": 13409222,
77+
"node_id": "MDEyOk9yZ2FuaXphdGlvbjEzNDA5MjIy",
78+
"avatar_url": "https://avatars.githubusercontent.com/u/13409222?v=4",
79+
"gravatar_id": "",
80+
"url": "https://api.github.com/users/electron",
81+
"html_url": "https://github.com/electron",
82+
"followers_url": "https://api.github.com/users/electron/followers",
83+
"following_url": "https://api.github.com/users/electron/following{/other_user}",
84+
"gists_url": "https://api.github.com/users/electron/gists{/gist_id}",
85+
"starred_url": "https://api.github.com/users/electron/starred{/owner}{/repo}",
86+
"subscriptions_url": "https://api.github.com/users/electron/subscriptions",
87+
"organizations_url": "https://api.github.com/users/electron/orgs",
88+
"repos_url": "https://api.github.com/users/electron/repos",
89+
"events_url": "https://api.github.com/users/electron/events{/privacy}",
90+
"received_events_url": "https://api.github.com/users/electron/received_events",
91+
"type": "Organization",
92+
"site_admin": false
93+
}
94+
},
95+
"repository": {
96+
"id": 9384267,
97+
"node_id": "MDEwOlJlcG9zaXRvcnk5Mzg0MjY3",
98+
"name": "electron",
99+
"full_name": "electron/electron",
100+
"private": false,
101+
"owner": {
102+
"login": "electron",
103+
"id": 13409222
104+
},
105+
"license": {
106+
"key": "mit",
107+
"name": "MIT License",
108+
"spdx_id": "MIT",
109+
"url": "https://api.github.com/licenses/mit",
110+
"node_id": "MDc6TGljZW5zZTEz"
111+
}
112+
}
113+
}
114+
}
115+
}

‎src/api-review-state.ts‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { log } from './utils/log-util';
33
import {
44
API_REVIEW_CHECK_NAME,
55
API_SKIP_DELAY_LABEL,
6+
API_SKIP_REVIEW_LABEL,
67
API_WORKING_GROUP,
78
EXCLUDE_LABELS,
89
MINIMUM_MINOR_OPEN_TIME,
@@ -126,6 +127,28 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr:
126127
return;
127128
}
128129

130+
// If the PR is semver-patch, it does not need API review.
131+
if (!pr.labels.some((l) => isSemverMajorMinorLabel(l.name))) {
132+
log(
133+
'addOrUpdateAPIReviewCheck',
134+
LogLevel.INFO,
135+
'Determined this PR is semver-patch and does not need review',
136+
);
137+
await resetToNeutral();
138+
return;
139+
}
140+
141+
// If the PR has the skip-review label, it doesn't need API review.
142+
if (pr.labels.some((l) => l.name === API_SKIP_REVIEW_LABEL)) {
143+
log(
144+
'addOrUpdateAPIReviewCheck',
145+
LogLevel.INFO,
146+
'This PR has the skip-review label and does not need review',
147+
);
148+
await resetToNeutral();
149+
return;
150+
}
151+
129152
// Fetch members of the API Working Group.
130153
const members = (
131154
await octokit.teams.listMembersInOrg({
@@ -214,17 +237,6 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr:
214237
`Found ${allReviews.length} relevant reviews from WG members`,
215238
);
216239

217-
// If the PR is semver-patch, it does not need API review.
218-
if (!pr.labels.some((l) => isSemverMajorMinorLabel(l.name))) {
219-
log(
220-
'addOrUpdateAPIReviewCheck',
221-
LogLevel.INFO,
222-
'Determined this PR is semver-patch and does not need review',
223-
);
224-
await resetToNeutral();
225-
return;
226-
}
227-
228240
const approved = allReviews.filter((r) => r.body?.match(lgtm)).map((r) => r.user?.login);
229241
const declined = allReviews.filter((r) => r.body?.match(decline)).map((r) => r.user?.login);
230242
const requestedChanges = allReviews

‎src/constants.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export const REVIEW_LABELS = {
4040
DECLINED: 'api-review/declined ❌',
4141
};
4242
export const API_SKIP_DELAY_LABEL = 'api-review/skip-delay ⏰';
43+
export const API_SKIP_REVIEW_LABEL = 'api-review/skip-review 🛂';
4344

4445
export const DEPRECATION_REVIEW_LABELS = {
4546
REQUESTED: 'deprecation-review/requested 📝',

0 commit comments

Comments
 (0)