kumarUjjawal commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4521055890
##########
.github/workflows/release_version_labeler.yaml:
##########
@@ -7,20 +7,17 @@
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:
branches:
- main
Review Comment:
Recover release labels for merges that skip push workflows. If a
squash-merge commit contains `[skip ci]`, `[ci skip]`, or another supported
skip marker, GitHub suppresses this `push` workflow. Unlike the previous
`pull_request_target: closed` trigger, the merged PR then never receives its
release-version label. Subsequent pushes only inspect their own commits, so
they cannot recover the missing label. Add a reconciliation path that does not
depend on the merge's push workflow running.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -4,6 +4,27 @@
// Used by the release_version_labeler workflow
+// A push can have many commits. Look them up this many at a time, to stay well
+// below GitHub's limit of 100 concurrent requests.
+const BATCH_SIZE = 10;
+
+// A squash merge or a merge queue commit maps to the merged pull request. The
+// pull request can lag the push by a few seconds, so retry.
+async function mergedPulls({ github, context, sha }) {
+ for (let attempt = 1; ; attempt++) {
+ const { data } = await
github.rest.repos.listPullRequestsAssociatedWithCommit({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ commit_sha: sha
+ });
+ const pulls = data.filter((pull) => pull.merged_at && pull.base.ref
=== 'main');
+ if (pulls.length > 0 || attempt === 3) {
Review Comment:
The lookup gives up about 10 s after the push (3 attempts, 5 s apart) and
then only logs, so the run stays green. The old `pull_request_target: closed`
trigger fired only after the PR was marked merged and always carried the PR
number.
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -199,13 +213,41 @@ 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(
+ (pull) => pull.head.sha ===
context.payload.workflow_run.head_sha
+ );
+ if (!pr) {
+ core.info('No open pull request to main at this commit;
nothing to mirror.');
+ return;
+ }
// Only fork PRs to main skip jobs upstream; release branches
have
// no merge queue and keep the full run.
if (!isForkPrToMain(pr)) {
core.info('Not a fork pull request to main; nothing to
mirror.');
return;
}
+ // A pull request of the same branch to a release branch, or a
+ // reopened pull request, signals a commit that can already have
+ // the check. Update that check instead of adding a second one.
+ const { data } = await github.rest.checks.listForRef({
Review Comment:
notify() checks for an existing `Rust (fork)` check before createCheck()
starts its wait for the fork run, which can last up to 60 s (12 attempts x 5
s). A check created during that wait is not seen, so the new dedup fails
exactly when it is needed.
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -199,13 +213,41 @@ 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(
+ (pull) => pull.head.sha ===
context.payload.workflow_run.head_sha
+ );
+ if (!pr) {
+ core.info('No open pull request to main at this commit;
nothing to mirror.');
+ return;
+ }
// Only fork PRs to main skip jobs upstream; release branches
have
// no merge queue and keep the full run.
if (!isForkPrToMain(pr)) {
core.info('Not a fork pull request to main; nothing to
mirror.');
return;
}
+ // A pull request of the same branch to a release branch, or a
+ // reopened pull request, signals a commit that can already have
+ // the check. Update that check instead of adding a second one.
+ const { data } = await github.rest.checks.listForRef({
Review Comment:
notify() calls checks.listForRef(check_name: CHECK) and then syncCheck(pr),
which immediately makes the same listForRef call again. That is a redundant API
request, and the existence check lives in two places.
##########
docs/source/contributor-guide/testing_infrastructure.md:
##########
@@ -10,16 +10,18 @@ This page covers checks for CI workflows, documentation,
repository maintenance,
## CI Workflow Tests
-The [Rust workflow validator tests] and [fork status bridge tests] run in CI
and in `./dev/rust_lint.sh`.
+The [Rust workflow validator tests], [fork status bridge tests] and [release
version labeler tests] run in CI and in `./dev/rust_lint.sh`.
Review Comment:
nit: should we also mention test_cancel_merge_queue_runs.cjs?
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -199,13 +213,41 @@ 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(
+ (pull) => pull.head.sha ===
context.payload.workflow_run.head_sha
+ );
+ if (!pr) {
+ core.info('No open pull request to main at this commit;
nothing to mirror.');
+ return;
+ }
// Only fork PRs to main skip jobs upstream; release branches
have
// no merge queue and keep the full run.
if (!isForkPrToMain(pr)) {
Review Comment:
In notify(), the isForkPrToMain(pr) guard is now dead code. pulls.list is
called with base 'main', and the job-level `if` already requires
head_repository.fork == true.
##########
ci/scripts/release_version_labeler.js:
##########
Review Comment:
https://github.com/apache/datafusion/blob/024e77091af10fa801f0465b9bb1b3618239a893/ci/scripts/release_version_labeler.js#L32
Creating the label is a non-atomic getLabel/createLabel pair, and the
push-triggered runs have no concurrency group. Two runs close together can both
get a 404 and both call createLabel; the loser throws 422 before labeling
anything.
Treating 422 from createLabel as success would fix it.
##########
.github/workflows/labeler.yml:
##########
@@ -4,36 +4,56 @@
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 }}
cancel-in-progress: true
on:
- pull_request_target:
- types:
- - opened
- - edited
- - synchronize
+ workflow_run:
+ workflows: ["Pull request signal"]
+ types: [requested]
+
+permissions: {}
jobs:
process:
name: Process
runs-on: ubuntu-slim
- # only run for users whose permissions allow them to update PRs
- # otherwise labeler is failing:
+ if: github.event.workflow_run.event == 'pull_request'
+ # The token of a `workflow_run` workflow can update PRs from forks.
+ # Previously `pull_request_target` was used for the same reason, see
# https://github.com/apache/datafusion/issues/3743
permissions:
contents: read
pull-requests: write
steps:
+ # This checks out the default branch, never the pull request.
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ sparse-checkout: ci/scripts
Review Comment:
Every labeler run makes an extra contents API request (and depends on the
API fallback path) for a file that the checkout already does could provide.
Adding `.github/workflows/labeler` to sparse-checkout would let the action read
the config locally.
##########
.github/workflows/release_version_labeler.yaml:
##########
Review Comment:
https://github.com/apache/datafusion/blob/024e77091af10fa801f0465b9bb1b3618239a893/.github/workflows/release_version_labeler.yaml#L26
Every push to main writes the write-scoped GITHUB_TOKEN into the workspace's
git config, where any later step or action can read it. It also clones the
whole repository when only ci/scripts is needed, which is inconsistent with the
hardening this PR applies to labeler.yml and fork_ci_status.yml.
##########
.github/workflows/release_version_labeler.yaml:
##########
Review Comment:
https://github.com/apache/datafusion/blob/024e77091af10fa801f0465b9bb1b3618239a893/.github/workflows/release_version_labeler.yaml#L28
should this also use commit SHA like others?
--
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]
