Repository navigation
ci: auto-approve held fork runs after a Kodiak branch update #3584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| 'use strict'; | ||
|
|
||
| // Run with: node --test .github/scripts/__tests__/kodiak-run-approval.test.js | ||
|
|
||
| const { describe, it } = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
|
|
||
| const { | ||
| decideApproval, | ||
| latestReviewStates, | ||
| } = require('../kodiak-run-approval'); | ||
|
|
||
| const BEFORE = 'b'.repeat(40); | ||
| const AFTER = 'a'.repeat(40); | ||
| const MAIN_SHA = 'c'.repeat(40); | ||
|
|
||
| function run(conclusion, actor) { | ||
| return { conclusion, triggering_actor: { login: actor } }; | ||
| } | ||
|
|
||
| function makeInput(overrides = {}) { | ||
| return { | ||
| sender: 'kodiakhq[bot]', | ||
| pr: { | ||
| head: { sha: AFTER, repo: { full_name: 'contributor/hyperdx' } }, | ||
| base: { repo: { full_name: 'hyperdxio/hyperdx' } }, | ||
| labels: [{ name: 'external' }, { name: 'automerge' }], | ||
| }, | ||
| before: BEFORE, | ||
| after: AFTER, | ||
| headCommit: { sha: AFTER, parents: [{ sha: BEFORE }, { sha: MAIN_SHA }] }, | ||
| mergeParentOnMain: true, | ||
| reviews: [{ user: { login: 'maintainer' }, state: 'APPROVED' }], | ||
| permissions: { maintainer: 'write' }, | ||
| beforeRuns: [ | ||
| run('success', 'maintainer'), | ||
| run('failure', 'github-actions[bot]'), | ||
| ], | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| describe('decideApproval', () => { | ||
| it('approves a Kodiak merge onto an approved head', () => { | ||
| assert.equal(decideApproval(makeInput()).approve, true); | ||
| }); | ||
|
|
||
| const rejected = { | ||
| 'push by someone other than Kodiak': { sender: 'contributor' }, | ||
| 'PR from the same repo': { | ||
| pr: { | ||
| ...makeInput().pr, | ||
| head: { sha: AFTER, repo: { full_name: 'hyperdxio/hyperdx' } }, | ||
| }, | ||
| }, | ||
| 'missing automerge label': { | ||
| pr: { ...makeInput().pr, labels: [{ name: 'external' }] }, | ||
| }, | ||
| 'PR head already moved on': { | ||
| pr: { | ||
| ...makeInput().pr, | ||
| head: { | ||
| sha: 'd'.repeat(40), | ||
| repo: { full_name: 'contributor/hyperdx' }, | ||
| }, | ||
| }, | ||
| }, | ||
| 'non-merge head': { | ||
| headCommit: { sha: AFTER, parents: [{ sha: BEFORE }] }, | ||
| }, | ||
| 'first parent is not the previous head': { | ||
| headCommit: { | ||
| sha: AFTER, | ||
| parents: [{ sha: 'e'.repeat(40) }, { sha: MAIN_SHA }], | ||
| }, | ||
| }, | ||
| 'merged-in parent not on main': { mergeParentOnMain: false }, | ||
| 'no reviews': { reviews: [] }, | ||
| 'approval from someone without write access': { | ||
| permissions: { maintainer: 'read' }, | ||
| }, | ||
| 'outstanding change request from a writer': { | ||
| reviews: [ | ||
| { user: { login: 'maintainer' }, state: 'APPROVED' }, | ||
| { user: { login: 'other' }, state: 'CHANGES_REQUESTED' }, | ||
| ], | ||
| permissions: { maintainer: 'write', other: 'admin' }, | ||
| }, | ||
| 'approval later dismissed': { | ||
| reviews: [ | ||
| { user: { login: 'maintainer' }, state: 'APPROVED' }, | ||
| { user: { login: 'maintainer' }, state: 'DISMISSED' }, | ||
| ], | ||
| }, | ||
| 'no runs for the previous head': { beforeRuns: [] }, | ||
| 'previous head runs still held': { | ||
| beforeRuns: [ | ||
| run('success', 'maintainer'), | ||
| run('action_required', 'contributor'), | ||
| ], | ||
| }, | ||
| 'previous head runs expired without approval': { | ||
| beforeRuns: [run('failure', 'contributor')], | ||
| }, | ||
| 'previous head runs cancelled without approval': { | ||
| beforeRuns: [run('cancelled', 'contributor')], | ||
| }, | ||
| 'previous head runs started by Kodiak, not approved': { | ||
| beforeRuns: [run('failure', 'kodiakhq[bot]')], | ||
| }, | ||
| 'previous head runs started by someone with read access': { | ||
| beforeRuns: [run('success', 'maintainer'), run('success', 'reader')], | ||
| permissions: { maintainer: 'write', reader: 'read' }, | ||
| }, | ||
| 'previous head run with no triggering actor': { | ||
| beforeRuns: [{ conclusion: 'success' }], | ||
| }, | ||
| }; | ||
|
|
||
| for (const [name, overrides] of Object.entries(rejected)) { | ||
| it(`rejects: ${name}`, () => { | ||
| const decision = decideApproval(makeInput(overrides)); | ||
| assert.equal(decision.approve, false); | ||
| assert.ok(decision.reason.length > 0); | ||
| }); | ||
| } | ||
|
|
||
| it('ignores change requests from people without write access', () => { | ||
| const decision = decideApproval( | ||
| makeInput({ | ||
| reviews: [ | ||
| { user: { login: 'maintainer' }, state: 'APPROVED' }, | ||
| { user: { login: 'drive-by' }, state: 'CHANGES_REQUESTED' }, | ||
| ], | ||
| permissions: { maintainer: 'write', 'drive-by': 'none' }, | ||
| }), | ||
| ); | ||
| assert.equal(decision.approve, true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('latestReviewStates', () => { | ||
| it('keeps an approval when a later review is only a comment', () => { | ||
| const states = latestReviewStates([ | ||
| { user: { login: 'm' }, state: 'APPROVED' }, | ||
| { user: { login: 'm' }, state: 'COMMENTED' }, | ||
| ]); | ||
| assert.equal(states.get('m'), 'APPROVED'); | ||
| }); | ||
|
|
||
| it('uses the latest decisive review', () => { | ||
| const states = latestReviewStates([ | ||
| { user: { login: 'm' }, state: 'CHANGES_REQUESTED' }, | ||
| { user: { login: 'm' }, state: 'APPROVED' }, | ||
| ]); | ||
| assert.equal(states.get('m'), 'APPROVED'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| 'use strict'; | ||
|
|
||
| // Entry point for actions/github-script. Approves the `pull_request` runs that | ||
| // GitHub holds for first-time fork contributors after Kodiak merges `main` | ||
| // into an already-approved PR. Decision logic lives in kodiak-run-approval.js. | ||
|
|
||
| const { decideApproval } = require('./kodiak-run-approval'); | ||
|
|
||
| // Held runs are created alongside this pull_request_target run, so they may | ||
| // not be listable yet when it starts. | ||
| const POLL_ATTEMPTS = 9; | ||
| const POLL_INTERVAL_MS = 10_000; | ||
| // Once something has been approved, stop after this many polls find nothing new. | ||
| const QUIET_POLLS_TO_STOP = 2; | ||
|
|
||
| const sleep = ms => new Promise(resolve => setTimeout(resolve, ms)); | ||
|
|
||
| module.exports = async ({ github, context, core }) => { | ||
| const { owner, repo } = context.repo; | ||
| const { pull_request: eventPr, before, after, sender } = context.payload; | ||
|
|
||
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner, | ||
| repo, | ||
| pull_number: eventPr.number, | ||
| }); | ||
|
|
||
| // head_sha matches runs from any PR with that commit, including a second PR | ||
| // the contributor opens from another branch. `pull_requests` is empty for | ||
| // fork runs, so scope by head repo and branch instead. | ||
| const listRuns = async head_sha => | ||
| ( | ||
| await github.paginate(github.rest.actions.listWorkflowRunsForRepo, { | ||
| owner, | ||
| repo, | ||
| head_sha, | ||
| event: 'pull_request', | ||
| per_page: 100, | ||
| }) | ||
| ).filter( | ||
| r => | ||
| r.head_repository?.full_name === pr.head.repo.full_name && | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — A null
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| r.head_branch === pr.head.ref, | ||
| ); | ||
| const { data: headCommit } = await github.rest.repos.getCommit({ | ||
| owner, | ||
| repo, | ||
| ref: after, | ||
| }); | ||
|
|
||
| let mergeParentOnMain = false; | ||
| const mergedIn = headCommit.parents[1]?.sha; | ||
| if (mergedIn) { | ||
| const { data: cmp } = await github.rest.repos.compareCommitsWithBasehead({ | ||
| owner, | ||
| repo, | ||
| basehead: `${mergedIn}...${pr.base.ref}`, | ||
| per_page: 1, | ||
| }); | ||
| mergeParentOnMain = cmp.status === 'identical' || cmp.status === 'ahead'; | ||
| } | ||
|
|
||
| const reviews = await github.paginate(github.rest.pulls.listReviews, { | ||
| owner, | ||
| repo, | ||
| pull_number: pr.number, | ||
| per_page: 100, | ||
| }); | ||
| const beforeRuns = await listRuns(before); | ||
| const permissions = {}; | ||
| for (const login of new Set( | ||
| [ | ||
| ...reviews.map(r => r.user?.login), | ||
| ...beforeRuns.map(r => r.triggering_actor?.login), | ||
| ].filter(Boolean), | ||
| )) { | ||
| try { | ||
| const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ | ||
| owner, | ||
| repo, | ||
| username: login, | ||
| }); | ||
| permissions[login] = data.permission; | ||
| } catch (err) { | ||
| if (err.status !== 404) throw err; | ||
| permissions[login] = 'none'; | ||
| } | ||
| } | ||
|
|
||
| const decision = decideApproval({ | ||
| sender: sender.login, | ||
| pr, | ||
| before, | ||
| after, | ||
| headCommit, | ||
| mergeParentOnMain, | ||
| reviews, | ||
| permissions, | ||
| beforeRuns, | ||
| }); | ||
| core.info(decision.reason); | ||
| if (!decision.approve) return; | ||
|
|
||
| const approved = new Set(); | ||
| let quietPolls = 0; | ||
| for (let attempt = 1; attempt <= POLL_ATTEMPTS; attempt++) { | ||
| const held = (await listRuns(after)).filter( | ||
| r => r.conclusion === 'action_required' && !approved.has(r.id), | ||
| ); | ||
| for (const run of held) { | ||
| await github.rest.actions.approveWorkflowRun({ | ||
| owner, | ||
| repo, | ||
| run_id: run.id, | ||
| }); | ||
| approved.add(run.id); | ||
| // run.name comes from the PR's workflow files, so don't log it. | ||
| core.info(`Approved run ${run.id} (${run.html_url})`); | ||
| } | ||
| if ( | ||
| held.length === 0 && | ||
| approved.size > 0 && | ||
| ++quietPolls >= QUIET_POLLS_TO_STOP | ||
| ) | ||
| break; | ||
| if (held.length > 0) quietPolls = 0; | ||
| if (attempt < POLL_ATTEMPTS) await sleep(POLL_INTERVAL_MS); | ||
| } | ||
|
|
||
| if (approved.size === 0) core.info(`No held runs found for ${after}`); | ||
| await core.summary | ||
| .addRaw( | ||
| `Approved ${approved.size} held run(s) for #${pr.number} at ${after}: ${decision.reason}.`, | ||
| ) | ||
| .write(); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — No test checks the
action_requiredrule on its own: deleting it fromdecideApprovalstill passes every testThe 'previous head runs still held' case pairs
action_requiredwithcontributor, who has no write permission, so the actor check rejects it anyway. Change it torun('action_required', 'maintainer')so ther.conclusion === 'action_required'condition in kodiak-run-approval.js:92 is actually tested, and assert ondecision.reasonin the rejected cases so each one proves which check fired.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond