comphead commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4515585690
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -199,7 +213,22 @@ jobs:
}
async function notify() {
- const pr = context.payload.pull_request;
+ // The same branch can also target a release branch, which gets
no
+ // mirrored check, so look only at the pull request to `main`.
+ const pulls = await
require('./ci/scripts/pull_requests_of_run.js')({
+ github,
+ context,
+ base: 'main',
+ });
+ // A run for an older commit finds nothing, and the scheduled
+ // update creates the check of the new head.
+ const pr = pulls.find(
Review Comment:
Confirmed. The old `pull_request_target` run of the release-branch PR
returned early, but the new lookup goes through the branch, so it finds the
`main` PR at the same commit and creates a second check. Fixed in 6ce320a0a:
`notify()` first lists the `Rust (fork)` checks of the commit, and when one
exists it updates it through the existing `syncCheck()` instead of creating
another. That also covers a reopened PR, which got a second check in the old
flow too. A test covers a release-branch PR next to the `main` PR. It fails
both without the lookup and when the existing check is skipped instead of
updated.
##########
.github/workflows/pull_request_signal.yml:
##########
@@ -0,0 +1,39 @@
+# 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. GitHub also skips `pull_request` workflows while a pull request has
+# merge conflicts, so such a pull request gets its labels after a push that
+# resolves them. The scheduled update in `fork_ci_status.yml` creates the check
+# in both cases, and the merge queue stays the gate.
+name: Pull request signal
+
+on:
+ pull_request:
Review Comment:
Confirmed. GitHub documents that the markers skip `pull_request` workflows
when they are in the head commit, and that they do not stop
`pull_request_target`. I listed it next to merge conflicts in the header in
6ce320a0a. The fork's own `rust.yml` push run is skipped by the same marker, so
the scheduled update creates a `Rust (fork)` check that reports no fork run for
the commit, which matches what happened. The labels follow with the next push
that has no marker. I also noted that a re-run of the signal does not send
`requested`, which GitHub documents, and how to trigger it again.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +48,44 @@ 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. Look the commits up a
+ // batch at a time, so the retries of commits without one overlap. A failed
+ // lookup or label must not cost the other pull requests their label, so
+ // collect the errors and throw the first one after labeling.
+ const pushed = (context.payload.commits || []).map((commit) => commit.id);
+ const shas = pushed.length ? pushed : [context.sha];
+ const numbers = new Set();
+ const errors = [];
+ for (let i = 0; i < shas.length; i += BATCH_SIZE) {
+ const batch = shas.slice(i, i + BATCH_SIZE);
+ const settled = await Promise.allSettled(
+ batch.map((sha) => mergedPulls({ github, context, sha }))
+ );
+ for (const result of settled) {
+ if (result.status === 'fulfilled') {
+ result.value.forEach((pull) => numbers.add(pull.number));
+ } else {
+ errors.push(result.reason);
+ }
+ }
+ }
+ if (numbers.size === 0 && errors.length === 0) {
+ console.log(`No merged pull request found for ${shas.join(', ')}`);
+ }
+ for (const number of numbers) {
+ try {
+ await github.rest.issues.addLabels({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ issue_number: number,
+ labels: [target_version]
+ });
+ } catch (error) {
+ errors.push(error);
+ }
+ }
+ if (errors.length > 0) {
Review Comment:
Fixed in 6ce320a0a. Each failed lookup is logged with its commit, and each
failed label with its pull request, when it happens. A summary line follows,
and the first error is still thrown so the run is red. A test checks the log
lines for a run with one failed lookup and one failed label.
--
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]
