diff --git a/__tests__/bundle/blunderbussDelay.test.ts b/__tests__/bundle/blunderbussDelay.test.ts new file mode 100644 index 00000000..b771bfe3 --- /dev/null +++ b/__tests__/bundle/blunderbussDelay.test.ts @@ -0,0 +1,243 @@ +import type { FakeGithub } from './fakeGithub' +import { Buffer } from 'node:buffer' +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest' + +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json' +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import { blobSha } from '../utils/ownersData' +import { start } from './fakeGithub' +import { comment, configReads, helpersFor, repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +describe('dist/index.js delayed blunderbuss', () => { + let gh: FakeGithub + const { routeOwners, expectRequests } = helpersFor(() => gh) + const owners = { OWNERS: 'reviewers:\n- bob\n' } + const timelinePath = `${repo}/issues/1/timeline` + + beforeAll(async () => { + gh = await start() + }) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function configure(section = ' delay: 1h\n') { + const file = structuredClone(labelFileContents) + file.content = Buffer.from(`blunderbuss:\n${section}`).toString('base64') + gh.route('GET', '/repos/Codertocat/.project/contents/prow.yaml', { status: 200, body: file }) + } + + function ago(minutes: number) { + return new Date(Date.now() - minutes * 60_000).toISOString() + } + + function pull(overrides: Record = {}) { + return { + ...structuredClone(pullReqListPulls[0]), + number: 1, + created_at: ago(70), + updated_at: ago(70), + labels: [], + requested_reviewers: [], + requested_teams: [], + assignees: [], + draft: false, + user: { login: 'Codertocat' }, + base: { ref: 'master', sha: 'basesha' }, + head: { sha: 'sha1', repo: { full_name: 'Codertocat/Hello-World' } }, + ...overrides, + } + } + + function routeSweep(pr = pull(), timeline: unknown[] = [], reviews: unknown[] = []) { + gh.route('GET', new RegExp(`^${repo}/pulls\\?`), { status: 200, body: [pr] }) + gh.route('GET', `${repo}/git/trees/master`, { + status: 200, + body: { sha: 'master', truncated: false, tree: [{ path: 'OWNERS', type: 'blob', sha: blobSha('OWNERS') }] }, + }) + routeOwners(owners, ['x.go'], pr) + gh.route('GET', timelinePath, { status: 200, body: timeline }) + gh.route('GET', `${repo}/pulls/1/reviews`, { status: 200, body: reviews }) + gh.route('POST', `${repo}/pulls/1/requested_reviewers`, { status: 201, body: {} }) + // Recent candidates still run the other sweep plugins. + gh.route('GET', `${repo}/issues/1`, { status: 200, body: { labels: [] } }) + gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'approved' }, { name: 'lgtm' }] }) + gh.route('GET', `${repo}/issues/1/comments`, { status: 200, body: [] }) + gh.route('POST', `${repo}/issues/1/comments`, { status: 201, body: {} }) + } + + function runSweep() { + return runBundle({ eventName: 'schedule', payload: {}, inputs: { ...token, jobs: 'sweep' }, apiUrl: gh.url }) + } + + function requested() { + return gh.requestsMatching('POST', /\/requested_reviewers$/).map(r => r.body) + } + + function timelineReads() { + return gh.requestsMatching('GET', /\/timeline\?per_page=100$/) + } + + function reviewReads() { + return gh.requestsMatching('GET', /\/reviews\?per_page=1$/) + } + + it.each(['opened', 'ready_for_review'])('%s defers reviewer assignment to the scheduled sweep', async (action) => { + configure() + routeOwners(owners, ['x.go']) + + const result = await runBundle({ + eventName: 'pull_request', + payload: { ...pullReqOpenedEvent, action }, + inputs: token, + apiUrl: gh.url, + }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain(`#1 ${action}; blunderbuss.delay is set`) + expect(requested()).toEqual([]) + expect(timelineReads()).toEqual([]) + }) + + it('/auto-cc still immediately requests reviewers on a draft when delay is configured', async () => { + configure() + routeOwners(owners, ['x.go'], { draft: true }) + gh.route('POST', `${repo}/pulls/1/requested_reviewers`, { status: 201, body: {} }) + const payload = comment('/auto-cc') + Object.assign(payload.issue, { pull_request: { url: `${gh.url}${repo}/pulls/1` } }) + + const result = await runBundle({ + eventName: 'issue_comment', + payload, + inputs: { ...token, 'prow-commands': '/auto-cc' }, + apiUrl: gh.url, + }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(requested()).toEqual([{ reviewers: ['bob'] }]) + expect(timelineReads()).toEqual([]) + }) + + it('requests reviewers for an untouched due pull outside the normal lookback without running other sweep steps', async () => { + configure() + routeSweep() + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('sweep: 0 candidates updated since') + expect(result.stdout).toContain('sweep: 1 more checked for delayed reviewers') + expect(requested()).toEqual([{ reviewers: ['bob'] }]) + expect(timelineReads()).toHaveLength(1) + expect(reviewReads()).toHaveLength(1) + expect(gh.requests.filter(r => r.method !== 'GET').map(r => r.path)).toEqual([`${repo}/pulls/1/requested_reviewers`]) + expect(gh.requestsMatching('GET', /\/issues\/1\/comments|\/actions\/runs/)).toEqual([]) + }) + + it.each([ + ['a draft', { draft: true }, [], 'is a draft', 0], + ['a manual reviewer', { requested_reviewers: [{ login: 'alice' }] }, [], 'already has requested reviewers', 0], + ['a manual team', { requested_teams: [{ slug: 'sdk' }] }, [], 'already has requested reviewers', 0], + ['an existing review', {}, [{ id: 7, user: { login: 'alice' }, state: 'COMMENTED' }], 'already has reviews', 1], + ])('does not assign a due pull with %s', async (_name, overrides, reviews, message, reads) => { + configure() + routeSweep(pull(overrides), [], reviews) + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain(`sweep: #1 ${message}`) + expect(requested()).toEqual([]) + expect(timelineReads()).toHaveLength(reads) + expect(reviewReads()).toHaveLength(reads) + }) + + it.each([ + ['not yet due', 30, 30, 'is not due for reviewers', 0], + ['opened before the due window', 180, 70, 'outside the due window', 1], + ])('leaves a pull %s alone', async (_name, created, updated, message, reads) => { + configure() + routeSweep(pull({ created_at: ago(created), updated_at: ago(updated) })) + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain(message) + expect(requested()).toEqual([]) + expect(timelineReads()).toHaveLength(reads) + expect(reviewReads()).toEqual([]) + }) + + it.each([ + ['due', 70, true], + ['recently marked ready again', 10, false], + ])('uses the latest ready_for_review clock for a former draft that is %s', async (_name, latest, due) => { + configure() + routeSweep(pull({ created_at: ago(300), updated_at: ago(latest) }), [ + { event: 'ready_for_review', created_at: ago(latest) }, + { event: 'ready_for_review', created_at: ago(240) }, + { event: 'labeled', created_at: ago(5) }, + { event: 'ready_for_review' }, + ]) + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(timelineReads()).toHaveLength(1) + expect(reviewReads()).toHaveLength(due ? 1 : 0) + expect(requested()).toEqual(due ? [{ reviewers: ['bob'] }] : []) + if (!due) { + expect(result.stdout).toContain('outside the due window') + expect(result.stdout).toContain('sweep: #1 evaluated') + } + }) + + it('uses creation time and requests on a due draft when ignore_drafts is false', async () => { + configure(' delay: 1h\n ignore_drafts: false\n') + routeSweep(pull({ draft: true })) + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(requested()).toEqual([{ reviewers: ['bob'] }]) + expect(timelineReads()).toEqual([]) + }) + + it('fails closed when a delayed pull timeline cannot be read', async () => { + configure() + gh.route('GET', timelinePath, { status: 403, body: { message: 'timeline denied' } }) + routeSweep() + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.join('\n')).toContain('sweep: 1 pull request(s) failed: #1 (blunderbuss: could not read the timeline: HttpError: timeline denied)') + expect(timelineReads()).toHaveLength(1) + expect(reviewReads()).toEqual([]) + expect(requested()).toEqual([]) + }) + + it.each([ + ['3600', 'blunderbuss.delay must be a duration string such as 1h or 30m'], + ['later', 'invalid blunderbuss.delay \'later\': expected a duration such as 5s, 2m or 500ms'], + ['25h', 'blunderbuss.delay must not be longer than 24h'], + ])('refuses invalid delay %s before listing or mutating pulls', async (delay, message) => { + configure(` delay: ${delay}\n`) + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.join('\n')).toContain(`Codertocat/.project:prow.yaml: ${message}`) + expectRequests(configReads({ org: '.project' }), []) + }) +}) diff --git a/__tests__/cronJobTest/sweep.test.ts b/__tests__/cronJobTest/sweep.test.ts index 4ad11be3..c82217e1 100644 --- a/__tests__/cronJobTest/sweep.test.ts +++ b/__tests__/cronJobTest/sweep.test.ts @@ -33,6 +33,7 @@ interface PullSpec { labels?: string[] draft?: boolean requested_reviewers?: string[] + requested_teams?: string[] head?: string mergeable_state?: string user?: string @@ -51,6 +52,7 @@ function pull(spec: PullSpec) { created_at: spec.created ?? spec.updated ?? ago(10 * 60_000), updated_at: spec.updated ?? ago(10 * 60_000), requested_reviewers: (spec.requested_reviewers ?? []).map(login => ({ login })), + requested_teams: (spec.requested_teams ?? []).map(slug => ({ slug })), assignees: [], user: { login: spec.user ?? 'some-author' }, head: { sha: spec.head ?? `sha${spec.number}` }, @@ -112,6 +114,9 @@ describe('resolveSweepLookback', () => { ['a non-string lookback', 'sweep:\n lookback: 3600\n', 'x: sweep.lookback must be a duration string such as 1h or 30m'], ['a malformed lookback', 'sweep:\n lookback: soon\n', 'x: invalid sweep.lookback \'soon\': expected a duration such as 5s, 2m or 500ms'], ['a zero lookback', 'sweep:\n lookback: \'0\'\n', 'x: sweep.lookback must be longer than 0'], + ['a non-string blunderbuss.delay', 'blunderbuss:\n delay: 3600\n', 'x: blunderbuss.delay must be a duration string such as 1h or 30m'], + ['a malformed blunderbuss.delay', 'blunderbuss:\n delay: later\n', 'x: invalid blunderbuss.delay \'later\': expected a duration such as 5s, 2m or 500ms'], + ['a blunderbuss.delay over 24h', 'blunderbuss:\n delay: 25h\n', 'x: blunderbuss.delay must not be longer than 24h'], ])('rejects %s', (_, text, error) => { expect(() => parseProwConfig('x', text)).toThrow(error) }) @@ -489,6 +494,82 @@ describe('sweep on a repository with OWNERS files', () => { }) }) +describe('sweep with blunderbuss.delay', () => { + const owners = { OWNERS: 'approvers:\n- alice\nreviewers:\n- carol\n' } + + function serve(specs: PullSpec[], timelines: Record = {}, reviewed: number[] = []) { + const tree = Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) })) + const requested: number[] = [] + const timelineReads: number[] = [] + server.use( + prowYaml('blunderbuss:\n delay: 1h\n'), + utils.defaultBranchTree(Object.keys(owners)), + http.get(`${repo}/git/trees/basesha`, utils.mockResponse(200, { sha: 'basesha', truncated: false, tree })), + http.get(`${repo}/pulls/:number/files`, utils.mockResponse(200, [{ filename: 'x.go', status: 'modified' }])), + ...Object.entries(owners).map(([path, contents]) => http.get(`${repo}/git/blobs/${blobSha(path)}`, utils.mockResponse(200, { encoding: 'base64', content: Buffer.from(contents).toString('base64') }))), + http.get(`${repo}/issues/:number`, utils.mockResponse(200, { labels: [] })), + http.get(`${repo}/issues/:number/comments`, utils.mockResponse(200, [])), + http.post(`${repo}/issues/:number/comments`, utils.mockResponse(201, {})), + http.get(`${repo}/issues/:number/timeline`, ({ params }) => { + timelineReads.push(Number(params.number)) + return json((timelines[Number(params.number)] ?? []).map(at => ({ event: 'ready_for_review', created_at: at }))) + }), + http.get(`${repo}/pulls/:number/reviews`, ({ params }) => json(reviewed.includes(Number(params.number)) ? [{ id: 1, state: 'COMMENTED', user: { login: 'carol' } }] : [])), + http.post(`${repo}/pulls/:number/requested_reviewers`, ({ params }) => { + requested.push(Number(params.number)) + return json({}, 201) + }), + utils.repoHasLabels(['approved', 'lgtm']), + ) + servePulls(specs) + return { requested, timelineReads } + } + + it('requests on a pull request opened an hour ago that nobody touched since, outside the normal window', async () => { + const { requested } = serve([{ number: 1, created: ago(70 * 60_000), updated: ago(70 * 60_000) }]) + + await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] }) + expect(requested).toEqual([1]) + }) + + it('does not request before the delay has passed, nor after the due window', async () => { + const { requested, timelineReads } = serve([ + { number: 1, created: ago(30 * 60_000), updated: ago(30 * 60_000) }, + { number: 2, created: ago(3 * hour), updated: ago(90 * 60_000) }, + ]) + + await expect(sweep(context)).resolves.toMatchObject({ failures: [] }) + expect(requested).toEqual([]) + // #1 is not due yet: no timeline read; #2 was opened long ago and never marked ready + expect(timelineReads).toEqual([2]) + }) + + it('leaves alone a pull request with a requested reviewer, a requested team, a review or that is a draft', async () => { + const { requested } = serve([ + { number: 1, created: ago(70 * 60_000), updated: ago(70 * 60_000), requested_reviewers: ['bob'] }, + { number: 2, created: ago(70 * 60_000), updated: ago(70 * 60_000), requested_teams: ['sdk'] }, + { number: 3, created: ago(70 * 60_000), updated: ago(70 * 60_000) }, + { number: 4, created: ago(70 * 60_000), updated: ago(70 * 60_000), draft: true }, + ], {}, [3]) + + await expect(sweep(context)).resolves.toMatchObject({ failures: [] }) + expect(requested).toEqual([]) + }) + + it('times a former draft from its last ready_for_review', async () => { + const { requested } = serve([ + { number: 1, created: ago(5 * hour), updated: ago(70 * 60_000) }, + { number: 2, created: ago(70 * 60_000), updated: ago(10 * 60_000) }, + ], { + 1: [ago(4 * hour), ago(70 * 60_000)], + 2: [ago(10 * 60_000)], + }) + + await expect(sweep(context)).resolves.toMatchObject({ candidates: [2], failures: [] }) + expect(requested).toEqual([1]) + }) +}) + describe('sweep and the ok-to-test label', () => { function serveRuns(runs: unknown[], approved: number[]) { server.use( diff --git a/__tests__/plugins/blunderbuss.test.ts b/__tests__/plugins/blunderbuss.test.ts index 7a659513..2483bffd 100644 --- a/__tests__/plugins/blunderbuss.test.ts +++ b/__tests__/plugins/blunderbuss.test.ts @@ -122,9 +122,14 @@ describe('blunderbussSettings', () => { exclude_approvers: false, ignore_drafts: true, ignore_authors: [], + delay: 0, }) }) + it('reads delay as milliseconds', () => { + expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { delay: '1h30m' }, sources: [] }).delay).toBe(90 * 60_000) + }) + it('lowercases ignore_authors', () => { expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { ignore_authors: ['Dependabot[bot]'] }, sources: [] }).ignore_authors).toEqual(['dependabot[bot]']) }) @@ -225,6 +230,18 @@ describe('blunderbuss handler', () => { expect(debug).toHaveBeenCalledWith('blunderbuss: #1 is a draft, waiting for ready_for_review') }) + it.each(['opened', 'ready_for_review'])('delay: %s requests nobody and leaves it to the sweep job', async (action) => { + serveConfig('blunderbuss:\n delay: 1h\n') + servePr(owners, ['sdk/x.go']) + const observe = observeRequestReviewers() + const info = vi.spyOn(core, 'info').mockImplementation(() => {}) + + await blunderbuss(prEvent(action), first) + + await expect(observe.notCalled()).resolves.toBe('not called') + expect(info).toHaveBeenCalledWith(`blunderbuss: #1 ${action}; blunderbuss.delay is set, so the sweep job requests reviewers once it is due and nobody has been asked`) + }) + it('ready_for_review: requests reviewers for the former draft', async () => { serveConfig() servePr(owners, ['sdk/x.go']) diff --git a/dist/index.js b/dist/index.js index f2323a6a..51090d73 100644 --- a/dist/index.js +++ b/dist/index.js @@ -40850,6 +40850,7 @@ const defaultAuthorization = { users: [], }; const defaultSweepLookback = '1h'; +const maxBlunderbussDelayMs = 24 * 3_600_000; const maxSweepLookbackMs = 24 * 3_600_000; /** repositories of the owner that may hold an organization wide prow.yaml, in precedence order */ const orgConfigRepos = ['.project', '.github']; @@ -41200,12 +41201,28 @@ function normalizeBlunderbuss(source, raw) { if (raw.ignore_authors !== undefined && !isStringList(raw.ignore_authors)) { throw new Error(`${source}: blunderbuss.ignore_authors must be a list of GitHub usernames`); } + if (raw.delay !== undefined) { + if (typeof raw.delay !== 'string') { + throw new TypeError(`${source}: blunderbuss.delay must be a duration string such as 1h or 30m`); + } + let ms; + try { + ms = parseDuration(raw.delay, 'blunderbuss.delay'); + } + catch (e) { + throw new Error(`${source}: ${e instanceof Error ? e.message : e}`); + } + if (ms > maxBlunderbussDelayMs) { + throw new Error(`${source}: blunderbuss.delay must not be longer than 24h`); + } + } return stripUndefined({ request_count: raw.request_count, max_request_count: raw.max_request_count, exclude_approvers: raw.exclude_approvers, ignore_drafts: raw.ignore_drafts, ignore_authors: raw.ignore_authors, + delay: raw.delay, }); } const approveFlags = ['require_self_approval', 'ignore_review_state', 'lgtm_acts_as_approve', 'github_review']; @@ -44880,6 +44897,7 @@ async function evaluateOnOwnersRepo(context, pullNumber) { + /** * blunderbussSettings resolves the `blunderbuss` configuration with Prow's * defaults: two reviewers, approvers count, drafts wait for ready_for_review. @@ -44894,6 +44912,7 @@ function blunderbussSettings(config) { exclude_approvers: raw.exclude_approvers ?? false, ignore_drafts: raw.ignore_drafts ?? true, ignore_authors: (raw.ignore_authors ?? []).map(login => login.toLowerCase()), + delay: parseDuration(raw.delay, 'blunderbuss.delay'), }; } /** @@ -44935,6 +44954,7 @@ function sample(items, count, rng) { * blunderbuss is the `pull_request` handler modelled on Prow's blunderbuss * plugin: on `opened` (and `ready_for_review` when drafts are ignored) it * requests reviews from the OWNERS reviewers covering the changed files. + * With `blunderbuss.delay` it defers to the `sweep` job instead. * * @param context - the github context of the current action event * @param rng - a source of numbers in [0, 1), injectable for tests @@ -44955,6 +44975,10 @@ async function blunderbuss(context = github_context, rng = Math.random) { core_debug(`blunderbuss: skipping ${action} action`); return; } + if (settings.delay > 0) { + info(`blunderbuss: #${pullNumber} ${action}; blunderbuss.delay is set, so the sweep job requests reviewers once it is due and nobody has been asked`); + return; + } await requestOwnersReviewers(octokit, context, pullNumber, settings, { explicit: false, rng }); } /** @@ -45338,6 +45362,11 @@ const pageSize = 100; * failure on one is collected and the rest still run. The run fails at the * end listing the failures. * + * With `blunderbuss.delay` the window reaches back a further `delay`: a pull + * request opened (or marked ready) inside the shifted window is due for + * reviewers now, even when nothing has touched it since. Those older pull + * requests get the blunderbuss step only. + * * @param context - the github actions event context * @param now - the current time, injectable for tests */ @@ -45346,13 +45375,27 @@ async function sweep(context = github_context, now = new Date()) { const config = await loadProwConfig(octokit, context); const lookbackMs = resolveSweepLookback(config.sweep); const since = new Date(now.getTime() - lookbackMs); - const candidates = await recentlyUpdatedPulls(octokit, context, since); + const reviewers = blunderbussSettings(config); + const due = { from: new Date(since.getTime() - reviewers.delay), to: new Date(now.getTime() - reviewers.delay) }; + const listed = await recentlyUpdatedPulls(octokit, context, due.from); + const candidates = listed.filter(pr => new Date(pr.updated_at) >= since); + const delayedOnly = listed.filter(pr => new Date(pr.updated_at) < since); info(`sweep: ${candidates.length} candidate${candidates.length === 1 ? '' : 's'} updated since ${since.toISOString()}`); - const result = { candidates: candidates.map(pr => pr.number), merged: [], enqueued: [], failures: [] }; - if (candidates.length === 0) { - return result; + if (reviewers.delay > 0) { + info(`sweep: ${delayedOnly.length} more checked for delayed reviewers, updated since ${due.from.toISOString()}`); } - const plugins = { lgtm: lgtmSettings(config), config, since }; + const result = { candidates: candidates.map(pr => pr.number), merged: [], enqueued: [], failures: [] }; + const plugins = { lgtm: lgtmSettings(config), config, reviewers, since, due }; + await forEachLimited(delayedOnly, sweepConcurrency, async (pr) => { + try { + if (await branchHasOwners(octokit, context, pr.base.ref)) { + await requestReviewersIfDue(octokit, context, pr, plugins); + } + } + catch (e) { + result.failures.push({ number: pr.number, message: `blunderbuss: ${e instanceof Error ? e.message : String(e)}` }); + } + }); await forEachLimited(candidates, sweepConcurrency, async (pr) => { const outcome = await sweepPullRequest(octokit, context, pr, plugins); if (outcome.result === 'merged') { @@ -45378,7 +45421,9 @@ async function sweepPullRequest(octokit, context, pr, plugins) { const tide = await loadTide(octokit, context, pr.base.ref); const ownersSteps = [ ['owners-label', () => applyOwnersLabels(octokit, context, pr.number)], - ['blunderbuss', () => requestReviewersIfFresh(octokit, context, pr, plugins)], + ['blunderbuss', () => plugins.reviewers.delay > 0 + ? requestReviewersIfDue(octokit, context, pr, plugins) + : requestReviewersIfFresh(octokit, context, pr, plugins)], ['approve', () => evaluateApproval(octokit, context, pr.number)], ]; const steps = [ @@ -45427,7 +45472,58 @@ async function requestReviewersIfFresh(octokit, context, pr, plugins) { core_debug(`sweep: #${pr.number} already has reviews`); return; } - await requestOwnersReviewers(octokit, context, pr.number, blunderbussSettings(plugins.config), { explicit: false, rng: Math.random }); + await requestOwnersReviewers(octokit, context, pr.number, plugins.reviewers, { explicit: false, rng: Math.random }); +} +/** + * requestReviewersIfDue is blunderbuss with `delay`: a pull request whose + * review clock (opened, or the last `ready_for_review` when drafts are + * ignored) started inside the due window, and that still has no requested + * reviewer or team and no review, gets reviewers now. + */ +async function requestReviewersIfDue(octokit, context, pr, plugins) { + const { reviewers: settings, due } = plugins; + if (settings.ignore_drafts && pr.draft === true) { + core_debug(`sweep: #${pr.number} is a draft`); + return; + } + if ((pr.requested_reviewers ?? []).length > 0 || (pr.requested_teams ?? []).length > 0) { + core_debug(`sweep: #${pr.number} already has requested reviewers`); + return; + } + let start = new Date(pr.created_at); + if (start > due.to) { + core_debug(`sweep: #${pr.number} is not due for reviewers before ${new Date(start.getTime() + settings.delay).toISOString()}`); + return; + } + if (settings.ignore_drafts) { + const ready = await lastReadyForReview(octokit, context, pr.number); + if (ready !== undefined && ready > start) { + start = ready; + } + } + if (start < due.from || start > due.to) { + core_debug(`sweep: #${pr.number} became ready at ${start.toISOString()}, outside the due window`); + return; + } + const { data: reviews } = await octokit.pulls.listReviews({ ...context.repo, pull_number: pr.number, per_page: 1 }); + if (reviews.length > 0) { + core_debug(`sweep: #${pr.number} already has reviews`); + return; + } + await requestOwnersReviewers(octokit, context, pr.number, settings, { explicit: false, rng: Math.random }); +} +async function lastReadyForReview(octokit, context, pullNumber) { + let events; + try { + events = await octokit.paginate(octokit.issues.listEventsForTimeline, { ...context.repo, issue_number: pullNumber, per_page: 100 }); + } + catch (e) { + throw new Error(`could not read the timeline: ${e}`); + } + const times = events + .filter(event => event.event === 'ready_for_review' && event.created_at !== undefined) + .map(event => new Date(event.created_at).getTime()); + return times.length === 0 ? undefined : new Date(Math.max(...times)); } async function recentlyUpdatedPulls(octokit, context, since) { const candidates = []; diff --git a/docs/configuration.md b/docs/configuration.md index bc2348f6..2c3f0111 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -314,6 +314,7 @@ Field | Default | Meaning `exclude_approvers` | `false` | only `reviewers` are candidates, not `approvers` `ignore_drafts` | `true` | a draft waits for `ready_for_review`; `false` requests on `opened` and skips `ready_for_review` `ignore_authors` | `[]` | pull requests by these authors get no automatic request (case-insensitive) +`delay` | `0` | wait this long after `opened` (or `ready_for_review`), then request only if the pull request still has no requested reviewer or team and no review; a Go style duration (`30m`, `1h`), at most `24h`. The [`sweep` job](./cron-jobs.md#sweep) makes the delayed request, so it must run on a schedule Event | Action --- | --- @@ -329,6 +330,21 @@ random among equals. With `max_request_count`, `request_count` is reduced so tha already requested reviewers plus the new ones do not exceed it. Nobody left to request is a debug-logged no-op; a refused request fails the run. No comment is posted. +With `delay` the `opened` and `ready_for_review` handlers request nothing and log that the +request is deferred. Each [`sweep`](./cron-jobs.md#sweep) run then requests reviewers on every +pull request whose review clock started between `now − delay − lookback` and `now − delay` and +that nobody has been asked to review yet. The clock starts at `opened`, or at the last +`ready_for_review` when `ignore_drafts` is on (one timeline read per such pull request). The +sweep's list reaches back the extra `delay`, so a pull request nobody touched since it opened is +still found; those older pull requests get this step only. Install the sweep job even under +`pull_request_target`; its other steps are idempotent. A request lands up to one sweep interval +after it is due, plus GitHub's scheduling delay. `/auto-cc` is never delayed. + +```yaml +blunderbuss: + delay: 1h +``` + The workflow token needs `pull-requests: write` and the workflow must subscribe to `ready_for_review` for drafts; see [events](./events.md#issues-and-pull_request). diff --git a/docs/cron-jobs.md b/docs/cron-jobs.md index 3906b6f5..bd636cea 100644 --- a/docs/cron-jobs.md +++ b/docs/cron-jobs.md @@ -26,7 +26,7 @@ Step | Reuses | Does | Skipped when --- | --- | --- | --- 1 | [`require_matching_label`](./configuration.md#require_matching_label) | every PR rule, no grace period | no rule is configured 2 | [`owners-label`](./labeling.md#labels-from-owners-files) | adds the missing OWNERS `labels:` | the repository has no OWNERS files -3 | [`blunderbuss`](./configuration.md#blunderbuss) | requests reviewers like `opened` would; honours `ignore_authors` and `ignore_drafts` | no OWNERS files; the PR was created before the window; it has requested reviewers, reviews, or is a draft +3 | [`blunderbuss`](./configuration.md#blunderbuss) | requests reviewers like `opened` would; honours `ignore_authors` and `ignore_drafts` | no OWNERS files; the PR was created before the window; it has requested reviewers, reviews, or is a draft. With [`blunderbuss.delay`](./configuration.md#blunderbuss): the PR's review clock did not start inside the window shifted back by `delay`; it has requested reviewers or teams, or reviews 4 | [`approve`](./commands.md#approve) | recomputes `approved` and the notifier | no OWNERS files 5 | [`ok-to-test`](./commands.md#trigger) | approves the workflow runs waiting for approval on the head | the PR does not carry `ok-to-test` 6 | [`tide`](./automatic-merging.md) | the label gate, the [`lgtm` binding](./automatic-merging.md#lgtm-is-bound-to-a-commit) (a stale `lgtm` is stripped), `mergeable_state`, the merge | never; `tide.merge_on_events` does not apply, the sweep is a scheduled job like `lgtm` diff --git a/src/cronJobs/sweep.ts b/src/cronJobs/sweep.ts index 303be450..43186e9f 100644 --- a/src/cronJobs/sweep.ts +++ b/src/cronJobs/sweep.ts @@ -1,4 +1,5 @@ import type { Octokit, RestEndpointMethodTypes } from '@octokit/rest' +import type { BlunderbussSettings } from '../plugins/blunderbuss' import type { ProwConfig } from '../utils/config' import type { Context } from '../utils/context' @@ -47,6 +48,11 @@ const pageSize = 100 * failure on one is collected and the rest still run. The run fails at the * end listing the failures. * + * With `blunderbuss.delay` the window reaches back a further `delay`: a pull + * request opened (or marked ready) inside the shifted window is due for + * reviewers now, even when nothing has touched it since. Those older pull + * requests get the blunderbuss step only. + * * @param context - the github actions event context * @param now - the current time, injectable for tests */ @@ -55,16 +61,30 @@ export async function sweep(context: Context = github.context, now: Date = new D const config = await loadProwConfig(octokit, context) const lookbackMs = resolveSweepLookback(config.sweep) const since = new Date(now.getTime() - lookbackMs) + const reviewers = blunderbussSettings(config) + const due: DueWindow = { from: new Date(since.getTime() - reviewers.delay), to: new Date(now.getTime() - reviewers.delay) } - const candidates = await recentlyUpdatedPulls(octokit, context, since) + const listed = await recentlyUpdatedPulls(octokit, context, due.from) + const candidates = listed.filter(pr => new Date(pr.updated_at) >= since) + const delayedOnly = listed.filter(pr => new Date(pr.updated_at) < since) core.info(`sweep: ${candidates.length} candidate${candidates.length === 1 ? '' : 's'} updated since ${since.toISOString()}`) + if (reviewers.delay > 0) { + core.info(`sweep: ${delayedOnly.length} more checked for delayed reviewers, updated since ${due.from.toISOString()}`) + } const result: SweepResult = { candidates: candidates.map(pr => pr.number), merged: [], enqueued: [], failures: [] } - if (candidates.length === 0) { - return result - } + const plugins = { lgtm: lgtmSettings(config), config, reviewers, since, due } - const plugins = { lgtm: lgtmSettings(config), config, since } + await forEachLimited(delayedOnly, sweepConcurrency, async (pr) => { + try { + if (await branchHasOwners(octokit, context, pr.base.ref)) { + await requestReviewersIfDue(octokit, context, pr, plugins) + } + } + catch (e) { + result.failures.push({ number: pr.number, message: `blunderbuss: ${e instanceof Error ? e.message : String(e)}` }) + } + }) await forEachLimited(candidates, sweepConcurrency, async (pr) => { const outcome = await sweepPullRequest(octokit, context, pr, plugins) @@ -86,10 +106,18 @@ export async function sweep(context: Context = github.context, now: Date = new D return result } +/** pull requests that became ready for review inside this window are due for delayed reviewers */ +interface DueWindow { + from: Date + to: Date +} + interface SweepPlugins { lgtm: ReturnType config: ProwConfig + reviewers: BlunderbussSettings since: Date + due: DueWindow } interface PullOutcome { @@ -106,7 +134,9 @@ async function sweepPullRequest(octokit: Octokit, context: Context, pr: PullsLis const tide = await loadTide(octokit, context, pr.base.ref) const ownersSteps: Step[] = [ ['owners-label', () => applyOwnersLabels(octokit, context, pr.number)], - ['blunderbuss', () => requestReviewersIfFresh(octokit, context, pr, plugins)], + ['blunderbuss', () => plugins.reviewers.delay > 0 + ? requestReviewersIfDue(octokit, context, pr, plugins) + : requestReviewersIfFresh(octokit, context, pr, plugins)], ['approve', () => evaluateApproval(octokit, context, pr.number)], ] const steps: Step[] = [ @@ -160,7 +190,63 @@ async function requestReviewersIfFresh(octokit: Octokit, context: Context, pr: P return } - await requestOwnersReviewers(octokit, context, pr.number, blunderbussSettings(plugins.config), { explicit: false, rng: Math.random }) + await requestOwnersReviewers(octokit, context, pr.number, plugins.reviewers, { explicit: false, rng: Math.random }) +} + +/** + * requestReviewersIfDue is blunderbuss with `delay`: a pull request whose + * review clock (opened, or the last `ready_for_review` when drafts are + * ignored) started inside the due window, and that still has no requested + * reviewer or team and no review, gets reviewers now. + */ +async function requestReviewersIfDue(octokit: Octokit, context: Context, pr: PullsListItem, plugins: SweepPlugins): Promise { + const { reviewers: settings, due } = plugins + if (settings.ignore_drafts && pr.draft === true) { + core.debug(`sweep: #${pr.number} is a draft`) + return + } + if ((pr.requested_reviewers ?? []).length > 0 || (pr.requested_teams ?? []).length > 0) { + core.debug(`sweep: #${pr.number} already has requested reviewers`) + return + } + + let start = new Date(pr.created_at) + if (start > due.to) { + core.debug(`sweep: #${pr.number} is not due for reviewers before ${new Date(start.getTime() + settings.delay).toISOString()}`) + return + } + if (settings.ignore_drafts) { + const ready = await lastReadyForReview(octokit, context, pr.number) + if (ready !== undefined && ready > start) { + start = ready + } + } + if (start < due.from || start > due.to) { + core.debug(`sweep: #${pr.number} became ready at ${start.toISOString()}, outside the due window`) + return + } + + const { data: reviews } = await octokit.pulls.listReviews({ ...context.repo, pull_number: pr.number, per_page: 1 }) + if (reviews.length > 0) { + core.debug(`sweep: #${pr.number} already has reviews`) + return + } + + await requestOwnersReviewers(octokit, context, pr.number, settings, { explicit: false, rng: Math.random }) +} + +async function lastReadyForReview(octokit: Octokit, context: Context, pullNumber: number): Promise { + let events: Array<{ event?: string, created_at?: string }> + try { + events = await octokit.paginate(octokit.issues.listEventsForTimeline, { ...context.repo, issue_number: pullNumber, per_page: 100 }) as Array<{ event?: string, created_at?: string }> + } + catch (e) { + throw new Error(`could not read the timeline: ${e}`) + } + const times = events + .filter(event => event.event === 'ready_for_review' && event.created_at !== undefined) + .map(event => new Date(event.created_at!).getTime()) + return times.length === 0 ? undefined : new Date(Math.max(...times)) } async function recentlyUpdatedPulls(octokit: Octokit, context: Context, since: Date): Promise { diff --git a/src/plugins/blunderbuss.ts b/src/plugins/blunderbuss.ts index 0f57ab3e..50831c3f 100644 --- a/src/plugins/blunderbuss.ts +++ b/src/plugins/blunderbuss.ts @@ -6,6 +6,7 @@ import * as core from '@actions/core' import * as github from '@actions/github' import { loadProwConfig } from '../utils/config' +import { parseDuration } from '../utils/duration' import { newOctokit } from '../utils/octokit' import { loadPullRequestOwners } from '../utils/pullRequestOwners' @@ -15,6 +16,8 @@ export interface BlunderbussSettings { exclude_approvers: boolean ignore_drafts: boolean ignore_authors: string[] + /** milliseconds to wait before requesting; 0 requests on the event itself */ + delay: number } export type Rng = () => number @@ -33,6 +36,7 @@ export function blunderbussSettings(config: ProwConfig): BlunderbussSettings { exclude_approvers: raw.exclude_approvers ?? false, ignore_drafts: raw.ignore_drafts ?? true, ignore_authors: (raw.ignore_authors ?? []).map(login => login.toLowerCase()), + delay: parseDuration(raw.delay, 'blunderbuss.delay'), } } @@ -78,6 +82,7 @@ function sample(items: string[], count: number, rng: Rng): string[] { * blunderbuss is the `pull_request` handler modelled on Prow's blunderbuss * plugin: on `opened` (and `ready_for_review` when drafts are ignored) it * requests reviews from the OWNERS reviewers covering the changed files. + * With `blunderbuss.delay` it defers to the `sweep` job instead. * * @param context - the github context of the current action event * @param rng - a source of numbers in [0, 1), injectable for tests @@ -102,6 +107,11 @@ export async function blunderbuss(context: Context = github.context, rng: Rng = return } + if (settings.delay > 0) { + core.info(`blunderbuss: #${pullNumber} ${action}; blunderbuss.delay is set, so the sweep job requests reviewers once it is due and nobody has been asked`) + return + } + await requestOwnersReviewers(octokit, context, pullNumber, settings, { explicit: false, rng }) } diff --git a/src/utils/config.ts b/src/utils/config.ts index 057b4a65..512bfc5e 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -72,6 +72,8 @@ export interface BlunderbussConfig { ignore_drafts?: boolean /** pull request authors that never get reviewers assigned */ ignore_authors?: string[] + /** wait this long after `opened` (or `ready_for_review`) and request only if nobody asked for a review meanwhile, ex: `1h`; default 0, at most 24h; the `sweep` job does the request */ + delay?: string } export interface ApproveConfig { @@ -169,6 +171,7 @@ export const defaultAuthorization: ResolvedAuthorization = { } export const defaultSweepLookback = '1h' +export const maxBlunderbussDelayMs = 24 * 3_600_000 export const maxSweepLookbackMs = 24 * 3_600_000 /** repositories of the owner that may hold an organization wide prow.yaml, in precedence order */ @@ -603,12 +606,29 @@ function normalizeBlunderbuss(source: string, raw: unknown): BlunderbussConfig { throw new Error(`${source}: blunderbuss.ignore_authors must be a list of GitHub usernames`) } + if (raw.delay !== undefined) { + if (typeof raw.delay !== 'string') { + throw new TypeError(`${source}: blunderbuss.delay must be a duration string such as 1h or 30m`) + } + let ms: number + try { + ms = parseDuration(raw.delay, 'blunderbuss.delay') + } + catch (e) { + throw new Error(`${source}: ${e instanceof Error ? e.message : e}`) + } + if (ms > maxBlunderbussDelayMs) { + throw new Error(`${source}: blunderbuss.delay must not be longer than 24h`) + } + } + return stripUndefined({ request_count: raw.request_count as number | undefined, max_request_count: raw.max_request_count as number | undefined, exclude_approvers: raw.exclude_approvers as boolean | undefined, ignore_drafts: raw.ignore_drafts as boolean | undefined, ignore_authors: raw.ignore_authors as string[] | undefined, + delay: raw.delay as string | undefined, }) }