kumarUjjawal commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4476496520
##########
.github/workflows/release_version_labeler.yaml:
##########
@@ -7,20 +7,15 @@
name: Release Version Labeler
-concurrency:
- group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
- cancel-in-progress: true
-
+# `pull_request_target` is disabled by ASF infra policy, so label on the push
of
+# the merge commit to `main` and look up the merged pull request from it.
on:
- pull_request_target:
- types:
- - closed
+ push:
Review Comment:
The new `push: main` trigger has no `if: github.repository ==
'apache/datafusion'` guard, so it now runs in every fork that has Actions
enabled. Forks were affected before only when PRs were closed inside the fork
itself.
Failure scenario: The contributor guide asks fork owners to enable
workflows. A contributor who syncs their fork's main then triggers this job on
each sync. It creates a `v57.0.0` label in the fork, waits through the retries,
and calls `addLabels` on the fork for any PR numbers the lookup returns. That
spends the fork's minutes and can label the wrong fork issue or fail with a
404. `merge_queue_cleanup.yml` and `draft_inactive_prs.yml` already use this
guard.
##########
ci/scripts/test_fork_ci_status.cjs:
##########
@@ -148,13 +171,101 @@ function assertRun(write, method, expected) {
test("notification selects a newer manual run instead of a push run", async ()
=> {
const latest = run(2, "workflow_dispatch");
const writes = await execute({
- event: "pull_request_target",
+ event: "workflow_run",
runs: [latest, run(1)],
});
assert.equal(writes.length, 1);
assertRun(writes[0], "create", latest);
});
+test("notification mirrors the pull request to main when a newer one targets a
release branch", async () => {
+ const release = { ...pr, number: 2, base: { ref: "branch-55" } };
+ const latest = run(2, "workflow_dispatch");
+ const writes = await execute({
+ event: "workflow_run",
+ openPrs: [release, pr],
+ runs: [latest],
+ });
+ assert.equal(writes.length, 1);
+ assertRun(writes[0], "create", latest);
+});
+
+test("notification skips a commit that is no longer a pull request head",
async () => {
+ const writes = await execute({
+ event: "workflow_run",
+ openPrs: [{ ...pr, head: { ...pr.head, sha: "b".repeat(40) } }],
+ runs: [run(1)],
+ });
+
+ test("notification skips a run whose fork was deleted", async () => {
Review Comment:
A misplaced closing brace leaves five new tests, plus the
`findPullRequests`/`lookup` helpers, inside the body of the test "notification
skips a commit that is no longer a pull request head"; that test's own
assertion ends up at line 266, after them.
Failure scenario: The inner `test()` calls (deleted fork, three `lookup`
tests) become unawaited subtests of the stale-head test instead of top-level
tests. If the parent finishes or fails first, node:test cancels them or reports
them as failures under the wrong name. Whether they run at all depends on the
Node version's subtest handling. The stale-head check itself only runs after
these extra subtests have been registered.
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -31,18 +33,28 @@ on:
permissions:
actions: read
checks: write
+ contents: read
pull-requests: read
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
'update' }}
- cancel-in-progress: ${{ github.event_name == 'pull_request_target' }}
+ group: ${{ github.workflow }}-${{
github.event.workflow_run.head_repository.full_name && format('{0}-{1}',
github.event.workflow_run.head_repository.full_name,
github.event.workflow_run.head_branch) || 'update' }}
Review Comment:
When `workflow_run.head_repository` is null (deleted fork), the concurrency
expression falls back to the `update` group used by the scheduled sweep, while
`cancel-in-progress` is still true because the event is `workflow_run`.
A signal run, or a rerun, from a PR whose fork was deleted triggers this
workflow. It joins group `Fork CI status-update` and cancels the scheduled or
manually dispatched sweep that is running, even though its own `notify()` finds
nothing to do.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,23 @@ module.exports = async ({ github, context }) => {
}
}
- await github.rest.issues.addLabels({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: context.payload.pull_request.number,
- labels: [target_version]
- });
+ // A push can include several merged pull requests.
+ const shas = (context.payload.commits || []).map((commit) => commit.id);
+ const numbers = new Set();
+ for (const sha of shas.length ? shas : [context.sha]) {
Review Comment:
Each commit in the push is looked up one after another, and every commit
without a merged PR costs two 5-second waits.
A direct push to main with N commits that have no PR blocks the job for
about 10s × N (20 commits is about 200s of runner time) before it gives up.
Retrying only the head commit, or looking up commits in parallel, would avoid
most of that.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,23 @@ module.exports = async ({ github, context }) => {
}
}
- await github.rest.issues.addLabels({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: context.payload.pull_request.number,
- labels: [target_version]
- });
+ // A push can include several merged pull requests.
+ const shas = (context.payload.commits || []).map((commit) => commit.id);
+ const numbers = new Set();
+ for (const sha of shas.length ? shas : [context.sha]) {
+ for (const pull of await mergedPulls({ github, context, sha })) {
+ numbers.add(pull.number);
+ }
+ }
+ if (numbers.size === 0) {
+ console.log(`No merged pull request found for ${context.sha}`);
Review Comment:
The "No merged pull request found" message prints only `context.sha`,
although the script searched every commit in `payload.commits`.
A multi-commit push where none of the commits maps to a merged PR logs only
the head SHA, so whoever debugs a missing label can't tell which commits were
checked.
##########
.github/workflows/pull_request_signal.yml:
##########
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: Apache-2.0
+# SPDX-FileCopyrightText: See the NOTICE file distributed with this work for
additional information regarding copyright ownership
+# SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF) under
one or more contributor license agreements
+
+# Signal for the workflows that need a write token on a pull request.
+#
+# ASF infra policy disables `pull_request_target`
+# (https://infra.apache.org/github-actions-policy.html), and GitHub is adding a
+# default policy that blocks it. The `workflow_run` workflows below run from
the
+# default branch with a write token and never check out or run pull request
+# code:
+# - labeler.yml
+# - fork_ci_status.yml
+#
+# They trigger on the `requested` event of this workflow. GitHub sends it at
+# once for every run, even one that waits for the approval of a first-time
+# contributor. This is not documented, but it is seen in
+# apache/spark-kubernetes-operator, which uses the same pattern.
+#
+# A pull request runs its own copy of this file, so it can change or delete it.
+# That only skips the labels and the immediate `Rust (fork)` check for that
pull
+# request. The scheduled update in `fork_ci_status.yml` still creates the
check,
+# and the merge queue stays the gate.
+name: Pull request signal
+
+on:
Review Comment:
The signal uses `pull_request`, which GitHub does not run while a PR has
merge conflicts (there is no merge ref). The old `pull_request_target` triggers
ran either way.
Failure scenario: A contributor pushes to a PR that conflicts with main. No
signal run starts, so `labeler.yml` never re-syncs labels for the new files and
`fork_ci_status.yml` never creates the immediate `Rust (fork)` check. Labels
stay stale until the conflict is fixed and another push lands, and the header
comment does not mention this gap.
--
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]
