jayzhan211 commented on code in PR #26715: URL: https://github.com/apache/datafusion/pull/26715#discussion_r4369052233
########## ci/scripts/draft_inactive_prs.cjs: ########## @@ -0,0 +1,179 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +const { setTimeout } = require("node:timers/promises"); + +const IDLE_DAYS = 14; +const QUERY = `query( + $owner: String!, $repo: String!, $number: Int!, + $checksCursor: String, $reviewsCursor: String, + $loadChecks: Boolean!, $loadReviews: Boolean! +) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + id number state isDraft updatedAt headRefOid mergeable + mergeQueueEntry { id } + timelineItems(last: 1, itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT]) { + nodes { ... on HeadRefForcePushedEvent { createdAt } } + } + latestOpinionatedReviews(first: 100, after: $reviewsCursor) @include(if: $loadReviews) { + nodes { state submittedAt commit { oid } } + pageInfo { hasNextPage endCursor } + } + commits(last: 1) @include(if: $loadChecks) { + nodes { commit { + pushedDate + statusCheckRollup { + contexts(first: 100, after: $checksCursor) { + nodes { + ... on CheckRun { status conclusion } + ... on StatusContext { state } + } + pageInfo { hasNextPage endCursor } + } + } + } } + } + } + } +}`; + +const sameVersion = (a, b) => + a.headRefOid === b.headRefOid && a.updatedAt === b.updatedAt; + +async function loadPullRequest(github, repo, number) { + const variables = { ...repo, number, loadChecks: true, loadReviews: true }; + let pr; + const checks = []; + const reviews = []; + while (variables.loadChecks || variables.loadReviews) { + const { repository } = await github.graphql(QUERY, variables); + const current = repository.pullRequest; + if (!current || (pr && !sameVersion(pr, current))) return null; + pr = { ...pr, ...current }; + if (variables.loadChecks) { + const connection = + current.commits.nodes[0]?.commit.statusCheckRollup?.contexts; + checks.push(...(connection?.nodes ?? [])); + variables.loadChecks = connection?.pageInfo.hasNextPage ?? false; + variables.checksCursor = connection?.pageInfo.endCursor; + } + if (variables.loadReviews) { + const connection = current.latestOpinionatedReviews; + reviews.push(...connection.nodes); + variables.loadReviews = connection.pageInfo.hasNextPage; + variables.reviewsCursor = connection.pageInfo.endCursor; + } + } + return { ...pr, checks, reviews }; +} + +function reasonsToDraft(pr, cutoff) { + if ( + !pr || + pr.state !== "OPEN" || + pr.isDraft || + pr.mergeQueueEntry || + !(Date.parse(pr.updatedAt) <= cutoff) + ) + return []; + + const reasons = []; + if (pr.mergeable === "CONFLICTING") reasons.push("merge conflicts"); + // The rollup reports FAILURE for cancelled checks too. + if ( + pr.checks.some( + (check) => + (check.status === "COMPLETED" && + ["FAILURE", "TIMED_OUT"].includes(check.conclusion)) || + ["FAILURE", "ERROR"].includes(check.state) + ) + ) + reasons.push("failing CI checks"); + + const forcePush = pr.timelineItems.nodes[0]?.createdAt; + const pushedDate = pr.commits.nodes[0]?.commit.pushedDate; + // pushedDate can be null; also check the reviewed commit and last force push. + if ( + pr.reviews.some( + (review) => + review.state === "CHANGES_REQUESTED" && + review.commit?.oid === pr.headRefOid && + (!forcePush || + Date.parse(review.submittedAt) > Date.parse(forcePush)) && + (!pushedDate || Date.parse(review.submittedAt) > Date.parse(pushedDate)) + ) + ) + reasons.push("changes requested on the latest push"); + return reasons; +} + +async function run({ github, context, core, dryRun = true }) { + const repo = { owner: context.repo.owner, repo: context.repo.repo }; + const cutoff = Date.now() - IDLE_DAYS * 24 * 60 * 60 * 1000; + // Collect the list before mutations change updated_at and its pagination. + const prs = await github.paginate(github.rest.pulls.list, { + ...repo, + state: "open", + sort: "updated", + direction: "asc", + per_page: 100, + }); + const results = []; + for (const listed of prs) { Review Comment: GitHub computes `mergeable` lazily, so the first query for a PR usually returns `UNKNOWN`, which this treats as "no conflict". I dry-ran the script twice against the repo: the first run found 9 conflicting PRs (27 eligible in total) and the second found 60 (69 in total). Since the cron usually runs after main has moved, most conflicts would be missed, and which PRs get drafted would depend on whether something else queried them first. Could we warm the state up with one pass and a short wait before deciding? With this change the dry run gives the same 69 PRs from a cold start: ```diff - const results = []; - for (const listed of prs) { - if (listed.draft || !(Date.parse(listed.updated_at) <= cutoff)) continue; + const candidates = prs.filter( + (listed) => !listed.draft && Date.parse(listed.updated_at) <= cutoff + ); + // GitHub computes `mergeable` lazily: the first query starts the job and + // returns UNKNOWN, so query every candidate once and wait before deciding. + for (const listed of candidates) { + await loadPullRequest(github, repo, listed.number); + } + await setTimeout(30_000); + const results = []; + for (const listed of candidates) { let pr = await loadPullRequest(github, repo, listed.number); ``` ########## ci/scripts/draft_inactive_prs.cjs: ########## @@ -0,0 +1,179 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +const { setTimeout } = require("node:timers/promises"); + +const IDLE_DAYS = 14; +const QUERY = `query( + $owner: String!, $repo: String!, $number: Int!, + $checksCursor: String, $reviewsCursor: String, + $loadChecks: Boolean!, $loadReviews: Boolean! +) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + id number state isDraft updatedAt headRefOid mergeable + mergeQueueEntry { id } + timelineItems(last: 1, itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT]) { + nodes { ... on HeadRefForcePushedEvent { createdAt } } + } + latestOpinionatedReviews(first: 100, after: $reviewsCursor) @include(if: $loadReviews) { + nodes { state submittedAt commit { oid } } + pageInfo { hasNextPage endCursor } + } + commits(last: 1) @include(if: $loadChecks) { + nodes { commit { + pushedDate + statusCheckRollup { + contexts(first: 100, after: $checksCursor) { + nodes { + ... on CheckRun { status conclusion } + ... on StatusContext { state } + } + pageInfo { hasNextPage endCursor } + } + } + } } + } + } + } +}`; + +const sameVersion = (a, b) => + a.headRefOid === b.headRefOid && a.updatedAt === b.updatedAt; + +async function loadPullRequest(github, repo, number) { + const variables = { ...repo, number, loadChecks: true, loadReviews: true }; + let pr; + const checks = []; + const reviews = []; + while (variables.loadChecks || variables.loadReviews) { + const { repository } = await github.graphql(QUERY, variables); + const current = repository.pullRequest; + if (!current || (pr && !sameVersion(pr, current))) return null; + pr = { ...pr, ...current }; + if (variables.loadChecks) { + const connection = + current.commits.nodes[0]?.commit.statusCheckRollup?.contexts; + checks.push(...(connection?.nodes ?? [])); + variables.loadChecks = connection?.pageInfo.hasNextPage ?? false; + variables.checksCursor = connection?.pageInfo.endCursor; + } + if (variables.loadReviews) { + const connection = current.latestOpinionatedReviews; + reviews.push(...connection.nodes); + variables.loadReviews = connection.pageInfo.hasNextPage; + variables.reviewsCursor = connection.pageInfo.endCursor; + } + } + return { ...pr, checks, reviews }; +} + +function reasonsToDraft(pr, cutoff) { + if ( + !pr || + pr.state !== "OPEN" || + pr.isDraft || + pr.mergeQueueEntry || + !(Date.parse(pr.updatedAt) <= cutoff) + ) + return []; + + const reasons = []; + if (pr.mergeable === "CONFLICTING") reasons.push("merge conflicts"); + // The rollup reports FAILURE for cancelled checks too. + if ( + pr.checks.some( + (check) => + (check.status === "COMPLETED" && + ["FAILURE", "TIMED_OUT"].includes(check.conclusion)) || + ["FAILURE", "ERROR"].includes(check.state) + ) + ) + reasons.push("failing CI checks"); + + const forcePush = pr.timelineItems.nodes[0]?.createdAt; Review Comment: nit: `review.commit?.oid === pr.headRefOid` already means nothing was pushed after the review, so the force-push and `pushedDate` checks aren't needed. `pushedDate` is also deprecated (its removal date was 2023-07-01) and returns null for every PR I checked, and if GitHub removes it the whole query will fail. Could we drop it together with `timelineItems` and `submittedAt`? The dry run gives the same results without them: ```diff - const forcePush = pr.timelineItems.nodes[0]?.createdAt; - const pushedDate = pr.commits.nodes[0]?.commit.pushedDate; - // pushedDate can be null; also check the reviewed commit and last force push. if ( pr.reviews.some( (review) => review.state === "CHANGES_REQUESTED" && - review.commit?.oid === pr.headRefOid && - (!forcePush || - Date.parse(review.submittedAt) > Date.parse(forcePush)) && - (!pushedDate || Date.parse(review.submittedAt) > Date.parse(pushedDate)) + review.commit?.oid === pr.headRefOid ) ) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
