comphead commented on code in PR #26683:
URL: https://github.com/apache/datafusion/pull/26683#discussion_r4470031802
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +27,29 @@ 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 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.
+ let pulls = [];
+ for (let attempt = 0; attempt < 3 && pulls.length === 0; attempt++) {
+ if (attempt > 0) {
+ await new Promise((resolve) => setTimeout(resolve, 5000));
+ }
+ const { data } = await
github.rest.repos.listPullRequestsAssociatedWithCommit({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ commit_sha: context.sha
+ });
Review Comment:
Fixed in 4c3c2e980. The script now looks up every commit in
`context.payload.commits` (or `context.sha` when the payload has none) and
labels each merged PR once. In the last 40 merges every merge commit was the
head of its own push, so this is robustness and not a current gap.
`test_release_version_labeler.cjs` covers a push with two commits that share a
PR.
##########
ci/scripts/release_version_labeler.js:
##########
@@ -27,10 +27,29 @@ 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 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.
+ let pulls = [];
+ for (let attempt = 0; attempt < 3 && pulls.length === 0; attempt++) {
+ if (attempt > 0) {
+ await new Promise((resolve) => setTimeout(resolve, 5000));
+ }
+ const { data } = await
github.rest.repos.listPullRequestsAssociatedWithCommit({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ commit_sha: context.sha
+ });
+ pulls = data.filter((pull) => pull.merged_at && pull.base.ref ===
'main');
+ }
+ if (pulls.length === 0) {
Review Comment:
Agreed. After the three attempts it now logs `No merged pull request found
for <sha>` and the run succeeds. All of the last 100 commits on `main` had a
merged PR, but a failed run on `main` is too noisy for a cosmetic label. A test
covers it.
##########
.github/workflows/pull_request_signal.yml:
##########
@@ -0,0 +1,29 @@
+# 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. They trigger on the `requested` event of this workflow, which GitHub
+# sends at once for every run, even for a first-time contributor whose run
waits
+# for approval:
+# - labeler.yml
+# - fork_ci_status.yml
+name: Pull request signal
+
Review Comment:
True, a PR runs its own copy of the file. It can only skip the labels and
the immediate `Rust (fork)` check for itself, and it gains nothing else. The
signal run has `permissions: {}`, and the privileged workflows only read the
head repository, branch and SHA of the run, which the PR author controls
anyway. The scheduled update in `fork_ci_status.yml` still creates a missing
`Rust (fork)` check, and the merge queue stays the gate. I documented this in
the header of the signal workflow in 4c3c2e980.
##########
.github/workflows/fork_ci_status.yml:
##########
@@ -198,8 +201,27 @@ jobs:
core.info(`Created ${CHECK} for ${sha}: ${payload.conclusion ||
payload.status}`);
}
+ // `workflow_run.pull_requests` is empty for pull requests from
+ // forks, so find the pull request by its head branch and commit.
+ // The same branch can also target a release branch, which gets no
+ // mirrored check, so list only the pull requests to `main`.
+ async function pullRequestOf(run) {
+ const { data } = await github.rest.pulls.list({
+ ...upstream,
+ state: 'open',
+ base: 'main',
+ head: `${run.head_repository.owner.login}:${run.head_branch}`,
+ });
+ return data.find((pr) => pr.head.sha === run.head_sha);
+ }
+
async function notify() {
- const pr = context.payload.pull_request;
+ // An outdated commit matches nothing, since a newer run mirrors
the new head.
Review Comment:
Fixed in 4c3c2e980. The shared lookup returns no pull requests when
`head_repository` is null, so neither workflow dereferences it. Tests cover the
lookup and the `fork_ci_status` notification.
##########
.github/workflows/labeler.yml:
##########
@@ -4,36 +4,60 @@
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:
+ # `workflow_run.pull_requests` is empty for pull requests from forks, so
+ # find the pull requests by their head branch and commit. The same branch
+ # can target `main` and a release branch, so label every match. An
+ # outdated commit matches nothing, since a newer run labels the new head.
+ - name: Find the pull requests
Review Comment:
Fixed in 4c3c2e980. Both workflows now call
`ci/scripts/pull_requests_of_run.js`, after a sparse checkout of `ci/scripts`
from the default branch (never the pull request). The tests in
`test_fork_ci_status.cjs` cover it directly, including the no-base case that
the labeler uses.
##########
.github/workflows/pull_request_signal.yml:
##########
@@ -0,0 +1,29 @@
+# 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. They trigger on the `requested` event of this workflow, which GitHub
+# sends at once for every run, even for a first-time contributor whose run
waits
Review Comment:
You are right that it is not documented. It is observed behavior. In
`apache/spark-kubernetes-operator` two signal runs from an outside fork stayed
`completed/action_required` (never approved), and each still started its
`workflow_run` consumer 2 to 3 seconds later. I noted this in the header of the
signal workflow in 4c3c2e980. If GitHub changes it, the scheduled update in
`fork_ci_status.yml` still creates a missing `Rust (fork)` check, but labeling
would need a follow-up.
##########
.github/workflows/release_version_labeler.yaml:
##########
@@ -11,16 +11,15 @@ concurrency:
group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{
github.workflow }}
Review Comment:
Fixed in 4c3c2e980. I removed the concurrency block. Each push needs its own
run, so there is nothing to cancel or serialize.
--
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]
