kumarUjjawal commented on code in PR #26683: URL: https://github.com/apache/datafusion/pull/26683#discussion_r4463321235
########## .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: The signal workflow runs from the PR's own version of the file, so a PR that renames, edits or deletes it turns off labeling and the `Rust (fork)` check for itself without any error. ########## .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: The PR lookup by head branch plus SHA is written twice: inline in `labeler.yml` and in `pullRequestOf` in `fork_ci_status.yml`. Only the second copy has tests. ########## .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: The script dereferences `run.head_repository.owner.login` without a null check; `head_repository` is null when the fork was deleted or is unavailable. ########## 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: On push, the script looks up PRs only for `context.sha`, so every other PR merged by the same push never gets the release version label. ########## .github/workflows/fork_ci_status.yml: ########## @@ -7,18 +7,20 @@ # A pull request from a fork to `main` runs `rust.yml` on the fork. This # workflow mirrors that run as one `Rust (fork)` check on the pull request, so # reviewers see the result without opening the fork. It never checks out or -# runs code from the pull request: on `pull_request_target` it reads the fork's -# public run list and creates the check, and on a schedule it updates the -# active or missing runs of recently updated pull requests plus a rotating -# slice of all open pull requests. The check is informational. The merge +# runs code from the pull request: on `workflow_run` of +# `pull_request_signal.yml` it reads the fork's public run list and creates the +# check (`pull_request_target` is disabled by ASF infra policy), and on a +# schedule it updates the active or missing runs of recently updated pull +# requests plus a rotating slice of all open pull requests. The check is informational. The merge Review Comment: The rewritten header comment is not re-wrapped; the line "requests plus a rotating slice ... The merge" runs far past the wrap width of the lines around 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 Review Comment: The design relies on GitHub sending `workflow_run: requested` for a run that is waiting for first-time-contributor approval. That is not documented; GitHub may hold the event until a maintainer approves the run. ########## 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: The script now throws when a push to main has no merged PR, which turns ordinary direct pushes into failed workflow runs on main. ########## .github/workflows/release_version_labeler.yaml: ########## @@ -11,16 +11,15 @@ concurrency: group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} Review Comment: The concurrency group still uses `github.head_ref || github.sha`. With a `push` trigger `head_ref` is always empty and every SHA is unique, so the group and `cancel-in-progress` no longer do anything. -- 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]
