comphead commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4483714416
##########
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:
Good catch. The `});` I matched was the end of the `execute(...)` call, so
the new tests ran as subtests of the stale-head test. Fixed in f64546bbd. They
are top-level tests now (the runner output has no indented test lines), and the
stale-head test asserts on its own again.
##########
.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:
Fixed in f64546bbd. The job now has `if: github.repository ==
'apache/datafusion'`, like `merge_queue_cleanup.yml` and
`draft_inactive_prs.yml`.
##########
.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:
Right, this is a known gap. I noted it in the header of the signal workflow
in f64546bbd. A PR with conflicts gets its labels after the push that resolves
them. The scheduled update in `fork_ci_status.yml` creates the `Rust (fork)`
check regardless, because it selects PRs by their head and base and not by
mergeability. The Rust workflow has the same limit, since it also runs on
`pull_request`.
##########
.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:
Confirmed and fixed in f64546bbd. The per-branch group is now chosen by
`github.event_name == 'workflow_run'`, so a signal run from a deleted fork gets
its own group and no longer cancels the sweep. Two tests cover the groups, and
the one for the deleted fork fails with the old expression.
##########
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:
Fixed in f64546bbd. The commits are looked up in parallel with
`Promise.all`, each with its own retries, so a push whose commits have no PR
waits about 10 seconds in total and not 10 seconds per commit. A test checks
that the lookups overlap and that only the lagging commit is retried.
##########
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:
Fixed in f64546bbd. The message lists every commit that was searched. The
old test had locked in the head SHA, so it now checks a push with two commits.
--
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]
