diff --git a/README.md b/README.md index 980899c..6e388d7 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ This group's review is mandated on all API changes, and their goal is twofold: Even changes that seem trivial can often be made more consistent and future-proof with some modifications, and the folks on the API WG have the expertise to spot and suggest those changes. -In accordance with the above goals, this bot performs several bookkeeping duties. When a new PR is opened which is either `semver/minor` or `semver/major`, it will automatically add an `api-review/requested 🗳` label to the PR. Backport PRs (PRs to a release branch that is not `main`) and PRs labeled `backport`, `backport-check-skip`, or `fast-track 🚅` are exempt from API review. To add clarity to whether a review is occurring in a given Electron governance member's capacity as a member of the API WG, this bot then adds a GitHub Check on the PR which will update as members of the API WG indicate their approval statuses. +In accordance with the above goals, this bot performs several bookkeeping duties. When a new PR is opened which is either `semver/minor` or `semver/major`, it will automatically add an `api-review/requested 🗳` label to the PR. Backport PRs (PRs to a release branch that is not `main`) and PRs labeled `backport`, `backport-check-skip`, or `fast-track 🚅` are exempt from API review. To add clarity to whether a review is occurring in a given Electron governance member's capacity as a member of the API WG, this bot then adds a GitHub Check on the PR which will update as members of the API WG indicate their approval statuses. PRs that do not need API review (those without an API review label, those that are not `semver/minor` or `semver/major`, and those labeled `api-review/skip-review 🛂`) instead get a passing `API Review` check, so the check never blocks them. Members of the API Working Group must indicate their approval by leaving a comment via a PR Review containing `API LGTM`. This may not necessarily be a full approval with the GitHub API since approval by the API is primarily about the API shape and design. Both a PR review that comments and includes an LGTM indication or which is an approval with LGTM are sufficient. diff --git a/spec/api-review-state.spec.ts b/spec/api-review-state.spec.ts index db1ead3..f8c3fd1 100644 --- a/spec/api-review-state.spec.ts +++ b/spec/api-review-state.spec.ts @@ -17,6 +17,7 @@ import { MINIMUM_PATCH_OPEN_TIME, API_REVIEW_CHECK_NAME, API_SKIP_DELAY_LABEL, + API_SKIP_REVIEW_LABEL, API_WORKING_GROUP, } from '../src/constants'; @@ -25,6 +26,14 @@ import { loadFixture } from './utils'; const GH_API = 'https://api.github.com'; +const APP_ID = 690857; + +const CHECK_RUNS_QUERY = { + check_name: API_REVIEW_CHECK_NAME, + app_id: APP_ID, + per_page: 100, +}; + const API_WORKING_GROUP_MEMBERS = [ { login: 'ckerr' }, { login: 'codebytere' }, @@ -48,11 +57,12 @@ describe('api review', () => { beforeEach(() => { nock.cleanAll(); nock.disableNetConnect(); + vi.stubEnv('APP_ID', String(APP_ID)); robot = new Probot({ githubToken: 'test', secret: 'secret', privateKey: 'private key', - appId: 690857, + appId: APP_ID, }); moctokit = { @@ -83,6 +93,7 @@ describe('api review', () => { }); afterEach(() => { + vi.unstubAllEnvs(); expect(nock.isDone()).toEqual(true); nock.cleanAll(); }); @@ -170,7 +181,13 @@ describe('api review', () => { expect(readyDate).toEqual(expectedDate); }); - it('should reset the check when PR does not have an API review label on a base PR', async () => { + const NOT_REQUIRED = { + name: API_REVIEW_CHECK_NAME, + status: 'completed', + conclusion: CheckRunStatus.SUCCESS, + }; + + it('should mark the existing check as not required when PR does not have an API review label', async () => { let { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); moctokit.rest.checks.listForRef = vi.fn().mockReturnValue({ @@ -179,6 +196,7 @@ describe('api review', () => { { name: API_REVIEW_CHECK_NAME, id: '12345', + status: 'in_progress', }, ], }, @@ -191,13 +209,29 @@ describe('api review', () => { expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); expect(moctokit.rest.checks.listForRef).toHaveBeenCalled(); - expect(moctokit.rest.checks.update).toHaveBeenCalled(); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + check_run_id: '12345', + output: { + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', + }, + }), + ); }); - it('should do nothing when the PR does not have an API review label on a fork PR', async () => { + it('should create a passing check on the base repo when a fork PR does not have an API review label', async () => { let { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); - pull_request.fork = true; + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'electron-fork', + full_name: 'contributor/electron-fork', + owner: { login: 'contributor' }, + fork: true, + }; const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); expect(users).toEqual(undefined); @@ -205,7 +239,191 @@ describe('api review', () => { expect(moctokit.rest.issues.addLabels).not.toHaveBeenCalled(); expect(moctokit.rest.issues.removeLabel).not.toHaveBeenCalled(); - expect(moctokit.rest.checks.listForRef).toHaveBeenCalled(); + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ owner: 'electron', repo: 'electron', ref: pull_request.head.sha }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + owner: 'electron', + repo: 'electron', + head_sha: pull_request.head.sha, + }), + ); + }); + + it('should create a passing check when a semver-patch PR has no existing check', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + pull_request.labels = [{ name: REVIEW_LABELS.REQUESTED }, { name: SEMVER_LABELS.PATCH }]; + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + head_sha: pull_request.head.sha, + output: { + title: 'Not required', + summary: 'This PR does not require API review: it is not semver-minor or semver-major.', + }, + }), + ); + }); + + it('should create a passing check when a PR has the skip-review label and no existing check', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + pull_request.labels = [ + { name: REVIEW_LABELS.REQUESTED }, + { name: SEMVER_LABELS.MINOR }, + { name: API_SKIP_REVIEW_LABEL }, + ]; + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + + expect(moctokit.rest.teams.listMembersInOrg).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + ...NOT_REQUIRED, + head_sha: pull_request.head.sha, + output: { + title: 'Not required', + summary: `This PR does not require API review: it has the "${API_SKIP_REVIEW_LABEL}" label.`, + }, + }), + ); + }); + + it('should not write a passing check when the check lookup throws', async () => { + const { pull_request } = loadFixture('api-review-state/pull_request.no_review_label.json'); + + moctokit.rest.checks.listForRef = vi.fn().mockRejectedValue(new Error('Server Error')); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual(undefined); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + }); + + it('should supersede a not required check with a new in-progress run when API review is requested', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + moctokit.rest.checks.listForRef = vi.fn().mockReturnValue({ + data: { + check_runs: [ + { + name: API_REVIEW_CHECK_NAME, + id: '12345', + status: 'completed', + conclusion: CheckRunStatus.SUCCESS, + }, + ], + }, + }); + + await addOrUpdateAPIReviewCheck(moctokit, pull_request); + + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }), + ); + }); + + it('should only look up API Review check runs created by Cation', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + // Simulate the API filtering by app: a same-named run from another app + // (e.g. a fork PR's Actions job) is only returned when not filtering. + moctokit.rest.checks.listForRef = vi.fn().mockImplementation(({ app_id }) => ({ + data: { + check_runs: + app_id === APP_ID + ? [] + : [{ name: API_REVIEW_CHECK_NAME, id: '67890', app: { id: 15368 } }], + }, + })); + + await addOrUpdateAPIReviewCheck(moctokit, pull_request); + + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ check_name: API_REVIEW_CHECK_NAME, app_id: APP_ID }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ name: API_REVIEW_CHECK_NAME, head_sha: pull_request.head.sha }), + ); + }); + + it('should still return approval state on a fork PR when the Checks API throws', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'electron-fork', + full_name: 'contributor/electron-fork', + owner: { login: 'contributor' }, + fork: true, + }; + + moctokit.rest.checks.create = vi.fn().mockRejectedValue(new Error('Resource not accessible')); + moctokit.rest.pulls.listReviews.mockReturnValue({ + data: [ + { user: { id: 1, login: 'codebytere' }, body: 'API LGTM', state: 'COMMENTED' }, + { user: { id: 2, login: 'jkleinsc' }, body: 'API LGTM', state: 'COMMENTED' }, + ], + }); + moctokit.rest.teams.listMembersInOrg.mockReturnValue({ + data: [{ login: 'codebytere' }, { login: 'jkleinsc' }], + }); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(moctokit.rest.checks.create).toHaveBeenCalled(); + expect(users).toEqual({ + approved: ['codebytere', 'jkleinsc'], + declined: [], + requestedChanges: [], + }); + + await checkPRReadyForMerge(moctokit, pull_request, users); + expect(moctokit.rest.issues.addLabels).toHaveBeenCalledWith( + expect.objectContaining({ labels: [REVIEW_LABELS.APPROVED] }), + ); + }); + + it('should still return approval state without writing the check when the check lookup throws', async () => { + const { pull_request } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + + moctokit.rest.checks.listForRef = vi.fn().mockRejectedValue(new Error('Server Error')); + moctokit.rest.pulls.listReviews.mockReturnValue({ + data: [{ user: { id: 1, login: 'codebytere' }, body: 'API DECLINED', state: 'COMMENTED' }], + }); + moctokit.rest.teams.listMembersInOrg.mockReturnValue({ + data: [{ login: 'codebytere' }], + }); + + const users = await addOrUpdateAPIReviewCheck(moctokit, pull_request); + expect(users).toEqual({ approved: [], declined: ['codebytere'], requestedChanges: [] }); + expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); }); @@ -531,9 +749,8 @@ describe('api review', () => { const payload = loadFixture('api-review-state/pull_request.no_review_label.json'); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -547,10 +764,10 @@ describe('api review', () => { name: API_REVIEW_CHECK_NAME, status: 'completed', output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', }, - conclusion: CheckRunStatus.NEUTRAL, + conclusion: CheckRunStatus.SUCCESS, }; nock(GH_API) @@ -571,9 +788,8 @@ describe('api review', () => { const payload = loadFixture('api-review-state/pull_request.api-skip-review_label.json'); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -587,10 +803,10 @@ describe('api review', () => { name: API_REVIEW_CHECK_NAME, status: 'completed', output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', + title: 'Not required', + summary: 'This PR does not require API review: it has no API review label.', }, - conclusion: CheckRunStatus.NEUTRAL, + conclusion: CheckRunStatus.SUCCESS, }; nock(GH_API) @@ -618,7 +834,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -676,7 +893,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -776,7 +994,8 @@ describe('api review', () => { .reply(200, API_WORKING_GROUP_MEMBERS); nock(GH_API) - .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs?per_page=100`) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { @@ -875,6 +1094,11 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); nock(GH_API) .get(`/repos/electron/electron/pulls/${pull_request.number}/reviews`) @@ -887,8 +1111,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + await robot.receive({ id: '123-456', name: 'pull_request_review', @@ -904,6 +1140,11 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm.json'); const r2 = loadFixture('api-review-state/pull_request_review/fork/review_lgtm_2.json'); nock(GH_API) @@ -917,8 +1158,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/labels?per_page=100&page=1`) .reply(200, [ @@ -974,6 +1227,11 @@ describe('api review', () => { .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) .reply(200, API_WORKING_GROUP_MEMBERS); + nock(GH_API) + .get(`/repos/electron/electron/commits/${pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [] }); + const r1 = loadFixture('api-review-state/pull_request_review/fork/review_declined.json'); nock(GH_API) .get(`/repos/electron/electron/pulls/${pull_request.number}/reviews`) @@ -985,8 +1243,20 @@ describe('api review', () => { nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/timeline`) + .times(2) .reply(200, []); + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + nock(GH_API) .get(`/repos/electron/electron/issues/${pull_request.number}/labels?per_page=100&page=1`) .reply(200, [ @@ -1051,11 +1321,33 @@ describe('api review', () => { .reply(200); nock(GH_API) - .get( - `/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, - ) + .get(`/repos/electron/electron/commits/${payload.pull_request.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [] }); + nock(GH_API) + .get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`) + .reply(200, API_WORKING_GROUP_MEMBERS); + + nock(GH_API).get(`/repos/electron/electron/pulls/${payload.number}/reviews`).reply(200, []); + + nock(GH_API).get(`/repos/electron/electron/issues/${payload.number}/comments`).reply(200, []); + + nock(GH_API).get(`/repos/electron/electron/issues/${payload.number}/timeline`).reply(200, []); + + // The check reflects the requested label even though this event's payload + // predates it, rather than being marked as not required. + nock(GH_API) + .post('/repos/electron/electron/check-runs', (body) => { + expect(body).toMatchObject({ + name: API_REVIEW_CHECK_NAME, + head_sha: payload.pull_request.head.sha, + status: 'in_progress', + }); + return true; + }) + .reply(200); + await robot.receive({ id: '123-456', name: 'pull_request', @@ -1164,4 +1456,65 @@ describe('api review', () => { payload, }); }); + + it('continues the cron pass past a PR that fails to update', async () => { + const { pull_request: failingPR } = loadFixture( + 'api-review-state/pull_request.requested_review_label.json', + ); + const passingPR = { + ...failingPR, + number: failingPR.number + 1, + head: { ...failingPR.head, sha: 'b7d7b8b1a1a0e6c0bdb56b2f6b0c4d2b1e0a9f31' }, + labels: [{ name: REVIEW_LABELS.REQUESTED }], + }; + + nock(GH_API) + .get('/app/installations') + .reply(200, [{ id: 1 }]); + + nock(GH_API) + .get('/installation/repositories') + .reply(200, { repositories: [{ name: 'electron', owner: { login: 'electron' } }] }); + + nock(GH_API) + .get('/repos/electron/electron/pulls') + .query({ per_page: 100, state: 'open' }) + .reply(200, [failingPR, passingPR]); + + // The first PR throws while fetching API WG members. + nock(GH_API) + .get(`/repos/electron/electron/commits/${failingPR.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [] }); + + nock(GH_API).get(`/orgs/electron/teams/${API_WORKING_GROUP}/members`).reply(404); + + // The second PR is no longer semver-minor, so its check is marked not required. + nock(GH_API) + .get(`/repos/electron/electron/commits/${passingPR.head.sha}/check-runs`) + .query(CHECK_RUNS_QUERY) + .reply(200, { check_runs: [{ name: API_REVIEW_CHECK_NAME, id: '12345' }] }); + + nock(GH_API) + .patch('/repos/electron/electron/check-runs/12345', (body) => { + expect(body).toMatchObject({ conclusion: CheckRunStatus.SUCCESS }); + return true; + }) + .reply(200); + + const setTimeoutSpy = vi.spyOn(global, 'setTimeout'); + try { + robot.load((app: Probot) => setupAPIReviewStateManagement(app)); + + // The pass is complete once the next interval is scheduled. + const nextRun = await vi.waitFor(() => { + const index = setTimeoutSpy.mock.calls.findIndex(([, ms]) => ms === 1000 * 60 * 5); + expect(index).not.toEqual(-1); + return setTimeoutSpy.mock.results[index].value; + }); + clearTimeout(nextRun); + } finally { + setTimeoutSpy.mockRestore(); + } + }); }); diff --git a/spec/deprecation-review-state.spec.ts b/spec/deprecation-review-state.spec.ts index 6aa9a1f..b8fad6a 100644 --- a/spec/deprecation-review-state.spec.ts +++ b/spec/deprecation-review-state.spec.ts @@ -23,6 +23,14 @@ import { loadFixture } from './utils'; const GH_API = 'https://api.github.com'; +const APP_ID = 690857; + +const CHECK_RUNS_QUERY = { + check_name: DEPRECATION_REVIEW_CHECK_NAME, + app_id: APP_ID, + per_page: 100, +}; + const handler = async (app: Probot) => { setupDeprecationReviewStateManagement(app); }; @@ -39,11 +47,12 @@ describe('deprecation review', () => { beforeEach(() => { nock.cleanAll(); nock.disableNetConnect(); + vi.stubEnv('APP_ID', String(APP_ID)); robot = new Probot({ githubToken: 'test', secret: 'secret', privateKey: 'private key', - appId: 690857, + appId: APP_ID, }); moctokit = { @@ -73,6 +82,7 @@ describe('deprecation review', () => { }); afterEach(() => { + vi.unstubAllEnvs(); console.log(nock.pendingMocks()); expect(nock.isDone()).toEqual(true); nock.cleanAll(); @@ -164,15 +174,65 @@ describe('deprecation review', () => { expect(moctokit.rest.checks.create).toHaveBeenCalledWith(expect.objectContaining(expected)); }); - it('should not use Checks API when the PR is from a fork', async () => { + it('should only look up Deprecation Review check runs created by Cation', async () => { + const { pull_request } = loadFixture( + 'deprecation-review-state/pull_request.requested_review_label.json', + ); + + // Simulate the API filtering by app: a same-named run from another app + // (e.g. a fork PR's Actions job) is only returned when not filtering. + moctokit.rest.checks.listForRef = vi.fn().mockImplementation(({ app_id }) => ({ + data: { + check_runs: + app_id === APP_ID + ? [] + : [{ name: DEPRECATION_REVIEW_CHECK_NAME, id: '67890', app: { id: 15368 } }], + }, + })); + + await addOrUpdateDeprecationReviewCheck(moctokit, pull_request); + + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ check_name: DEPRECATION_REVIEW_CHECK_NAME, app_id: APP_ID }), + ); + expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: DEPRECATION_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + }), + ); + }); + + it('should create the check on the base repo when the PR is from a fork', async () => { const { pull_request } = loadFixture( 'deprecation-review-state/pull_request.requested_review_label.json', ); - pull_request.head.repo.fork = true; + pull_request.head.repo = { + ...pull_request.head.repo, + name: 'deprecation-review-fork', + full_name: 'contributor/deprecation-review-fork', + owner: { login: 'contributor' }, + fork: true, + }; await addOrUpdateDeprecationReviewCheck(moctokit, pull_request); - expect(moctokit.rest.checks.create).not.toHaveBeenCalled(); + expect(moctokit.rest.checks.listForRef).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'dsanders11', + repo: 'deprecation-review', + ref: pull_request.head.sha, + }), + ); + expect(moctokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'dsanders11', + repo: 'deprecation-review', + name: DEPRECATION_REVIEW_CHECK_NAME, + head_sha: pull_request.head.sha, + }), + ); expect(moctokit.rest.checks.update).not.toHaveBeenCalled(); }); @@ -258,8 +318,9 @@ describe('deprecation review', () => { nock(GH_API) .get( - `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, + `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs`, ) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [], }); @@ -308,8 +369,9 @@ describe('deprecation review', () => { nock(GH_API) .get( - `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs?per_page=100`, + `/repos/dsanders11/deprecation-review/commits/${payload.pull_request.head.sha}/check-runs`, ) + .query(CHECK_RUNS_QUERY) .reply(200, { check_runs: [ { diff --git a/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json b/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json index c88d142..806a5df 100644 --- a/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json +++ b/spec/fixtures/api-review-state/pull_request.api-skip-review_label.json @@ -36,6 +36,7 @@ }], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.approved_review_label.json b/spec/fixtures/api-review-state/pull_request.approved_review_label.json index cda698a..a890169 100644 --- a/spec/fixtures/api-review-state/pull_request.approved_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.approved_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.declined_review_label.json b/spec/fixtures/api-review-state/pull_request.declined_review_label.json index e4b1189..a16baa5 100644 --- a/spec/fixtures/api-review-state/pull_request.declined_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.declined_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json b/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json index 753d748..60ac8c4 100644 --- a/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json +++ b/spec/fixtures/api-review-state/pull_request.labeled.exclusion_labels.json @@ -35,6 +35,7 @@ ], "base": { "repo": { + "name": "electron", "owner": { "login": "electron" } diff --git a/spec/fixtures/api-review-state/pull_request.requested_review_label.json b/spec/fixtures/api-review-state/pull_request.requested_review_label.json index b5c2698..b03851f 100644 --- a/spec/fixtures/api-review-state/pull_request.requested_review_label.json +++ b/spec/fixtures/api-review-state/pull_request.requested_review_label.json @@ -45,6 +45,7 @@ ], "base": { "repo": { + "name": "electron", "full_name": "electron/electron", "owner": { "login": "electron" diff --git a/src/api-review-state.ts b/src/api-review-state.ts index c105d5c..21e8f83 100644 --- a/src/api-review-state.ts +++ b/src/api-review-state.ts @@ -104,59 +104,78 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: type CommentOrReview = ListReviewsItem & ListCommentsItem; - if (!pr.head.repo) { - log( - 'addOrUpdateAPIReviewCheck', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot update check`, - ); - return; - } + type CheckRun = GetResponseDataTypeFromEndpointMethod< + typeof octokit.rest.checks.listForRef + >['check_runs'][0]; - const fork = pr.head.repo.fork; + // Checks are always written to the base repo against the head SHA, so that + // they also work for PRs opened from forks (where Cation is not installed). const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; + const repo = pr.base.repo.name; - if (fork) { + // Checks API failures are logged rather than thrown, so that they can never + // block API review label automation. + const logCheckError = (err: unknown) => log( 'addOrUpdateAPIReviewCheck', - LogLevel.INFO, - `${pr.number} is a fork - checks will not be created or updated`, + LogLevel.ERROR, + `Failed to update ${API_REVIEW_CHECK_NAME} check for #${pr.number}: ${err}`, ); + + // Fetch the latest API Review check for the PR. Only runs created by Cation + // itself are considered, so that a same-named run from another app (e.g. an + // Actions job added by a fork PR) can't shadow it. + let checkRun: CheckRun | undefined; + let checkLookupFailed = false; + try { + checkRun = ( + await octokit.rest.checks.listForRef({ + ref: pr.head.sha, + check_name: API_REVIEW_CHECK_NAME, + app_id: Number(getEnvVar('APP_ID')), + per_page: 100, + owner, + repo, + }) + ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); + } catch (err) { + checkLookupFailed = true; + logCheckError(err); } - // Fetch the latest API Review check for the PR. - const checkRun = fork - ? null - : ( - await octokit.rest.checks.listForRef({ - ref: pr.head.sha, - per_page: 100, - owner, - repo, - }) - ).data.check_runs.find((run) => run.name === API_REVIEW_CHECK_NAME); + // PRs that don't need API review get a passing check on every head SHA, so + // that the check can be required without blocking them. + const markNotRequired = async (reason: string) => { + // If we couldn't look up the existing check, don't risk creating a duplicate. + if (checkLookupFailed) return; - const resetToNeutral = async () => { - if (!checkRun) return; - return await octokit.rest.checks.update({ + const opts = { owner, repo, name: API_REVIEW_CHECK_NAME, - status: 'completed', + status: 'completed' as const, + conclusion: CheckRunStatus.SUCCESS, output: { - title: 'Outdated', - summary: 'PR no longer requires API Review', + title: 'Not required', + summary: `This PR does not require API review: ${reason}.`, }, - check_run_id: checkRun.id, - conclusion: CheckRunStatus.NEUTRAL, - }); + }; + + try { + if (checkRun) { + await octokit.rest.checks.update({ ...opts, check_run_id: checkRun.id }); + } else { + await octokit.rest.checks.create({ ...opts, head_sha: pr.head.sha }); + } + } catch (err) { + logCheckError(err); + } }; // We do not care about PRs without an API review label of any kind. const currentReviewLabel = pr.labels.find((l) => Object.values(REVIEW_LABELS).includes(l.name)); if (!currentReviewLabel) { - await resetToNeutral(); + await markNotRequired('it has no API review label'); return; } @@ -167,7 +186,7 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: LogLevel.INFO, 'Determined this PR is semver-patch and does not need review', ); - await resetToNeutral(); + await markNotRequired('it is not semver-minor or semver-major'); return; } @@ -178,7 +197,7 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: LogLevel.INFO, 'This PR has the skip-review label and does not need review', ); - await resetToNeutral(); + await markNotRequired(`it has the "${API_SKIP_REVIEW_LABEL}" label`); return; } @@ -284,9 +303,6 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: const users = { approved, declined, requestedChanges }; - // If the PR is a fork PR, return early as the Checks API doesn't work. - if (fork) return users; - // Update the GitHub Check with appropriate API review information. const updateCheck = async ( opts: Omit< @@ -294,25 +310,32 @@ export async function addOrUpdateAPIReviewCheck(octokit: Context['octokit'], pr: 'baseUrl' | 'headers' | 'mediaType' | 'owner' | 'repo' | 'name' | 'head_sha' >, ) => { - if ( - checkRun && - (checkRun.status === opts.status || !opts.status || opts.status === 'completed') - ) { - await octokit.rest.checks.update({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, - name: API_REVIEW_CHECK_NAME, - check_run_id: checkRun.id, - ...opts, - }); - } else { - await octokit.rest.checks.create({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, - name: API_REVIEW_CHECK_NAME, - head_sha: pr.head.sha, - ...opts, - }); + // If we couldn't look up the existing check, don't risk creating a duplicate. + if (checkLookupFailed) return users; + + try { + if ( + checkRun && + (checkRun.status === opts.status || !opts.status || opts.status === 'completed') + ) { + await octokit.rest.checks.update({ + owner, + repo, + name: API_REVIEW_CHECK_NAME, + check_run_id: checkRun.id, + ...opts, + }); + } else { + await octokit.rest.checks.create({ + owner, + repo, + name: API_REVIEW_CHECK_NAME, + head_sha: pr.head.sha, + ...opts, + }); + } + } catch (err) { + logCheckError(err); } return users; @@ -522,7 +545,7 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest sender: { login: initiator }, } = context.payload; - const pr = context.payload.pull_request as PullRequest; + let pr = context.payload.pull_request as PullRequest; if (!label) { throw new Error('Something went wrong - label does not exist.'); @@ -557,6 +580,15 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest labels: [REVIEW_LABELS.REQUESTED], }); + // This event's payload predates the label we just added, so include it + // when updating the check to avoid briefly marking it as not required. + if (!hasAPIReviewRequestedLabel(pr)) { + pr = { + ...pr, + labels: [...pr.labels, { name: REVIEW_LABELS.REQUESTED } as PullRequest['labels'][0]], + }; + } + // If the human-added label is an approve/decline API review label // remove it. } else if (isReviewLabel(label.name)) { @@ -676,8 +708,15 @@ export function setupAPIReviewStateManagement(probot: Probot, disableCronForTest ); for (const pr of reviewPRs) { - const approvalState = await addOrUpdateAPIReviewCheck(octokit, pr); - await checkPRReadyForMerge(octokit, pr, approvalState); + // Isolate each PR so that one failure doesn't skip the remaining PRs. + try { + const approvalState = await addOrUpdateAPIReviewCheck(octokit, pr); + await checkPRReadyForMerge(octokit, pr, approvalState); + } catch (err) { + probot.log.error( + `Failed to update API review state for ${repo.owner.login}/${repo.name}#${pr.number}: ${err}`, + ); + } } } } diff --git a/src/deprecation-review-state.ts b/src/deprecation-review-state.ts index 6dfa372..74a57b7 100644 --- a/src/deprecation-review-state.ts +++ b/src/deprecation-review-state.ts @@ -32,32 +32,19 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, `Validating ${pr.number} by ${pr.user.login}`, ); - if (!pr.head.repo) { - log( - 'addOrUpdateDeprecationReviewCheck', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot update check`, - ); - return; - } - + // Checks are always written to the base repo against the head SHA, so that + // they also work for PRs opened from forks (where Cation is not installed). const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; - - if (pr.head.repo.fork) { - log( - 'addOrUpdateDeprecationReviewCheck', - LogLevel.INFO, - `${pr.number} is a fork - checks will not be created or updated`, - ); - // If the PR is a fork PR, return early as the Checks API doesn't work. - return; - } + const repo = pr.base.repo.name; - // Fetch the latest Deprecation Review check for the PR. + // Fetch the latest Deprecation Review check for the PR. Only runs created by + // Cation itself are considered, so that a same-named run from another app + // (e.g. an Actions job added by a fork PR) can't shadow it. const checkRun = ( await octokit.rest.checks.listForRef({ ref: pr.head.sha, + check_name: DEPRECATION_REVIEW_CHECK_NAME, + app_id: Number(getEnvVar('APP_ID')), per_page: 100, owner, repo, @@ -99,16 +86,16 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, (checkRun.status === opts.status || !opts.status || opts.status === 'completed') ) { await octokit.rest.checks.update({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: DEPRECATION_REVIEW_CHECK_NAME, check_run_id: checkRun.id, ...opts, }); } else { await octokit.rest.checks.create({ - owner: pr.head.repo!.owner.login, - repo: pr.head.repo!.name, + owner, + repo, name: DEPRECATION_REVIEW_CHECK_NAME, head_sha: pr.head.sha, ...opts, @@ -147,17 +134,8 @@ export async function addOrUpdateDeprecationReviewCheck(octokit: ProbotOctokit, } export async function maybeAddChecklistComment(octokit: ProbotOctokit, pr: PullRequest) { - if (!pr.head.repo) { - log( - 'maybeAddChecklistComment', - LogLevel.WARN, - `PR #${pr.number} does not have a head repo - cannot add checklist comment`, - ); - return; - } - const owner = pr.base.repo.owner.login; - const repo = pr.head.repo.name; + const repo = pr.base.repo.name; // We do not care about PRs without the deprecation review requested label. const currentReviewLabel = pr.labels.find(({ name }) => isReviewLabel(name));