comphead commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4495449310
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,21 @@ 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 in
+ // parallel, so the retries of commits without one overlap.
+ const pushed = (context.payload.commits || []).map((commit) => commit.id);
+ const shas = pushed.length ? pushed : [context.sha];
+ const found = await Promise.all(shas.map((sha) => mergedPulls({ github,
context, sha })));
Review Comment:
Sound, and fixed in aa555be8c. The commits are looked up ten at a time with
`Promise.allSettled`, so a failed lookup no longer drops the labels of the
other pull requests. The script labels what it found and then rethrows the
first error, so the run is red and a rerun can finish the job. A failed
`addLabels` is isolated the same way. The docs allow up to 2048 commits in the
push payload, so the bound is not only theoretical, although each of the last
40 merges to `main` was a single commit. Tests: 25 commits never have more than
10 lookups in flight, and a failed lookup or a failed label still labels the
others.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +44,21 @@ 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 in
+ // parallel, so the retries of commits without one overlap.
+ const pushed = (context.payload.commits || []).map((commit) => commit.id);
+ const shas = pushed.length ? pushed : [context.sha];
+ const found = await Promise.all(shas.map((sha) => mergedPulls({ github,
context, sha })));
Review Comment:
Covered by the same change in aa555be8c. Lookups run ten at a time, and a
rejected lookup no longer drops the labels of the others. A 25-commit stub
never has more than 10 requests in flight.
##########
.github/workflows/labeler.yml:
##########
@@ -4,36 +4,55 @@
name: Labeler
+# `pull_request_target` is disabled by ASF infra policy. This workflow runs
from
+# the default branch when `pull_request_signal.yml` starts, so it never runs
the
+# code of a pull request. See that file for details.
concurrency:
- group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
+ group: ${{ github.repository }}-${{
github.event.workflow_run.head_repository.full_name }}-${{
github.event.workflow_run.head_branch }}-${{ github.workflow }}
Review Comment:
Sound, and fixed in aa555be8c with your first suggestion.
`pull_requests_of_run.js` no longer compares `head.sha` with the run's commit,
so the run that survives a cancellation labels the current head. That is safe
because `actions/labeler` computes labels from the files of the PR as it is
now, so a delayed run for an older commit gives the same result as the newest
one. I kept the group and `cancel-in-progress` as they are, since putting the
SHA in the group would only multiply runs. A test covers a PR whose head moved
after the run started.
##########
ci/scripts/pull_requests_of_run.js:
##########
@@ -0,0 +1,25 @@
+// 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
+
+// Used by the labeler and fork_ci_status workflows, which run on
`workflow_run`.
+//
+// `workflow_run.pull_requests` is empty for pull requests from forks, so find
+// the open pull requests by head branch and commit. A commit that is no longer
+// a head matches nothing, since a newer run handles the new head. The same
+// branch can target several bases, so pass `base` to select one.
+module.exports = async ({ github, context, base }) => {
+ const run = context.payload.workflow_run;
+ // The head repository is null when the fork was deleted.
+ if (!run.head_repository) {
+ return [];
+ }
+ const { data } = await github.rest.pulls.list({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ state: 'open',
+ head: `${run.head_repository.owner.login}:${run.head_branch}`,
+ ...(base && { base })
+ });
+ return data.filter((pull) => pull.head.sha === run.head_sha);
Review Comment:
Same fix as in the labeler thread. The lookup now returns the open pull
requests of the branch as they are, so whichever signal run survives labels the
current head. `fork_ci_status.yml` still compares the commit in `notify()`. A
late run that created the check for the current head could add a second `Rust
(fork)` check to a commit that already has one, and its scheduled sweep covers
the cancelled case.
##########
.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_name == 'workflow_run' &&
format('{0}-{1}', github.event.workflow_run.head_repository.full_name,
github.event.workflow_run.head_branch) || 'update' }}
+ cancel-in-progress: ${{ github.event_name == 'workflow_run' }}
jobs:
bridge:
name: Mirror the fork run
+ if: github.event_name != 'workflow_run' || github.event.workflow_run.event
== 'pull_request'
Review Comment:
Fixed in aa555be8c. For `workflow_run` events the job `if` now also requires
`github.event.workflow_run.head_repository.fork == true`. Branches of this
repository (Dependabot among them) and deleted forks no longer start a runner,
and `notify()` still checks the base. Tests evaluate the expression for a fork,
a branch of this repository, a deleted fork, another triggering event and the
scheduled and manual runs.
--
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]
