mirror of
https://github.com/NixOS/nixpkgs.git
synced 2026-07-06 17:13:24 -05:00
ci/github-script/merge: share reviews fetch with bot.js, drop events
`bot.js` already pulls reviews via GraphQL for the approval-count
labels. Move that fetch above the `handleMerge` call, add `commit
{ oid }` to the query, and pass the result through. `handleMerge` no
longer issues its own `listReviews` REST call.
While here, the `approvals` set is now derived from the same reviews
data. Two upsides:
- A reviewer who approved and was later dismissed no longer counts
towards the approval check; their review now surfaces with state
DISMISSED instead of leaving a stale `reviewed` event behind.
- `events` is no longer needed by `runChecklist` (it was only feeding
the approvals filter); the parameter is dropped from both call
sites. `handleMerge` still uses `events` for tracking the latest
push and merge-command comments.
Also drop the redundant `user` truthy checks (and the stale "some
users have been deleted" comment) in `runChecklist`. The GraphQL
query uses `author { ... on User { login id } }` and bot.js then
filters via `r.user?.login`, so by the time reviews reach
`runChecklist` every entry already has a populated `user`. Verified
by querying real PRs: bots surface as `{__typename: "Bot"}` (no
`login`/`id`, filtered out by bot.js), and deleted accounts surface
as the "ghost" user (login `"ghost"`, id `10137`), which passes the
filter but matches no committer - harmless for both checks.
Assisted-by: claude-code with claude-opus-4-8[1m]-high
(cherry picked from commit 0c1c3d4813)
This commit is contained in:
parent
e934e8a364
commit
926237f642
2 changed files with 22 additions and 34 deletions
|
|
@ -206,20 +206,8 @@ module.exports = async ({ github, context, core, dry }) => {
|
|||
|
||||
const maintainers = await getMaintainerMap(pull_request.base.ref)
|
||||
|
||||
const merge_bot_eligible = await handleMerge({
|
||||
github,
|
||||
context,
|
||||
core,
|
||||
log,
|
||||
dry,
|
||||
pull_request,
|
||||
events,
|
||||
maintainers,
|
||||
getTeamMembers,
|
||||
getUser,
|
||||
})
|
||||
|
||||
// Check for any human reviews other than the PR author, GitHub actions and other GitHub apps.
|
||||
// `commit { oid }` is needed by handleMerge to verify approvals are against the current head.
|
||||
const reviews = (
|
||||
await github.graphql(
|
||||
`query($owner: String!, $repo: String!, $pr: Int!) {
|
||||
|
|
@ -231,6 +219,7 @@ module.exports = async ({ github, context, core, dry }) => {
|
|||
reviews(first: 100) {
|
||||
nodes {
|
||||
state
|
||||
commit { oid }
|
||||
user: author {
|
||||
# Only get users, no bots
|
||||
... on User {
|
||||
|
|
@ -266,6 +255,20 @@ module.exports = async ({ github, context, core, dry }) => {
|
|||
r.user.id !== pull_request.user?.id,
|
||||
)
|
||||
|
||||
const merge_bot_eligible = await handleMerge({
|
||||
github,
|
||||
context,
|
||||
core,
|
||||
log,
|
||||
dry,
|
||||
pull_request,
|
||||
events,
|
||||
reviews,
|
||||
maintainers,
|
||||
getTeamMembers,
|
||||
getUser,
|
||||
})
|
||||
|
||||
const approvals = new Set(
|
||||
reviews
|
||||
.filter((review) => review.state === 'APPROVED')
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ const { classify } = require('../supportedBranches.js')
|
|||
|
||||
function runChecklist({
|
||||
committers,
|
||||
events,
|
||||
files,
|
||||
pull_request,
|
||||
log,
|
||||
|
|
@ -28,18 +27,15 @@ function runChecklist({
|
|||
.reduce((acc, cur) => acc?.intersection(cur) ?? cur)
|
||||
|
||||
const approvals = new Set(
|
||||
events
|
||||
reviews
|
||||
.filter(
|
||||
({ event, state, commit_id }) =>
|
||||
event === 'reviewed' &&
|
||||
state === 'approved' &&
|
||||
({ state, commit }) =>
|
||||
state === 'APPROVED' &&
|
||||
// Only approvals for the current head SHA count, otherwise authors could push
|
||||
// bad code between the approval and the merge.
|
||||
commit_id === pull_request.head.sha,
|
||||
commit?.oid === pull_request.head.sha,
|
||||
)
|
||||
.map(({ user }) => user?.id)
|
||||
// Some users have been deleted, so filter these out.
|
||||
.filter(Boolean),
|
||||
.map(({ user }) => user.id),
|
||||
)
|
||||
|
||||
// A "changes requested" review from a committer blocks both the merge queue and
|
||||
|
|
@ -52,7 +48,6 @@ function runChecklist({
|
|||
const committerReviewState = new Map()
|
||||
for (const { user, state } of reviews) {
|
||||
if (
|
||||
user &&
|
||||
committers.has(user.id) &&
|
||||
['APPROVED', 'CHANGES_REQUESTED'].includes(state)
|
||||
) {
|
||||
|
|
@ -150,6 +145,7 @@ async function handleMerge({
|
|||
dry,
|
||||
pull_request,
|
||||
events,
|
||||
reviews,
|
||||
maintainers,
|
||||
getTeamMembers,
|
||||
getUser,
|
||||
|
|
@ -183,15 +179,6 @@ async function handleMerge({
|
|||
})
|
||||
).data.find(({ context }) => context === 'no PR failures')?.state
|
||||
|
||||
// Reviews are returned in chronological order; the latest review per user wins below.
|
||||
// Dismissed reviews surface with state DISMISSED and comment-only follow-ups as
|
||||
// COMMENTED, so both naturally drop out of the committer-blocking check.
|
||||
const reviews = await github.paginate(github.rest.pulls.listReviews, {
|
||||
...context.repo,
|
||||
pull_number,
|
||||
per_page: 100,
|
||||
})
|
||||
|
||||
// Only look through comments *after* the latest (force) push.
|
||||
const lastPush = events.findLastIndex(
|
||||
({ event, sha, commit_id }) =>
|
||||
|
|
@ -339,7 +326,6 @@ async function handleMerge({
|
|||
|
||||
const { result, eligible, checklist } = runChecklist({
|
||||
committers,
|
||||
events,
|
||||
files,
|
||||
pull_request,
|
||||
log,
|
||||
|
|
@ -411,7 +397,6 @@ async function handleMerge({
|
|||
|
||||
const { result } = runChecklist({
|
||||
committers,
|
||||
events,
|
||||
files,
|
||||
pull_request,
|
||||
log,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue