diff --git a/__tests__/bundle/cronLgtmFailureArms.test.ts b/__tests__/bundle/cronLgtmFailureArms.test.ts index fd0fe616..fcd5f909 100644 --- a/__tests__/bundle/cronLgtmFailureArms.test.ts +++ b/__tests__/bundle/cronLgtmFailureArms.test.ts @@ -83,15 +83,35 @@ describe('dist/index.js schedule lgtm job failure arms', () => { ]) }) - it('fails the run when a listed pull request cannot be re-read for evaluation, and stops paging', async () => { - routePage([openPr(['lgtm'])]) + it('keeps evaluating the later pull requests and pages when one cannot be re-read, and fails the run naming it', async () => { + gh.route('GET', new RegExp(`^${repo}/pulls\\?`), (req) => { + const page = new URL(req.path, gh.url).searchParams.get('page') + return { status: 200, body: page === '1' ? [openPr(['lgtm'])] : page === '2' ? [openPr(['lgtm'], { number: 4 })] : [] } + }) gh.route('GET', `${repo}/pulls/2`, { status: 500, body: { message: 'Internal Server Error' } }) + gh.route('GET', `${repo}/pulls/4`, { status: 200, body: { ...openPr(['lgtm'], { number: 4 }), mergeable: true, mergeable_state: 'clean' } }) + gh.commitStatuses(repo, head, [{ context: 'prow/lgtm', state: 'success' }]) + gh.route('PUT', `${repo}/pulls/4/merge`, { status: 200, body: { merged: true } }) const result = await runCron() expect(result.status, result.stdout).toBe(1) - expect(result.errors.some(e => e.includes('error handling cron job: TypeError: error processing pr: HttpError: Internal Server Error'))).toBe(true) - expect(gh.requestsMatching('PUT', /./)).toEqual([]) - expectRequests(configReads(), [page1, ownersProbe, `GET ${repo}/pulls/2`]) + expect(result.errors.some(e => e.includes('could not evaluate pr #2: HttpError: Internal Server Error'))).toBe(true) + expect(result.errors.some(e => e.includes('error handling cron job: Error: 1 pull request(s) could not be merged: #2 (could not evaluate: HttpError: Internal Server Error)'))).toBe(true) + expect(result.stdout).toContain('merged pr #4') + const merges = gh.requestsMatching('PUT', /./) + expect(merges).toHaveLength(1) + expect(merges[0].path).toBe(`${repo}/pulls/4/merge`) + expectRequests(configReads(), [ + page1, + ownersProbe, + `GET ${repo}/pulls/2`, + page2, + `GET ${repo}/pulls/4`, + `GET ${repo}/commits/${head}/status?per_page=100`, + queueRead, + `PUT ${repo}/pulls/4/merge`, + `GET ${repo}/pulls?state=open&page=3`, + ]) }) }) diff --git a/__tests__/cronJobTest/lgtm.test.ts b/__tests__/cronJobTest/lgtm.test.ts index 5531afa3..4af2b8ce 100644 --- a/__tests__/cronJobTest/lgtm.test.ts +++ b/__tests__/cronJobTest/lgtm.test.ts @@ -506,20 +506,46 @@ describe('cronLgtm', () => { expect(setFailed).not.toHaveBeenCalled() }) - it('fails the run when evaluating a pr throws instead of returning a verdict', async () => { + it('keeps evaluating the other prs when evaluating one throws, and fails the run naming it', async () => { utils.setupJobsEnv('lgtm') const context = new utils.MockContext(pullReqOpenedEvent) - routePulls([lgtmPr(7)]) - const merge = new utils.ObserveRequest() + routePulls([lgtmPr(7), lgtmPr(8)]) + const broken = new utils.ObserveRequest() + const merged = observeMerge(8) server.use( http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls/7`, utils.mockResponse(500, { message: 'Server Error' })), - http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/7/merge`, utils.mockResponse(200, null, merge)), + http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/7/merge`, utils.mockResponse(200, null, broken)), ) const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + const error = vi.spyOn(core, 'error').mockImplementation(() => {}) + await expect(handleCronJobs(context)).resolves.not.toThrow() + await expect(broken.notCalled()).resolves.toBe('not called') + await expect(merged.called()).resolves.toBe('called') + expect(error).toHaveBeenCalledWith(expect.stringContaining('could not evaluate pr #7: HttpError: Server Error')) + expect(setFailed).toHaveBeenCalledTimes(1) + expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('1 pull request(s) could not be merged: #7 (could not evaluate: HttpError: Server Error)')) + expect(setFailed).not.toHaveBeenCalledWith(expect.stringContaining('#8')) + }) + + it('keeps paging after a pr whose evaluation throws, and names every pr that failed', async () => { + utils.setupJobsEnv('lgtm') + const context = new utils.MockContext(pullReqOpenedEvent) + const pages: Record = { 1: [lgtmPr(7)], 2: [lgtmPr(9)] } + server.use( + http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls`, ({ request }) => { + const page = new URL(request.url).searchParams.get('page') ?? '' + return utils.mockResponse(200, pages[page] ?? [])({ request }) + }), + http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls/7`, utils.mockResponse(500, { message: 'Server Error' })), + servePullsByNumber([lgtmPr(9)]), + http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/9/merge`, utils.mockResponse(405, { message: 'Pull Request is not mergeable' })), + ) + + const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + vi.spyOn(core, 'error').mockImplementation(() => {}) await expect(handleCronJobs(context)).resolves.not.toThrow() - await expect(merge.notCalled()).resolves.toBe('not called') expect(setFailed).toHaveBeenCalledTimes(1) - expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('error processing pr')) + expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('2 pull request(s) could not be merged: #7 (could not evaluate: HttpError: Server Error), #9 (Pull Request is not mergeable)')) }) }) diff --git a/dist/index.js b/dist/index.js index 1a333a85..a48922af 100644 --- a/dist/index.js +++ b/dist/index.js @@ -32176,7 +32176,7 @@ function setCommandEcho(enabled) { */ function setFailed(message) { process.exitCode = ExitCode.Failure; - error(message); + core_error(message); } //----------------------------------------------------------------------- // Logging Commands @@ -32199,7 +32199,7 @@ function core_debug(message) { * @param message error issue message. Errors will be converted to string via toString() * @param properties optional properties to add to the annotation. */ -function error(message, properties = {}) { +function core_error(message, properties = {}) { command_issueCommand('error', toCommandProperties(properties), message instanceof Error ? message.toString() : message); } /** @@ -42936,12 +42936,12 @@ async function react(octokit, context) { } } async function refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) { - error(msg); + core_error(msg); try { await createComment(octokit, context, issueNumber, msg); } catch (commentE) { - error(`Could not comment with an auth error: ${commentE}`); + core_error(`Could not comment with an auth error: ${commentE}`); } throw cause; } @@ -43374,7 +43374,7 @@ async function write(result, name, done, action) { } catch (e) { const message = e instanceof Error ? e.message : String(e); - error(`label-sync: could not sync ${name}: ${message}`); + core_error(`label-sync: could not sync ${name}: ${message}`); result.failures.push({ name, message }); } } @@ -43800,7 +43800,7 @@ async function evaluateMerge(octokit, context, number, source, lgtm = defaultLgt if (outcome.status === 403 && first.fork && await explainForkWorkflows(octokit, context, number, first)) { return skip(number, 'fork pull request with workflow changes: the token may not merge it'); } - error(`could not merge pr #${number}: ${outcome.message}`); + core_error(`could not merge pr #${number}: ${outcome.message}`); return outcome; } let warnedMergeMethodIgnored = false; @@ -43895,11 +43895,11 @@ async function evaluateInQueue(octokit, context, number, tide, first, queue) { return skip(number, `not ready for the merge queue: ${outcome.message}`); case 'forbidden': { const message = `cannot add pr #${number} to the merge queue: the token may not enqueue (grant contents: write and pull-requests: write, or pass a token that can — see automatic-merging.md#merge-queues): ${outcome.message}`; - error(message); + core_error(message); return { result: 'failed', message }; } default: - error(`could not enqueue pr #${number}: ${outcome.message}`); + core_error(`could not enqueue pr #${number}: ${outcome.message}`); return { result: 'failed', message: outcome.message }; } } @@ -44120,7 +44120,8 @@ function pullNumber(context) { * through the shared merge path: the lgtm binding, GitHub's mergeability, * then the merge. It is the backstop of the event-driven tide handlers. * Every PR is attempted; once all pages are processed the run fails - * if any merge was refused, listing the affected PRs. + * if any merge was refused or any evaluation threw, listing the affected PRs. + * Only a listing that cannot be read stops the run early. * * @param currentPage - the page to return from the github api * @param context - The github actions event context @@ -44150,7 +44151,7 @@ async function cronLgtm(currentPage, context, progress = { jobsDone: 0, failures } return progress.jobsDone; } - const results = await Promise.all(prs.map(async (pr) => { + await Promise.all(prs.map(async (pr) => { info(`processing pr: ${pr.number}`); if (pr.state === 'closed') { return; @@ -44164,14 +44165,11 @@ async function cronLgtm(currentPage, context, progress = { jobsDone: 0, failures } } catch (error) { - return error; + // collected like a refused merge, so one PR that cannot be evaluated does not stop the others + core_error(`could not evaluate pr #${pr.number}: ${error}`); + progress.failures.push({ number: pr.number, message: `could not evaluate: ${error}` }); } })); - for (const result of results) { - if (result instanceof Error) { - throw new TypeError(`error processing pr: ${result}`); - } - } // Recurse, continue to next page return await cronLgtm(currentPage + 1, context, progress); } @@ -45608,12 +45606,12 @@ async function assertReviewer(octokit, context, issueNumber, commenterId) { } // refuse logs and replies with msg, then fails the run with cause (or msg) async function lgtm_refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) { - error(msg); + core_error(msg); try { await createComment(octokit, context, issueNumber, msg); } catch (commentE) { - error(`Could not comment with an auth error: ${commentE}`); + core_error(`Could not comment with an auth error: ${commentE}`); } throw cause; } @@ -45756,12 +45754,12 @@ async function authorize(octokit, context, issueNumber, commenterLogin) { } // refuse logs and replies with msg, then fails the run with cause (or msg) async function approve_refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) { - error(msg); + core_error(msg); try { await createComment(octokit, context, issueNumber, msg); } catch (commentE) { - error(`Could not comment with an auth error: ${commentE}`); + core_error(`Could not comment with an auth error: ${commentE}`); } throw cause; } @@ -46918,7 +46916,7 @@ async function run() { const context = github_context; const handler = Object.hasOwn(eventHandlers, context.eventName) ? eventHandlers[context.eventName] : undefined; if (!handler) { - error(`${context.eventName} not yet supported`); + core_error(`${context.eventName} not yet supported`); return; } await handler(context); diff --git a/docs/automatic-merging.md b/docs/automatic-merging.md index 3427b05c..d0913186 100644 --- a/docs/automatic-merging.md +++ b/docs/automatic-merging.md @@ -206,8 +206,11 @@ jobs: Locked and closed PRs are skipped. Every eligible PR is attempted, so one un-mergeable PR does not stop the others. A PR GitHub reports as not mergeable is skipped with the state logged; a stale `lgtm` is stripped. Each refused merge is logged as an error annotation -(`could not merge pr #: `); once all pages are processed the run fails if any merge -was refused, listing the PRs: `2 pull request(s) could not be merged: #1 (Pull Request is not mergeable), #7 (...)`. +(`could not merge pr #: `), and so is a PR whose evaluation fails (`could not evaluate +pr #: `, for example a `500` reading it); the remaining PRs and pages are still +evaluated. Once all pages are processed the run fails if any merge was refused or any evaluation +failed, listing the PRs: `2 pull request(s) could not be merged: #1 (Pull Request is not mergeable), #7 (could not evaluate: ...)`. +Only a listing of the open pull requests that cannot be read stops the run early. With event-driven merging in place an hourly or daily schedule is plenty; drop the cron entirely if a missed event is acceptable. diff --git a/docs/cron-jobs.md b/docs/cron-jobs.md index 99583c97..3906b6f5 100644 --- a/docs/cron-jobs.md +++ b/docs/cron-jobs.md @@ -9,7 +9,7 @@ evaluated in this run`); event handlers never dedupe. Jobs | Description | Permissions --- | --- | --- -`lgtm` | Backstop for [event-driven merging](./automatic-merging.md#event-driven-merging): catches events GitHub dropped or runs that were skipped; comment commands (`/lgtm`, `/approve`, `/unhold`) evaluate the PR themselves and do not need it. Pages through the repository's open pull requests, following pages until one comes back empty, and sends every one that passes the [merge gate](./automatic-merging.md#the-merge-gate) on its listed labels (`tide.labels` present, no `tide.missing_labels`; by default `lgtm` and none of `do-not-merge/*`, `needs-rebase`, `hold`) through the shared merge path: the [`lgtm` binding](./automatic-merging.md#lgtm-is-bound-to-a-commit) (a stale `lgtm` is stripped, not merged), then `mergeable_state` (`clean` and `has_hooks` merge). A PR that fails the gate on its listed labels costs no further call. On a branch that requires a merge queue the PR is enqueued instead (`enqueued pr #`), which counts as done ([merge queues](./automatic-merging.md#merge-queues)). Skips locked and closed PRs and logs why each other PR was skipped; a refused merge fails the run listing those PRs. With the events subscribed an hourly or daily schedule is enough, or drop the job. | `contents: write`, `pull-requests: write`, `statuses: write` +`lgtm` | Backstop for [event-driven merging](./automatic-merging.md#event-driven-merging): catches events GitHub dropped or runs that were skipped; comment commands (`/lgtm`, `/approve`, `/unhold`) evaluate the PR themselves and do not need it. Pages through the repository's open pull requests, following pages until one comes back empty, and sends every one that passes the [merge gate](./automatic-merging.md#the-merge-gate) on its listed labels (`tide.labels` present, no `tide.missing_labels`; by default `lgtm` and none of `do-not-merge/*`, `needs-rebase`, `hold`) through the shared merge path: the [`lgtm` binding](./automatic-merging.md#lgtm-is-bound-to-a-commit) (a stale `lgtm` is stripped, not merged), then `mergeable_state` (`clean` and `has_hooks` merge). A PR that fails the gate on its listed labels costs no further call. On a branch that requires a merge queue the PR is enqueued instead (`enqueued pr #`), which counts as done ([merge queues](./automatic-merging.md#merge-queues)). Skips locked and closed PRs and logs why each other PR was skipped; a refused merge or a PR whose evaluation fails (`could not evaluate pr #: `) does not stop the others, and fails the run at the end listing those PRs. With the events subscribed an hourly or daily schedule is enough, or drop the job. | `contents: write`, `pull-requests: write`, `statuses: write` `sweep` | For the [`pull_request` install mode](./installing.md#without-pull_request_target): evaluates every open pull request updated within [`sweep.lookback`](#sweep) the way the `pull_request` and `pull_request_review` handlers would have, which those events cannot do for fork pull requests (read-only token). `needs-*` rules, OWNERS labels, reviewers, approval, the pending runs of PRs labeled `ok-to-test`, the merge path. | `contents: write`, `issues: write`, `pull-requests: write`, `statuses: write`, `actions: write` `label-sync` | Creates the labels the prow configuration describes and updates the color or description of those that drifted. Never deletes or renames a label. | `contents: read`, `issues: write` diff --git a/src/cronJobs/lgtm.ts b/src/cronJobs/lgtm.ts index 1abee8e1..a6f620ee 100644 --- a/src/cronJobs/lgtm.ts +++ b/src/cronJobs/lgtm.ts @@ -42,7 +42,8 @@ interface MergePolicy { * through the shared merge path: the lgtm binding, GitHub's mergeability, * then the merge. It is the backstop of the event-driven tide handlers. * Every PR is attempted; once all pages are processed the run fails - * if any merge was refused, listing the affected PRs. + * if any merge was refused or any evaluation threw, listing the affected PRs. + * Only a listing that cannot be read stops the run early. * * @param currentPage - the page to return from the github api * @param context - The github actions event context @@ -81,7 +82,7 @@ export async function cronLgtm( return progress.jobsDone } - const results = await Promise.all( + await Promise.all( prs.map(async (pr) => { core.info(`processing pr: ${pr.number}`) if (pr.state === 'closed') { @@ -98,17 +99,13 @@ export async function cronLgtm( } } catch (error) { - return error + // collected like a refused merge, so one PR that cannot be evaluated does not stop the others + core.error(`could not evaluate pr #${pr.number}: ${error}`) + progress.failures.push({ number: pr.number, message: `could not evaluate: ${error}` }) } }), ) - for (const result of results) { - if (result instanceof Error) { - throw new TypeError(`error processing pr: ${result}`) - } - } - // Recurse, continue to next page return await cronLgtm(currentPage + 1, context, progress) }