ganeshashree commented on code in PR #57867:
URL: https://github.com/apache/spark/pull/57867#discussion_r3746623188
##########
.github/workflows/update_build_status.yml:
##########
@@ -46,44 +46,110 @@ jobs:
// See
https://docs.github.com/en/graphql/reference/enums#mergestatestatus
const maybeReady = ['behind', 'clean', 'draft', 'has_hooks',
'unknown', 'unstable'];
+ // A fork workflow-run lookup can fail transiently (server errors,
network drops, or
+ // REST rate limiting, which GitHub returns as 403 with rate-limit
headers or 429).
+ // These should be retried on a later scheduled pass rather than
reported to the
+ // contributor as a broken fork. Any other failure (e.g. a 404 for
a missing
+ // build_main.yml) is treated as permanent.
+ //
https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
+ const isTransientError = (e) => {
+ if (!e.status) return true;
+ if (e.status >= 500 || e.status == 429) return true;
+ if (e.status == 403) {
+ const headers = (e.response && e.response.headers) || {};
+ return !!headers['retry-after'] ||
headers['x-ratelimit-remaining'] === '0'
+ || /rate limit/i.test(e.message || '');
+ }
+ return false;
+ };
+
+ // List all check-runs for a commit. per_page=100 (not the default
30) matches
+ // notify_test_workflow.yml: a SHA can accumulate more check-runs
than one page
+ // (CI matrix, external checks, duplicate Build checks from
reopened PRs), which
+ // could otherwise push the target Build check off the first page
and leave the PR
+ // stuck in 'queued' forever.
+ const listCheckRuns = (ref) => github.paginate(
+ 'GET /repos/{owner}/{repo}/commits/{ref}/check-runs',
+ {
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ ref: ref,
+ per_page: 100
+ }
+ );
+
+ // Parse a Build check's output text into the {owner, repo,
run_id} the run fetch
+ // needs, or return null if it is absent, malformed, or missing a
field. JSON.parse
+ // succeeding is not enough: a check from an older version, a
manual run, or another
+ // app can carry null, {}, or unrelated JSON that parses but lacks
these fields, and
+ // the run fetch would then fail.
+ const parseRunParams = (cr) => {
+ let params;
+ try {
+ params = JSON.parse(cr.output.text);
+ } catch (error) {
+ return null;
+ }
+ if (!params || !params.owner || !params.repo || !params.run_id) {
+ return null;
+ }
+ return params;
+ };
+
+ // A Build check this updater can actually sync: not
action_required (notify writes
+ // that when it missed the fork run, and it carries no run params)
and with output
+ // text carrying the {owner, repo, run_id} needed to fetch the
fork run. A Build check
+ // with empty/malformed/fieldless output can never be synced, so
it must not count as
+ // present - otherwise it would suppress the backfill and leave
the PR stuck with an
+ // unsyncable check.
+ const isSyncableBuildCheck = (cr) =>
+ cr.name == 'Build' && cr.conclusion != 'action_required'
+ && parseRunParams(cr) != null;
+
+ // An action_required Build check: the contributor-facing "enable
Actions / rebase"
+ // status notify writes when it found no fork run. It is not
syncable, but it already
+ // carries the guidance the no-run backfill branch would create,
so it is "useful".
+ const isActionRequiredBuildCheck = (cr) =>
+ cr.name == 'Build' && cr.conclusion == 'action_required';
+
// Iterate open PRs
for await (const prs of github.paginate.iterator(endpoint,params))
{
// Each page
for await (const pr of prs.data) {
console.log('SHA: ' + pr.head.sha)
console.log(' Mergeable status: ' + pr.mergeable_state)
if (pr.mergeable_state == null ||
maybeReady.includes(pr.mergeable_state)) {
- // Paginate with per_page=100 to match
notify_test_workflow.yml. The default
- // page size is 30, and a SHA can accumulate more check-runs
than that (CI
- // matrix, external checks, duplicate Build checks from
reopened PRs), which
- // could push the target Build check off the first page and
leave the PR
- // stuck in 'queued' forever.
- const checkRuns = await github.paginate(
- 'GET /repos/{owner}/{repo}/commits/{ref}/check-runs',
- {
- owner: context.repo.owner,
- repo: context.repo.repo,
- ref: pr.head.sha,
- per_page: 100
- }
- )
+ const checkRuns = await listCheckRuns(pr.head.sha)
- // Iterator GitHub Checks in the PR
+ // Does this SHA already carry an action_required Build
check? notify (or an
+ // earlier pass of this updater) writes one when no fork run
was found. It is
+ // not syncable, so it never suppresses the backfill below -
which means a PR
+ // that permanently lacks a fork run (Actions disabled, old
master) would
+ // otherwise re-poll on every 15-minute pass forever. When
one is already
+ // present, the backfill's re-poll is unnecessary (see
below).
+ const hasActionRequiredBuildCheck =
checkRuns.some(isActionRequiredBuildCheck)
+
+ // Track whether a syncable Build check exists (see
isSyncableBuildCheck).
+ // notify_test_workflow.yml creates one per push; if that
job never completed
+ // (e.g. cancelled while starved of an ASF runner) the check
is missing and the
+ // backfill after this loop recreates it. Sync every match
(no early break): a
+ // SHA can carry more than one Build check (reopened PRs, or
a backfill that
+ // raced notify). Branch protection evaluates the newest
check-run of a given
+ // name, so syncing only the first would leave a newer
duplicate stuck in
+ // 'queued' and block the PR.
+ let syncableBuildCheck = false
for await (const cr of checkRuns) {
if (cr.name == 'Build' && cr.conclusion !=
"action_required") {
- // text contains parameters to make request in JSON. A
Build check
- // created by something other than
notify_test_workflow.yml (an older
- // version, a manual run, or another app) may have empty
or malformed
- // output text; skip it instead of aborting the whole
scheduled run,
- // which would block updates for every PR queued behind
it.
- let params
- try {
- params = JSON.parse(cr.output.text)
- } catch (error) {
- console.error('Skipping Build check ' + cr.id + ' with
unparseable output text')
- console.error(error)
+ // Skip a check with unusable output (see
parseRunParams) instead of
+ // aborting the whole scheduled run, which would block
every PR queued
+ // behind it. Leaving syncableBuildCheck false lets the
backfill below
+ // replace it rather than stranding the PR.
+ const params = parseRunParams(cr)
+ if (!params) {
+ console.error('Skipping Build check ' + cr.id + ' with
unusable output')
continue
}
+ syncableBuildCheck = true
Review Comment:
Fixed. `syncableBuildCheck` is now set only after the run fetch succeeds. In
the catch, a transient error (5xx/rate-limit) still suppresses the backfill so
we retry next pass, while a permanent 404 leaves the flag false so
reconciliation recreates a useful status.
I also had to exclude these checks from the pre-create recheck
(`staleCheckIds`): it uses `isSyncableBuildCheck`, which only parses the output
text, so the deleted-run check would otherwise re-register as syncable and
re-suppress the backfill. Thanks!
--
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]
