comphead commented on code in PR #7311: URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4417470912
########## dev/ci/fork-push-changed-files.sh: ########## @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Prints the files a push to a branch of a contributor's fork changed, as the +# pull request from that branch would show them: relative to upstream main. +# +# There is no pull request to diff against on a push, and the fork's own default +# branch is the wrong base. A fork that has not synced is behind upstream, so a +# branch based on newer upstream code that reverts one of its files matches the +# fork's old copy, and a diff against it omits the file. Fetching upstream main +# gives the diff the pull request will have. The fetch drops the credentials +# actions/checkout leaves in the git config: they belong to the fork, and the +# upstream repository is public. +# +# Prints every tracked file when there is nothing to go on: the fetch fails, there +# is no merge base, or the diff is empty. An empty diff means the branch adds +# nothing to upstream main, such as `git push fork main:my-fix` before upstream +# has the commits, and selecting no suites would report a green run for a change +# nobody built. Review Comment: The `main:my-fix` example does not produce an empty diff. When the fork's `main` has commits upstream lacks, `"$ref"..."$head"` lists their files, which `test_commits_made_on_the_fork_main_are_kept_when_pushed_to_a_branch` asserts. With a fetch and a merge base, an empty diff means the branch adds nothing to upstream `main`, such as a branch pushed before its first commit or one whose commits already landed, and listing every file then runs the whole PR tier to test no change. Could the empty-diff case print nothing, keeping the full listing for a failed fetch or a missing merge base? The README line that repeats the example would change with it. ########## dev/ci/compute-changes.py: ########## @@ -660,7 +708,7 @@ def event_allows(job, event): if gates: if not any(label in event.get("labels", []) for label in gates): return False - elif "pr" not in tiers: + elif "pr" not in tiers or is_fork_pull_request(event): Review Comment: For a fork PR carrying `run-all-spark-profiles`, `opened` and `synchronize` used to take the "with the label -> profiles: all" row. Now this turns off `build_linux` and `build_linux_full`, only the label-gated `build_linux_all_profiles` stays on, and the `profiles:` expression in `ci.yml` picks `nightly`, which skips the lints and the 4.1 row. With a fork run the coverage adds up to the same, but without one the label no longer brings those forward, so "The `run-*` label suites and the `labeled` run are unchanged" in the description is not quite accurate for this label. Could the profile table in `ci.yml` gain a fork row, and `POLICY_CASES` pin `fork_pr(labels=["run-all-spark-profiles"])` on `synchronize`? ########## .github/workflows/fork_ci_status.yml: ########## @@ -0,0 +1,260 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Mirror a contributor's fork run (ci_fork.yml) onto its pull request as the informational `CI (fork)` check, +# after upstream PR CI completes and through scheduled polling for late runs and +# reruns. The check is feedback only: `Required Checks` does not depend on it, +# because a contributor controls the workflow that produced it, and the merge +# queue runs the queue tier on the merge result. +# +# Reads fork runs and writes upstream checks using reviewed default-branch code. +# No checkout, artifacts, or code from the pull request are used here. +# +# Adapted from apache/arrow-rs#11788 and apache/datafusion#25591. +# Test with Node.js 24: +# node --test dev/ci/test-fork-ci-status.cjs +# Tests mock GitHub APIs. End-to-end testing needs a fork pull request after this +# workflow is installed on the default branch. +name: Fork CI status + +on: + workflow_run: + workflows: [Comet CI] + types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + actions: read + checks: write + pull-requests: read Review Comment: ASF's GitHub Actions policy, in the section on replacing `pull_request_target` with `workflow_run`, says a workflow holding a write token MUST "set `permissions: {}` at the top level and grant each job only the scopes it needs". Could the top level be `permissions: {}`, with these scopes on the `bridge` job? Is `actions: read` needed at all? Every `github.rest.actions.*` call in the script targets the fork, and the token only grants scopes on this repository. I have not run the bridge without it. The `check-ci-config.py` guard for this workflow rejects only `contents`, `pull-requests` and `issues` at `write`, so pinning `{}` plus the job's exact scopes there would also be tighter. ########## .github/workflows/fork_ci_status.yml: ########## @@ -0,0 +1,260 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Mirror a contributor's fork run (ci_fork.yml) onto its pull request as the informational `CI (fork)` check, +# after upstream PR CI completes and through scheduled polling for late runs and +# reruns. The check is feedback only: `Required Checks` does not depend on it, +# because a contributor controls the workflow that produced it, and the merge +# queue runs the queue tier on the merge result. +# +# Reads fork runs and writes upstream checks using reviewed default-branch code. +# No checkout, artifacts, or code from the pull request are used here. +# +# Adapted from apache/arrow-rs#11788 and apache/datafusion#25591. +# Test with Node.js 24: +# node --test dev/ci/test-fork-ci-status.cjs +# Tests mock GitHub APIs. End-to-end testing needs a fork pull request after this +# workflow is installed on the default branch. +name: Fork CI status + +on: + workflow_run: + workflows: [Comet CI] + types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + actions: read + checks: write + pull-requests: read + +# Like DataFusion, group notifications separately from scheduled/manual sweeps. +# workflow_run identifies the fork and branch rather than the PR number. +concurrency: + group: ${{ github.workflow }}-${{ github.event.workflow_run.head_repository.full_name || 'update' }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: ${{ github.event_name == 'workflow_run' }} + +jobs: + bridge: + # A workflow_run for a pull request from a branch of this repository has no + # fork run to mirror, so it never starts a job. + if: >- + github.repository == 'apache/datafusion-comet' + && (github.event_name != 'workflow_run' + || (github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.head_repository.full_name != github.repository)) + name: Mirror the fork run + runs-on: ubuntu-slim + timeout-minutes: 10 + steps: + - name: Create or update the check + uses: actions/github-script@v9 + with: + script: | + // Adapted from apache/datafusion#25591. Fork results are informational: + // contributors control their workflows; only upstream merge-queue CI gates merging. + const CHECK = 'CI (fork)'; + const WORKFLOW = 'ci_fork.yml'; + const upstream = { owner: context.repo.owner, repo: context.repo.repo }; + const repository = upstream.owner + '/' + upstream.repo; + const GUIDE = 'https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/ci.md#pull-requests-from-forks'; + const SWEEP_SIZE = 25; + const listing = { ...upstream, state: 'open', base: 'main', sort: 'updated', direction: 'desc' }; + const isForkPr = (pr) => pr.state === 'open' && pr.head.repo && + pr.head.repo.full_name.toLowerCase() !== repository.toLowerCase() && pr.base.ref === 'main'; + const externalId = (pr) => 'comet-fork-ci:' + pr.number; + const conclusions = new Set([ + 'action_required', 'cancelled', 'failure', 'neutral', 'skipped', 'success', 'timed_out', + ]); + const UPDATE_NOTE = + 'This check is informational: the contributor controls the fork workflow. ' + + 'The upstream merge queue runs the required CI checks on the proposed merge result. ' + + 'Active or missing runs on recently updated PRs are polled every 15 minutes; ' + + 'a rotating sweep also discovers reruns on older PRs. Scheduled updates may be delayed.'; + + // An absent workflow/run is expected on a fork with Actions disabled. Other + // API errors must fail the bridge rather than falsely reporting missing CI. + async function optional(request) { + try { + return (await request()).data; + } catch (error) { + if (error.status === 404) return null; + throw error; + } + } + + async function syncCheck(pr, includeCompleted = true) { + if (!isForkPr(pr)) return; + const fork = { owner: pr.head.repo.owner.login, repo: pr.head.repo.name }; + const sha = pr.head.sha; + const checks = await github.paginate(github.rest.checks.listForRef, { + ...upstream, ref: sha, check_name: CHECK, per_page: 100, + }); + const check = checks.find((item) => + item.app?.slug === 'github-actions' && item.external_id === externalId(pr)); + let previous = {}; + try { previous = JSON.parse(check?.output?.text || '{}'); } catch {} + // A missing run must keep being polled, even though its check is completed. + if (!includeCompleted && check?.status === 'completed' && previous.run_id) return; + + // A fork's default branch is never tested: ci_fork.yml skips it, and a + // push there is a sync, which runs nothing. Looking for a run would + // find none, or worse, one that tested nothing and passed. + const fromMain = pr.head.ref === pr.head.repo.default_branch; + const data = fromMain ? null : await optional(() => github.rest.actions.listWorkflowRuns({ + ...fork, workflow_id: WORKFLOW, head_sha: sha, branch: pr.head.ref, per_page: 100, + })); + // Ignore PR merge-ref runs. The fork workflow on push/manual events + // tests the branch head; contributors still control its code. + const run = (data?.workflow_runs || []).filter((item) => + item.head_sha === sha && item.head_branch === pr.head.ref && + ['push', 'workflow_dispatch'].includes(item.event) Review Comment: `ci_fork.yml` triggers only on `push`, and `check-ci-config.py` fails if it gains a `workflow_dispatch` trigger, so the `workflow_dispatch` branch here cannot match. The two `test-fork-ci-status.cjs` cases about manual runs cover a case that cannot happen. Could the filter be `item.event === 'push'`, with the comment and those tests updated? -- 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]
