From f2e5fa60c8d943af8daa9384a1db1baa7a9d8015 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Wed, 28 Oct 2026 14:07:37 -0600 Subject: [PATCH 01/12] ci: run the pull request tier for fork pull requests on the fork Nearly every pull request comes from a fork, and the pull request tier plus label runs were about 60% of the runner-hours ci.yml and ci_label.yml spent in the week to 2026-10-28. For a pull request from a fork into main, the tier now runs on the contributor's fork. The merge queue stays on ASF runners and still runs every suite on the merge result. - compute-changes.py: a push to a fork gets the `pr` tier, and a pull request from a fork into main loses it. Every `run-*` label suite and the `labeled` run are unchanged, as are pull requests from a branch of this repository, pull requests to a release branch, and pull requests opened inside a fork. - ci_fork.yml: calls ci.yml on a push to any fork branch except main. It is a separate workflow so a push to a branch of this repository cannot publish a skipped `Required Checks` next to a pull request's real one. Upstream its only job is skipped. - ci.yml: a fork push diffs against the fork's default branch, and its concurrency group is keyed on the branch so the next push cancels the run. - fork_ci_status.yml: mirrors the fork's run onto the pull request as an informational `CI (fork)` check, adapted from apache/arrow-rs#11788 and apache/datafusion#25591. It never checks out pull request code. The fork result is advisory because a contributor controls the workflow. - check-ci-config.py: policy cases for fork pushes and fork pull requests, the new environment wiring, and a check that the caller, the bridge and ci.yml's push trigger agree. - docs: contributor guide and workflows README. --- .github/workflows/README.md | 57 +++- .github/workflows/ci.yml | 29 +- .github/workflows/ci_fork.yml | 71 ++++ .github/workflows/fork_ci_status.yml | 238 +++++++++++++ dev/ci/check-ci-config.py | 237 ++++++++++++- dev/ci/compute-changes.py | 49 ++- dev/ci/test-fork-ci-status.cjs | 338 +++++++++++++++++++ docs/source/contributor-guide/ci.md | 56 ++- docs/source/contributor-guide/development.md | 4 + 9 files changed, 1053 insertions(+), 26 deletions(-) create mode 100644 .github/workflows/ci_fork.yml create mode 100644 .github/workflows/fork_ci_status.yml create mode 100644 dev/ci/test-fork-ci-status.cjs diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 1976ca2632a..ba3512b76b9 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -16,7 +16,9 @@ ruleset in `.asf.yaml`. That splits CI into three tiers: - **PR tier** (`pr`): fast feedback while a change is being iterated on. The Linux build, with the Comet test suites run against the default Spark - profile (4.1) only, and nothing else. + profile (4.1) only, and nothing else. For a pull request from a fork this + tier runs on the contributor's fork instead of here; see + [Fork runs](#fork-runs). - **Queue tier** (`queue`): the authoritative gate. The PR tier plus the macOS build, the benchmark compile check, the Delta contrib build gate, the PyArrow UDF suite, Spark SQL on Spark 4.1 and Iceberg 1.11, evaluated @@ -198,6 +200,57 @@ still needs its label, so a `labeled` run there adds `spark_3_4` or nothing. `check-ci-config.py` pins that wiring, because a dropped variable reads as an empty string and would route the pull request as if it targeted `main`. +## Fork runs + +Nearly every pull request comes from a fork, and the PR tier is the +largest share of the runner-hours CI spends (about 60% in the week to +2026-10-28, label runs included). A fork pull request therefore does not run +the PR tier on ASF's runners. The contributor's fork runs it, and the merge +queue, which stays on ASF, runs everything on the merge result before anything +lands. The design follows `apache/datafusion` (#25591) and `apache/arrow-rs` +(#11788). Three files carry it: + +| File | Role | +| -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `ci_fork.yml` | Fires on a push to any branch of a fork except `main` and calls `ci.yml`. Upstream its only job is skipped. | +| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | +| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | + +Things that follow from that, and are easy to break: + +- **The fork result is advisory.** `Required Checks` does not depend on + `CI (fork)`, because a contributor controls the workflow that produced it. A + green fork run that never ran the tests costs one queue slot, not a bad merge. +- **The fork's push to `main` uses `ci.yml` directly,** since `ci.yml` already + triggers on push to `main`, and `ci_fork.yml` ignores `main` so the two never + both fire. That run is the PR tier for whatever the push changed, which also + writes the fork's cache entries. A branch restores caches saved on its own + ref or on the fork's default branch only, so a fork whose `main` has never + run starts every branch cold (about 27 minutes for the native build alone). + The bridge reads `ci.yml` for a pull request opened from a fork's `main`, and + `ci_fork.yml` for every other branch. +- **`ci.yml` must not trigger on every push.** A skipped job still publishes a + check run under its own name, so a push to a branch of this repository that + lands on a pull request's commit would put a skipped `Required Checks` next to + the real one. `ci_fork.yml` is a separate workflow so the only name that can + be skipped is its own, and `check-ci-config.py` pins its trigger and the + `github.repository != 'apache/datafusion-comet'` guard. +- **A fork push diffs against the fork's default branch.** There is no pull + request to diff against, and a fork does not know what upstream `main` is, so + `Detect changes` uses `git diff origin/...`. A fork that + has not synced diffs against an older `main`, which selects more suites, never + fewer. +- **Exceptions stay upstream:** a pull request from a branch of this + repository (a committer's, dependabot's), one that targets a release branch, + and the opt-in `run-*` suites. A fork cannot see the labels on its pull + request, and a release branch has no queue to catch what a fork run misses. +- **The bridge never checks out or runs anything from the pull request.** It + runs with `checks: write` from the default branch and reads the fork's public + run list. Its logic is tested by `dev/ci/test-fork-ci-status.cjs`, which + Preflight runs, against mocked GitHub APIs. `workflow_run` and `schedule` + events use the copy on the default branch, so a change to the bridge cannot be + exercised by the pull request that makes it. + ### Label events `ci_label.yml` fires on `pull_request.types: [labeled]` and calls `ci.yml` @@ -300,6 +353,8 @@ umbrella doesn't watch, or operate independently of the rest of CI: | `take.yml` | Issue-comment trigger for `take` / `untake`. | | `label_new_issues.yml` | Issue trigger to apply `requires-triage`. | | `label_prs.yml` | Runs on `pull_request_target` so it can label pull requests opened from forks. | +| `ci_fork.yml` | Runs `ci.yml` on a push to a fork's branch; skipped upstream. See [Fork runs](#fork-runs). | +| `fork_ci_status.yml` | Mirrors a fork's run onto its pull request as `CI (fork)`. See [Fork runs](#fork-runs). | ## Reusable workflows (called by `ci.yml`) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 936b804272d..57d43ed1387 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,9 +73,11 @@ name: Comet CI # each other. Every other run is keyed on its event name: opened, synchronize # and reopened all map to `pull_request`, so a new push still supersedes its # predecessor, while a scheduled run at the tip of main never cancels that -# commit's push run. +# commit's push run. A push to a fork (ci_fork.yml) is keyed on its branch +# instead of its commit, so the next push to the branch cancels the run in +# flight. concurrency: - group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} + group: ${{ github.repository }}-${{ github.head_ref || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} cancel-in-progress: true # No `labeled` here: ci_label.yml handles that event and calls this file @@ -194,6 +196,9 @@ jobs: - name: Check pull request type labeling run: node --test dev/ci/pr-type-label.test.mjs + - name: Check fork CI status bridge + run: node --test dev/ci/test-fork-ci-status.cjs + - name: Check Iceberg shard inventory validation run: python3 dev/ci/test-iceberg-shards.py @@ -282,6 +287,10 @@ jobs: LABEL_NAME: ${{ github.event.label.name }} PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} PR_BASE_REF: ${{ github.event.pull_request.base.ref }} + PR_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + REPOSITORY: ${{ github.repository }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + PUSH_REF: ${{ github.ref }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} MQ_BASE_SHA: ${{ github.event.merge_group.base_sha }} @@ -329,8 +338,22 @@ jobs: # merge_group runs; the queue's ALLGREEN strategy is what makes # that enough (see the Merge Queue ruleset in .asf.yaml). git diff --name-only "$MQ_BASE_SHA"..."$MQ_HEAD_SHA" > changed_files.txt + elif [[ "$REPOSITORY" != "apache/datafusion-comet" && "$PUSH_REF" != "refs/heads/$DEFAULT_BRANCH" ]]; then + # A push to a feature branch of a contributor's fork (ci_fork.yml). + # There is no pull request to diff against, and the fork cannot know + # what upstream main is, so diff against the fork's own default + # branch. A fork that has not synced yet diffs against an older + # main, which can only select more suites than the pull request + # needs, never fewer. Without a default branch to diff against, or + # without a merge base, run everything the tier selects. + if git merge-base "origin/$DEFAULT_BRANCH" HEAD >/dev/null 2>&1; then + git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" > changed_files.txt + else + git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt + fi else - # push to main; first push to a branch has all-zero before sha + # push to main, upstream or a fork's own; the first push to a + # branch has an all-zero before sha if [[ "$PUSH_BEFORE" =~ ^0+$ ]]; then git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt else diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml new file mode 100644 index 00000000000..24e3bb5ebbc --- /dev/null +++ b/.github/workflows/ci_fork.yml @@ -0,0 +1,71 @@ +# 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. + +# Fork runs: a push to a branch of a contributor's fork runs the pull request +# tier on the contributor's own GitHub runners, so a fork pull request does not +# spend ASF's shared capacity on it. ci.yml upstream skips the PR tier for a +# fork pull request into main (see `is_fork_pull_request` in +# dev/ci/compute-changes.py), keeping only the preflight checks and the opt-in +# `run-*` suites, and fork_ci_status.yml mirrors this run onto the pull request +# as the informational `CI (fork)` check. The merge queue runs every suite on +# the merge result, so a fork run is feedback and never a gate. +# +# This is a separate workflow, and not a wider `push` trigger on ci.yml, for the +# same reason ci_label.yml is one. A skipped job still publishes a check run +# under its own name, and a push to a branch of this repository can land on the +# very commit a pull request is testing. A skipped `Required Checks` from that +# push would compete with the pull request's real verdict. Here the only job +# that can be skipped is named after this workflow's own run, and upstream it is +# always skipped. +# +# `main` is excluded because ci.yml already runs on push to main, in a fork as +# upstream. There it refreshes the fork's cache entries and runs the PR tier +# for what the push changed, which is what makes a fork's branch runs fast: a +# branch can restore caches saved on its own ref or on the default branch only. +name: Comet CI (fork run) + +# No `concurrency` here. ci.yml declares its own group, which for a push to a +# fork is keyed on the branch, so the next push to the branch supersedes this +# one. The same group on the caller as well would deadlock the two. +on: + push: + branches-ignore: + - main + - asf-site + # Merge queue branches exist only in this repository; the guard below + # would skip them anyway. + - 'gh-readonly-queue/**' + +permissions: + contents: read + +jobs: + ci: + name: Fork run + # Upstream, a push to any branch other than main is a pull request's own + # business: the pull request run tests it, and a second run here would + # test it twice. + if: github.repository != 'apache/datafusion-comet' + # The union of what ci.yml's jobs request, for the same reason as in + # ci_label.yml: GitHub checks a called workflow's permissions against + # this ceiling for every job when the run starts, including the ones this + # event skips. + permissions: + actions: read + contents: write + issues: write + uses: ./.github/workflows/ci.yml diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml new file mode 100644 index 00000000000..2c89eafdd1c --- /dev/null +++ b/.github/workflows/fork_ci_status.yml @@ -0,0 +1,238 @@ +# 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, or ci.yml for a PR from the +# fork's main) 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 every suite 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: + if: github.repository == 'apache/datafusion-comet' && (github.event_name != 'workflow_run' || github.event.workflow_run.event == 'pull_request') + name: Mirror the fork run + runs-on: ubuntu-slim + timeout-minutes: 10 + steps: + - name: Create or update the check + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + 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)'; + // A branch is built by ci_fork.yml; a fork's main, which that workflow + // skips, by ci.yml itself. + const workflowFor = (pr) => (pr.head.ref === 'main' ? 'ci.yml' : '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; + + const data = await optional(() => github.rest.actions.listWorkflowRuns({ + ...fork, workflow_id: workflowFor(pr), 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) + ).sort((a, b) => b.id - a.id)[0]; + let payload; + if (run) { + const status = run.status === 'completed' ? 'completed' : + run.status === 'in_progress' ? 'in_progress' : 'queued'; + payload = { + status, + ...(status === 'completed' ? { + conclusion: conclusions.has(run.conclusion) ? run.conclusion : 'failure', + } : {}), + details_url: run.html_url, + output: { + title: 'Fork CI: ' + (status === 'completed' ? run.conclusion : status), + summary: [ + '[CI run on ' + pr.head.repo.full_name + '](' + run.html_url + ') for ' + sha + '.', + 'Rerun failed checks in the fork. A green fork run tests the branch head, not its merge with main.', + UPDATE_NOTE, + ].join('\n\n'), + text: JSON.stringify({ head_sha: sha, run_id: run.id, run_attempt: run.run_attempt }), + }, + }; + } else { + const workflow = await optional(() => github.rest.actions.getWorkflow({ + ...fork, workflow_id: workflowFor(pr), + })); + const disabled = !workflow || workflow.state.startsWith('disabled'); + payload = { + status: 'completed', + conclusion: disabled ? 'action_required' : 'neutral', + details_url: 'https://github.com/' + pr.head.repo.full_name + '/actions', + output: { + title: disabled ? 'Enable CI on your fork' : 'No fork CI run for this commit', + summary: [ + 'No CI run was found for ' + sha + ' on ' + pr.head.repo.full_name + '.', + 'Enable workflows in the fork’s Actions tab, sync/rebase to include the fork CI workflow, ' + + 'then push your branch. A newly pushed run may take a moment to appear.', + UPDATE_NOTE, + '[Fork CI setup](' + GUIDE + ')', + ].join('\n\n'), + text: JSON.stringify({ head_sha: sha, run_id: null }), + }, + }; + } + if (check && check.status === payload.status && + check.conclusion === (payload.conclusion || null) && + check.output?.text === payload.output.text && + check.output?.summary === payload.output.summary) return; + if (check) { + await github.rest.checks.update({ ...upstream, check_run_id: check.id, ...payload }); + } else { + await github.rest.checks.create({ + ...upstream, name: CHECK, head_sha: sha, external_id: externalId(pr), ...payload, + }); + } + core.info('#' + pr.number + ': ' + (payload.conclusion || payload.status)); + } + + async function notify() { + const run = context.payload.workflow_run; + if (run.event !== 'pull_request' || !run.head_repository || + run.head_repository.full_name.toLowerCase() === repository.toLowerCase()) return; + // GitHub can leave workflow_run.pull_requests empty for fork PRs. Resolve + // from the API instead, and recheck the current head to ignore stale runs. + const prs = await github.paginate(github.rest.pulls.list, { + ...listing, head: run.head_repository.owner.login + ':' + run.head_branch, per_page: 100, + }); + for (const pr of prs) { + if (pr.head.sha === run.head_sha && + pr.head.repo?.full_name.toLowerCase() === run.head_repository.full_name.toLowerCase()) { + await syncCheck(pr); + } + } + } + + async function update() { + const errors = []; + // A failed update must not prevent unrelated PRs from being refreshed. + async function syncPr(pr, includeCompleted) { + try { + await syncCheck(pr, includeCompleted); + } catch (error) { + errors.push('#' + pr.number + ': ' + error.message); + } + } + const count = await github.rest.pulls.list({ ...listing, per_page: 1 }); + const last = /[?&]page=(\d+)>;\s*rel="last"/.exec(count.headers.link || ''); + const total = last ? Number(last[1]) : count.data.length; + if (!total) return; + const slices = Math.ceil(total / SWEEP_SIZE); + const slot = Math.floor(Date.now() / (15 * 60 * 1000)); + const { data: slice } = await github.rest.pulls.list({ + ...listing, per_page: SWEEP_SIZE, page: (slot % slices) + 1, + }); + const sweepNumbers = new Set(slice.map((pr) => pr.number)); + const seen = new Set(); + const cutoff = Date.now() - 6 * 3600 * 1000; + // Poll active and missing checks on recently updated PRs, and completed + // checks only in the rotating slice. This bounds expensive fork API calls. + const pages = github.paginate.iterator(github.rest.pulls.list, { ...listing, per_page: 100 }); + for await (const page of pages) { + let stale = false; + for (const pr of page.data) { + if (new Date(pr.updated_at).getTime() < cutoff) { stale = true; break; } + seen.add(pr.number); + await syncPr(pr, sweepNumbers.has(pr.number)); + } + if (stale) break; + } + for (const pr of slice) { + if (!seen.has(pr.number)) await syncPr(pr); + } + if (errors.length) core.setFailed(errors.join('\n')); + } + + if (repository === 'apache/datafusion-comet') { + if (context.eventName === 'workflow_run') await notify(); + else await update(); + } diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index c752a50c266..8a5ff9103ba 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -# Guards seven CI invariants that are silent when broken: +# Guards eight CI invariants that are silent when broken: # # 1. Change-filter routing. dev/ci/compute-changes.py decides which heavy # jobs run. A file that a job depends on but that no filter lists makes @@ -73,6 +73,11 @@ # which is how every native build in the queue came to miss its cargo # cache and pay a cold ~26 minute compile. # +# 8. Fork runs. A pull request from a fork runs no PR tier upstream; the +# contributor's fork does (ci_fork.yml), and fork_ci_status.yml mirrors it +# as an informational check. The caller, the bridge and ci.yml's own push +# trigger have to agree, or the PR tier runs twice, or nowhere. +# # Run from the repository root: python3 dev/ci/check-ci-config.py import importlib.util @@ -243,6 +248,9 @@ ALL_JOBS = QUEUE_TIER | NIGHTLY_TIER | SPARK_DEPRECATED | {"docs"} assert not QUEUE_TIER & NIGHTLY_TIER, "a job is queue or nightly, never both" +# The repository whose queue gates main; see UPSTREAM in compute-changes.py. +UPSTREAM = "apache/datafusion-comet" + POLICY_CASES = [ # A manual run may exercise anything. ({"name": "workflow_dispatch"}, ALL_JOBS), @@ -511,6 +519,90 @@ }, PR_TIER, ), + # A pull request from a fork into main runs no PR tier upstream: the + # contributor's fork runs it (ci_fork.yml) and fork_ci_status.yml mirrors it + # as the informational `CI (fork)` check. Every opt-in label still works, + # because a fork cannot see the labels on the pull request. + ( + { + "name": "pull_request", + "action": "synchronize", + "labels": [], + "base": "main", + "repository": UPSTREAM, + "head_repository": "contributor/datafusion-comet", + }, + set(), + ), + ( + { + "name": "pull_request", + "action": "synchronize", + "labels": ["run-spark-4.1-tests", "run-delta-tests"], + "base": "main", + "repository": UPSTREAM, + "head_repository": "contributor/datafusion-comet", + }, + {"spark_4_1", "spark_4_1_hive"} | DELTA_OPT_IN, + ), + # The label run adds the non-default Spark profiles to the fork's default + # profile run, exactly as it does for any other pull request. + ( + { + "name": "pull_request", + "action": "labeled", + "label": "run-all-spark-profiles", + "labels": ["run-all-spark-profiles"], + "base": "main", + "repository": UPSTREAM, + "head_repository": "contributor/datafusion-comet", + }, + {"build_linux_all_profiles"}, + ), + # The exceptions keep the PR tier upstream: a pull request from a branch of + # this repository (a committer's, or dependabot's), one that targets a + # release branch, which has no queue or nightly to catch what a fork run + # misses, and one opened inside a fork, whose own CI is the only CI. + ( + { + "name": "pull_request", + "action": "synchronize", + "labels": [], + "base": "main", + "repository": UPSTREAM, + "head_repository": UPSTREAM, + }, + PR_TIER, + ), + ( + { + "name": "pull_request", + "action": "synchronize", + "labels": [], + "base": "branch-1.1", + "repository": UPSTREAM, + "head_repository": "contributor/datafusion-comet", + }, + QUEUE_TIER | NIGHTLY_TIER, + ), + ( + { + "name": "pull_request", + "action": "synchronize", + "labels": [], + "base": "main", + "repository": "contributor/datafusion-comet", + "head_repository": "contributor/datafusion-comet", + }, + PR_TIER, + ), + # A push to a fork is the contributor's pull request tier and nothing more: + # not main's cache refresh or site deploy (`docs` publishes to asf-site), + # and not the queue's or the nightly's suites. The same holds on the fork's + # own main, which is why a sync of the fork refreshes its caches. + ({"name": "push", "repository": "contributor/datafusion-comet"}, PR_TIER), + # Upstream, push stays the cache refresh and the site deploy. + ({"name": "push", "repository": UPSTREAM}, {"docs", "build_linux", "build_macos"}), ] @@ -787,6 +879,10 @@ def check_event_policy(): label += f" +{event['label']}" if event.get("base"): label += f" base={event['base']}" + if event.get("repository"): + label += f" in {event['repository']}" + if event.get("head_repository"): + label += f" from {event['head_repository']}" failures.append( f"{label} labels={event.get('labels', [])}: " f"unexpectedly allowed {sorted(actual - expected) or 'nothing'}, " @@ -831,6 +927,8 @@ def check_event_policy(): "LABEL_NAME": "github.event.label.name", "PR_LABELS": "toJSON(github.event.pull_request.labels.*.name)", "PR_BASE_REF": "github.event.pull_request.base.ref", + "PR_HEAD_REPO": "github.event.pull_request.head.repo.full_name", + "REPOSITORY": "github.repository", } ENV_READ = re.compile(r'os\.environ\.get\("([A-Z_]+)"') ENV_SET = re.compile(r"^\s+([A-Z_]+):\s+\$\{\{\s*(.+?)\s*\}\}\s*$") @@ -1198,6 +1296,32 @@ def ci_triggers(): return triggers +def permission_ceiling_failures(caller): + """Failures for each ci.yml permission that `caller` does not grant. + + A workflow that calls ci.yml passes it a `permissions:` ceiling, and GitHub + refuses to start the run if any job in ci.yml asks for more, even a job the + event would skip. + """ + granted = permission_blocks(caller.read_text(encoding="utf-8").splitlines()) + ceiling = {} + for block in granted: + for scope, level in block.items(): + ceiling[scope] = max(ceiling.get(scope, 0), PERMISSION_RANK[level]) + failures = [] + ci = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8").splitlines() + for block in permission_blocks(ci): + for scope, level in block.items(): + if PERMISSION_RANK[level] > ceiling.get(scope, 0): + failures.append( + f"ci.yml requests `{scope}: {level}`, but {caller.name} grants " + f"the call at most `{scope}: " + f"{PERMISSION_NAME[ceiling.get(scope, 0)]}`, so every run it " + f"starts fails at startup. Raise the grant in {caller.name}" + ) + return failures + + def check_label_runs_separate(): """`labeled` runs come from ci_label.yml, never from ci.yml's own trigger. @@ -1242,21 +1366,7 @@ def check_label_runs_separate(): # even a job the `labeled` event would skip. That is how the `docs` job's # `contents: write` stopped every label run on 2026-09-29. if label_workflow.exists(): - granted = permission_blocks(label_workflow.read_text(encoding="utf-8").splitlines()) - ceiling = {} - for block in granted: - for scope, level in block.items(): - ceiling[scope] = max(ceiling.get(scope, 0), PERMISSION_RANK[level]) - ci = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8").splitlines() - for block in permission_blocks(ci): - for scope, level in block.items(): - if PERMISSION_RANK[level] > ceiling.get(scope, 0): - failures.append( - f"ci.yml requests `{scope}: {level}`, but ci_label.yml grants " - f"the call at most `{scope}: " - f"{PERMISSION_NAME[ceiling.get(scope, 0)]}`, so every label " - f"run fails at startup. Raise the grant in ci_label.yml" - ) + failures.extend(permission_ceiling_failures(label_workflow)) for failure in failures: print(f"label runs: {failure}") @@ -1560,6 +1670,100 @@ def check_cache_save_scope(): return not failures +# Fork runs. A push to a contributor's fork runs the PR tier there (ci_fork.yml), +# upstream skips that tier for a fork pull request (compute-changes.py), and +# fork_ci_status.yml mirrors the fork's run onto the pull request. The pieces +# are in three files and break silently apart: a caller that also fires upstream +# runs the PR tier twice, a caller that fires on main runs it twice in a fork, a +# caller that cannot start leaves a fork pull request with no PR tier anywhere, +# and a bridge that reads the wrong workflow name reports "no fork CI" on every +# pull request. +FORK_WORKFLOW = WORKFLOWS / "ci_fork.yml" +BRIDGE_WORKFLOW = WORKFLOWS / "fork_ci_status.yml" +FORK_TEST = Path("dev/ci/test-fork-ci-status.cjs") + + +def workflow_name(path): + match = re.search(r"^name:\s*(.+?)\s*$", path.read_text(encoding="utf-8"), re.M) + return match.group(1) if match else None + + +def check_fork_runs(): + failures = [] + ci = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8") + + # ci.yml itself must stay a push-to-main trigger: it is what runs on a + # fork's main, and a wider one would run the PR tier twice on every push to + # a fork's branch and publish skipped `Required Checks` upstream. + if not re.search(r"^ push:\n branches:\n - main\n", ci, re.M): + failures.append( + "ci.yml must trigger on `push` to `branches: [main]` only; " + "fork branches go through ci_fork.yml" + ) + if FORK_TEST.name not in ci: + failures.append(f"ci.yml's Preflight does not run {FORK_TEST}, so the bridge goes untested") + + if not FORK_WORKFLOW.exists(): + failures.append(f"{FORK_WORKFLOW} is missing; a fork pull request would run no PR tier anywhere") + else: + body = FORK_WORKFLOW.read_text(encoding="utf-8") + if not re.search(r"^\s+uses:\s*\./\.github/workflows/ci\.yml\s*$", body, re.M): + failures.append(f"{FORK_WORKFLOW} does not call ./.github/workflows/ci.yml") + ignored = re.search(r"^ branches-ignore:\n((?: .*\n)+)", body, re.M) + entries = ( + [ + line.strip()[2:].strip() + for line in ignored.group(1).splitlines() + if line.strip().startswith("- ") + ] + if ignored + else [] + ) + for needed in ("main", "asf-site", "'gh-readonly-queue/**'"): + if needed not in entries: + failures.append(f"{FORK_WORKFLOW} must list {needed} under `push: branches-ignore`") + if not re.search( + r"^ if:\s*github\.repository\s*!=\s*'apache/datafusion-comet'\s*$", body, re.M + ): + failures.append( + f"{FORK_WORKFLOW} has no `if: github.repository != 'apache/datafusion-comet'` " + f"guard, so every upstream branch push would run the PR tier a second time" + ) + if re.search(r"^concurrency:", body, re.M): + failures.append( + f"{FORK_WORKFLOW} declares `concurrency`; ci.yml's own group is keyed on the " + f"branch for a fork push, and a second group on the caller deadlocks the pair" + ) + if re.search(r"^ (pull_request|pull_request_target|workflow_dispatch|schedule):", body, re.M): + failures.append(f"{FORK_WORKFLOW} must trigger on `push` and nothing else") + failures.extend(permission_ceiling_failures(FORK_WORKFLOW)) + + if not BRIDGE_WORKFLOW.exists(): + failures.append(f"{BRIDGE_WORKFLOW} is missing; fork pull requests would show no fork result") + else: + body = BRIDGE_WORKFLOW.read_text(encoding="utf-8") + listed = re.search(r"^ workflows:\s*\[(.*?)\]\s*$", body, re.M) + ci_name = workflow_name(WORKFLOWS / "ci.yml") + if not listed or [w.strip() for w in listed.group(1).split(",")] != [ci_name]: + failures.append( + f"{BRIDGE_WORKFLOW} must trigger on `workflow_run` of exactly ci.yml's name, " + f"{ci_name!r}; a rename there silences the bridge" + ) + if "github.repository == 'apache/datafusion-comet'" not in body: + failures.append(f"{BRIDGE_WORKFLOW} has no repository guard, so it would run in every fork") + if re.search(r"uses:\s*actions/checkout@", body): + failures.append( + f"{BRIDGE_WORKFLOW} checks out code. It runs with `checks: write` on data from a " + f"fork and must never check out or run anything from the pull request" + ) + if re.search(r"^\s+(contents|pull-requests|issues):\s*write\s*$", body, re.M): + failures.append(f"{BRIDGE_WORKFLOW} asks for more than `checks: write` and read access") + + for failure in failures: + print(f"fork runs: {failure}") + return not failures + + if __name__ == "__main__": ok = check_change_filters() ok = check_event_policy() and ok @@ -1571,6 +1775,7 @@ def check_cache_save_scope(): ok = check_local_actions_have_checkout() and ok ok = check_required_checks() and ok ok = check_label_runs_separate() and ok + ok = check_fork_runs() and ok ok = check_cache_refresh_scope() and ok ok = check_nightly_scope() and ok ok = check_nightly_base_fallback() and ok diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 2e91d4c91a0..3f504c307cd 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -478,7 +478,8 @@ # Which events may run each job, independent of the path filters above. # -# "pr" every pull request +# "pr" every pull request, except one from a fork (see +# UPSTREAM below), plus a push to a fork # "queue" the merge queue, i.e. a merge_group event # "nightly" the scheduled run against main, once a day # "push" push to main @@ -622,6 +623,35 @@ } +# Pull requests from a contributor's fork do not run the PR tier on ASF runners. +# The contributor's fork runs it instead, on a push to the PR branch (see +# ci_fork.yml), and a bridge workflow (fork_ci_status.yml) mirrors that run onto +# the pull request as the informational `CI (fork)` check. The merge queue still +# runs everything on the merge result, so the fork's verdict is feedback and +# never a gate: a contributor controls their own workflows. A fork PR keeps +# every opt-in label job, and a PR from a branch of this repository, or one that +# targets a release branch, keeps the PR tier upstream. +UPSTREAM = "apache/datafusion-comet" + + +def is_fork_pull_request(event): + """Is this a pull request from a fork of UPSTREAM into the default branch?""" + head = event.get("head_repository", "") + return ( + event.get("name") == "pull_request" + and event.get("repository") == UPSTREAM + and bool(head) + and head != UPSTREAM + and event.get("base") == "main" + ) + + +def is_fork_push(event): + """Is this a push to a contributor's fork, rather than to UPSTREAM?""" + repository = event.get("repository", "") + return event.get("name") == "push" and bool(repository) and repository != UPSTREAM + + # Release branches are named `branch-.`, as in branch-0.17 and # branch-1.0. A pull request against one runs these tiers; see the end of the # comment above POLICY. @@ -636,9 +666,11 @@ def gating_labels(job): def event_allows(job, event): """Does `event` permit `job` to run, ignoring which files changed? - `event` is {"name", "action", "label", "labels", "base"}: the workflow event - name, the pull_request action, the label just added on a `labeled` event, - the labels currently on the pull request, and the branch it targets. + `event` is {"name", "action", "label", "labels", "base", "repository", + "head_repository"}: the workflow event name, the pull_request action, the + label just added on a `labeled` event, the labels currently on the pull + request, the branch it targets, the repository the workflow runs in, and + the repository the pull request's branch lives in. """ tiers = POLICY[job] name = event.get("name") @@ -646,6 +678,11 @@ def event_allows(job, event): if name == "workflow_dispatch": return True if name == "push": + # A push to a fork is the contributor's own pull request tier. The + # "push" tier is main's cache refresh and site deploy, and a fork's + # main has no site to deploy. + if is_fork_push(event): + return "pr" in tiers return "push" in tiers if name == "merge_group": return "queue" in tiers @@ -660,7 +697,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): return False # A `labeled` event fires at the same commit as the opened/synchronize run @@ -702,6 +739,8 @@ def event_from_env(): "label": os.environ.get("LABEL_NAME", ""), "labels": json.loads(labels) if labels.strip() else [], "base": os.environ.get("PR_BASE_REF", ""), + "repository": os.environ.get("REPOSITORY", ""), + "head_repository": os.environ.get("PR_HEAD_REPO", ""), } diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs new file mode 100644 index 00000000000..022fdbff8cc --- /dev/null +++ b/dev/ci/test-fork-ci-status.cjs @@ -0,0 +1,338 @@ +// 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. + +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const { readFileSync } = require('node:fs'); +const path = require('node:path'); +const { test } = require('node:test'); + +function workflowText(name) { + return readFileSync(path.join(__dirname, '../../.github/workflows', name), 'utf8'); +} +// The bridge is one github-script step, so its `script: |` block is the code +// under test. Read it textually rather than parse the YAML: that keeps this test +// free of a YAML dependency. +function scriptOf(text) { + const lines = text.split('\n'); + const start = lines.findIndex((line) => /^\s+script: \|\s*$/.test(line)); + assert.notEqual(start, -1, 'no `script: |` block'); + const indent = lines[start + 1].match(/^ */)[0].length; + const body = []; + for (const line of lines.slice(start + 1)) { + if (line.trim() !== '' && !line.startsWith(' '.repeat(indent))) break; + body.push(line.slice(indent)); + } + return body.join('\n'); +} +const bridgeScript = scriptOf(workflowText('fork_ci_status.yml')); +const ciConcurrency = /^concurrency:\n group: (.*)\n cancel-in-progress: (.*)$/m.exec( + workflowText('ci.yml')); +assert.ok(ciConcurrency, 'ci.yml has no top-level concurrency block'); +const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; +const bridge = new AsyncFunction('github', 'context', 'core', 'Date', bridgeScript); +const now = Date.UTC(2026, 9, 20, 12); +class Clock extends Date { static now() { return now; } } +const sha = 'a'.repeat(40); +const pr = { + number: 10, state: 'open', updated_at: new Date(now).toISOString(), + head: { + sha, ref: 'feature', + repo: { owner: { login: 'contributor' }, name: 'datafusion-comet', full_name: 'contributor/datafusion-comet' }, + }, + base: { ref: 'main' }, +}; +const run = (id = 1, extra = {}) => ({ + id, event: 'push', head_sha: sha, head_branch: 'feature', run_attempt: 1, + status: 'completed', conclusion: 'success', + html_url: 'https://github.com/contributor/datafusion-comet/actions/runs/' + id, + ...extra, +}); + +async function execute({ + event = 'schedule', checks = [], runs = [run()], workflowState = 'active', + recent = [pr], slice = [pr], candidates = [pr], repo = 'datafusion-comet', + owner = 'apache', runError, count = 1, notification = {}, checkErrors = {}, +} = {}) { + const writes = [], calls = [], failures = []; + let sweepPage; + const github = { rest: { checks: {}, actions: {}, pulls: {} } }; + github.rest.checks.listForRef = async () => {}; + github.rest.pulls.list = async (params) => { + calls.push(['pulls', params]); + if (params.per_page === 1) return { + data: count ? [pr] : [], + headers: count ? { link: '; rel="last"' } : {}, + }; + assert.equal(params.per_page, 25); + sweepPage = params.page; + return { data: slice }; + }; + github.rest.actions.listWorkflowRuns = async (params) => { + calls.push(['runs', params]); + assert.equal(params.owner, 'contributor'); + assert.equal(params.workflow_id, 'ci_fork.yml'); + assert.equal(params.head_sha, sha); + assert.equal(params.branch, 'feature'); + if (runError) throw Object.assign(new Error('API failure'), { status: runError }); + return { data: { workflow_runs: runs } }; + }; + github.rest.actions.getWorkflow = async () => { + if (workflowState === null) throw Object.assign(new Error('Not Found'), { status: 404 }); + return { data: { state: workflowState } }; + }; + github.rest.checks.create = async (payload) => writes.push({ method: 'create', ...payload }); + github.rest.checks.update = async (payload) => writes.push({ method: 'update', ...payload }); + github.paginate = async (fn, params) => { + if (fn === github.rest.checks.listForRef) { + calls.push(['checks', params]); + if (checkErrors[params.ref]) throw new Error(checkErrors[params.ref]); + return checks; + } + assert.equal(fn, github.rest.pulls.list); + assert.equal(params.head, 'contributor:feature'); + return candidates; + }; + github.paginate.iterator = async function* () { + for (let i = 0; i < recent.length; i += 100) yield { data: recent.slice(i, i + 100) }; + }; + await bridge(github, { + repo: { owner, repo }, eventName: event, + payload: { workflow_run: { + event: 'pull_request', head_sha: sha, head_branch: 'feature', + head_repository: pr.head.repo, pull_requests: [], ...notification, + } }, + }, { info: () => {}, setFailed: (message) => failures.push(message) }, Clock); + return { writes, calls, sweepPage, failures }; +} +async function existing(runValue = run()) { + const { writes } = await execute({ runs: [runValue] }); + return { + id: 100, app: { slug: 'github-actions' }, external_id: 'comet-fork-ci:10', + ...writes[0], conclusion: writes[0].conclusion || null, + }; +} + +test('CI completion discovers a fork PR even when the event PR list is empty', async () => { + const { writes } = await execute({ event: 'workflow_run' }); + assert.equal(writes.length, 1); + assert.equal(writes[0].head_sha, sha); + assert.equal(writes[0].name, 'CI (fork)'); + assert.equal(writes[0].conclusion, 'success'); + assert.equal(writes[0].details_url, run().html_url); +}); +test('notifications ignore stale branch heads, release PRs, closed PRs and deleted forks', async () => { + for (const candidate of [ + { ...pr, head: { ...pr.head, sha: 'b'.repeat(40) } }, + { ...pr, base: { ref: 'maintenance' } }, + { ...pr, state: 'closed' }, + { ...pr, head: { ...pr.head, repo: null } }, + ]) { + assert.deepEqual((await execute({ event: 'workflow_run', candidates: [candidate] })).writes, []); + } +}); +test('upstream pushes and merge-queue completions cannot create fork checks', async () => { + for (const event of ['push', 'merge_group']) { + assert.deepEqual((await execute({ event: 'workflow_run', notification: { event } })).writes, []); + } +}); +test('the bridge does nothing when copied into a fork', async () => { + const result = await execute({ owner: 'contributor' }); + assert.deepEqual(result.calls, []); + assert.deepEqual(result.writes, []); +}); +test('only runs for the exact branch head on push/manual events are mirrored', async () => { + const result = await execute({ runs: [ + run(8, { head_sha: 'b'.repeat(40) }), + run(7, { head_branch: 'other' }), + run(6, { event: 'pull_request' }), + run(5, { event: 'workflow_dispatch' }), + run(1), + ] }); + assert.equal(result.writes[0].details_url, run(5).html_url); +}); +for (const [state, expected] of [ + ['active', 'neutral'], ['disabled_fork', 'action_required'], [null, 'action_required'], +]) { + test('missing run reports workflow state ' + state, async () => { + const { writes } = await execute({ runs: [], workflowState: state }); + assert.equal(writes[0].conclusion, expected); + assert.match(writes[0].output.summary, /Enable workflows/); + assert.equal(JSON.parse(writes[0].output.text).run_id, null); + }); +} +test('a missing workflow endpoint is handled as missing CI', async () => { + const { writes } = await execute({ runError: 404, workflowState: null }); + assert.equal(writes[0].conclusion, 'action_required'); +}); +test('permission, rate limit and server errors are not reported as missing CI', async () => { + for (const runError of [403, 429, 500]) { + const result = await execute({ runError }); + assert.deepEqual(result.writes, []); + assert.deepEqual(result.failures, ['#10: API failure']); + await assert.rejects(execute({ event: 'workflow_run', runError }), /API failure/); + } +}); +for (const conclusion of ['failure', 'cancelled', 'timed_out', 'startup_failure', 'stale']) { + test('mirrors ' + conclusion + ' without reporting success', async () => { + const { writes } = await execute({ runs: [run(1, { conclusion })] }); + assert.equal(writes[0].conclusion, + ['startup_failure', 'stale'].includes(conclusion) ? 'failure' : conclusion); + }); +} +test('a stale run updates an existing check with failure, not the reserved conclusion', async () => { + const { writes } = await execute({ + checks: [await existing()], runs: [run(1, { conclusion: 'stale' })], + }); + assert.equal(writes[0].method, 'update'); + assert.equal(writes[0].conclusion, 'failure'); +}); +for (const source of ['recent', 'slice']) { + test('the sweep continues after errors in the ' + source + ' PRs and reports every failure', async () => { + const broken = ['b', 'c'].map((letter, index) => ({ + ...pr, number: 20 + index, head: { ...pr.head, sha: letter.repeat(40) }, + })); + const result = await execute({ + recent: [], slice: [], [source]: [...broken, pr], count: 3, + checkErrors: { + [broken[0].head.sha]: 'First API failure', + [broken[1].head.sha]: 'Second API failure', + }, + }); + assert.equal(result.writes.length, 1); + assert.equal(result.writes[0].external_id, 'comet-fork-ci:10'); + assert.equal(result.writes[0].conclusion, 'success'); + assert.deepEqual(result.failures, ['#20: First API failure\n#21: Second API failure']); + }); +} +test('a rerun of the same workflow run updates the existing check', async () => { + const check = await existing(run(1, { conclusion: 'failure' })); + const { writes } = await execute({ checks: [check], runs: [run(1, { run_attempt: 2 })] }); + assert.equal(writes[0].method, 'update'); + assert.equal(writes[0].check_run_id, 100); + assert.equal(writes[0].conclusion, 'success'); + assert.equal(JSON.parse(writes[0].output.text).run_attempt, 2); +}); +test('a manual run can replace a failed push run', async () => { + const check = await existing(run(1, { conclusion: 'failure' })); + const { writes } = await execute({ + checks: [check], runs: [run(2, { event: 'workflow_dispatch' }), run(1)], + }); + assert.equal(writes[0].details_url, run(2).html_url); +}); +test('a deleted run clears its stale success result', async () => { + const { writes } = await execute({ checks: [await existing()], runs: [] }); + assert.equal(writes[0].method, 'update'); + assert.equal(writes[0].conclusion, 'neutral'); +}); +test('unchanged results do not consume check write requests', async () => { + const check = await existing(); + // GitHub can replace details_url with the check's own URL. + for (const details_url of [check.details_url, 'https://github.com/apache/datafusion-comet/runs/100']) { + assert.deepEqual((await execute({ checks: [{ ...check, details_url }] })).writes, []); + } +}); +test('a same-name check from another app or PR is not overwritten', async () => { + const check = await existing(); + for (const foreign of [ + { ...check, app: { slug: 'other-app' } }, + { ...check, external_id: 'comet-fork-ci:20' }, + ]) { + assert.equal((await execute({ checks: [foreign] })).writes[0].method, 'create'); + } +}); +test('recent completed checks wait for the rotating sweep', async () => { + const { writes, calls } = await execute({ checks: [await existing()], slice: [] }); + assert.deepEqual(writes, []); + assert.equal(calls.filter(([kind]) => kind === 'runs').length, 0); +}); +test('late runs are recovered outside the sweep slice', async () => { + const missing = await execute({ runs: [] }); + const check = { ...await existing(), ...missing.writes[0] }; + const { writes } = await execute({ checks: [check], slice: [] }); + assert.equal(writes[0].conclusion, 'success'); +}); +for (const status of ['queued', 'in_progress']) { + test('polls recent ' + status + ' runs outside the sweep slice', async () => { + const check = await existing(run(1, { status, conclusion: null })); + const { writes } = await execute({ checks: [check], slice: [] }); + assert.equal(writes[0].conclusion, 'success'); + }); +} +test('the sweep reaches old PRs beyond 500 open PRs', async () => { + const old = { ...pr, updated_at: new Date(now - 24 * 3600 * 1000).toISOString() }; + const { writes, sweepPage } = await execute({ count: 600, recent: [old], slice: [old] }); + assert.equal(sweepPage, Math.floor(now / (15 * 60 * 1000)) % 24 + 1); + assert.equal(writes.length, 1); +}); +test('an active PR appearing in both lists is polled once', async () => { + const { calls } = await execute(); + assert.equal(calls.filter(([kind]) => kind === 'runs').length, 1); +}); +test('no open PRs means no fork API calls', async () => { + const result = await execute({ count: 0 }); + assert.equal(result.calls.length, 1); + assert.deepEqual(result.writes, []); +}); + +test('a PR from the fork main branch reads ci.yml, the workflow that builds main', async () => { + const mainPr = { ...pr, head: { ...pr.head, ref: 'main' } }; + const asked = []; + const github = { rest: { checks: {}, actions: {}, pulls: {} } }; + github.rest.checks.listForRef = async () => {}; + github.rest.checks.create = async () => {}; + github.rest.pulls.list = async () => ({ data: [mainPr], headers: {} }); + github.rest.actions.listWorkflowRuns = async (params) => { + asked.push(params.workflow_id); + return { data: { workflow_runs: [] } }; + }; + github.rest.actions.getWorkflow = async (params) => { + asked.push(params.workflow_id); + return { data: { state: 'active' } }; + }; + github.paginate = async (fn) => (fn === github.rest.checks.listForRef ? [] : [mainPr]); + github.paginate.iterator = async function* () { yield { data: [mainPr] }; }; + await bridge(github, { + repo: { owner: 'apache', repo: 'datafusion-comet' }, eventName: 'schedule', payload: {}, + }, { info: () => {}, setFailed: (message) => assert.fail(message) }, Clock); + assert.deepEqual(asked, ['ci.yml', 'ci.yml']); +}); + +function group(github) { + return ciConcurrency[1].replace(/\$\{\{\s*(.*?)\s*\}\}/g, + (_, expression) => new Function('github', 'return (' + expression + ');')(github)); +} +const forkPush = { + // GitHub expressions resolve absent properties to an empty string. + repository: 'contributor/datafusion-comet', event_name: 'push', event: {}, head_ref: '', + ref: 'refs/heads/feature', sha: 'first', workflow: 'Comet CI (fork run)', +}; +test('successive fork pushes share a cancellation group, separate branches do not', () => { + assert.equal(ciConcurrency[2], 'true'); + assert.equal(group(forkPush), group({ ...forkPush, sha: 'second' })); + assert.notEqual(group(forkPush), group({ ...forkPush, ref: 'refs/heads/other' })); +}); +test('upstream pushes, queue runs and pull requests stay separate per commit or head', () => { + for (const event_name of ['push', 'merge_group']) { + const upstream = { ...forkPush, repository: 'apache/datafusion-comet', event_name }; + assert.notEqual(group(upstream), group({ ...upstream, sha: 'second' })); + } + const upstream = { ...forkPush, repository: 'apache/datafusion-comet' }; + assert.notEqual(group(upstream), group({ ...upstream, event_name: 'merge_group' })); + const pull = { ...upstream, event_name: 'pull_request', head_ref: 'feature' }; + assert.equal(group(pull), group({ ...pull, sha: 'second' })); +}); diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index c567e6f0a75..ee6c27b8e13 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -100,6 +100,9 @@ verdict like any other suite. See [Opting a pull request into a suite the PR tier skips](#opting-a-pull-request-into-a-suite-the-pr-tier-skips) for the mechanics. +For a pull request from a fork, the pull request column's "yes" rows run on your fork rather than +here; see [Pull requests from forks](#pull-requests-from-forks). + Every job's result feeds one flat job named `Required Checks`, and that is the only status check `main` requires. A job that is skipped because the change did not touch its inputs counts as a pass. A job that fails or is cancelled turns `Required Checks` red. @@ -111,6 +114,56 @@ cases that pin both down. A pull request against a release branch runs all three tiers at once; see [Release branches](#release-branches). +## Pull requests from forks + +Nearly every pull request comes from a fork, and the PR tier is the largest share of the runner time +the project spends on CI. So for a pull request from a fork into `main`, the PR tier runs on **your +fork's** GitHub runners, not on the Apache Software Foundation's shared ones. What stays here is the +preflight checks, `Required Checks`, and any `run-*` suite a maintainer opts the pull request +into. The merge queue, which also stays here, runs every suite on the merge result before anything +lands. This is the arrangement `apache/datafusion` and `apache/arrow-rs` use. + +To get it working: + +1. **Enable GitHub Actions on your fork.** GitHub disables workflows on a new fork. Open the + fork's **Actions** tab and choose **I understand my workflows, go ahead and enable them**. +2. **Sync your fork and rebase your branch onto `main`.** The workflows that run on your fork are + the ones in your branch, so a branch cut before this arrangement existed runs nothing. Keep your + fork's own `main` in sync as well, for the reason under "The first run is slow" below. +3. **Push your branch.** Each push to a branch other than `main` starts a `Comet CI (fork run)` in + your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs + the PR tier for the files your branch changes relative to your fork's `main`: the Linux build, + lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites against Spark 4.1. + +On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is updated when +the upstream run finishes and every 15 minutes after, so it can lag the fork. Read the details in +your fork and re-run failed jobs there. If the check says **Enable CI on your fork** or **No fork CI +run for this commit**, no run was found for the pull request's head commit: Actions is disabled, the +branch does not include the fork workflows, or the run has not appeared yet. + +Some things to know: + +- **`CI (fork)` is informational.** `Required Checks` does not depend on it, because a contributor + controls the workflow that produced it. A green fork run is a good sign, not a merge gate. The + queue is the gate, and it tests your change merged with `main` and everything queued ahead of it, + where the fork run tests your branch head alone. +- **The first run is slow.** Caches are scoped to a branch or to the repository's default branch. + A fork whose `main` has never run starts every branch with empty caches. A cold run of the PR tier + on a fork took about an hour of wall time and 240 runner-minutes, of which the native build alone + was 23 minutes; with warm caches the native build takes about 4. Syncing your fork's `main` runs + the workflow there once, which writes the caches your branches then restore. +- **Opt-in suites still run here.** A fork cannot see the `run-*` labels on your pull request, so + those suites, and the `labeled` runs that start when a maintainer applies one, are unchanged. See + [Opting a pull request into a suite the PR tier skips](#opting-a-pull-request-into-a-suite-the-pr-tier-skips). +- **Not everything moves.** A pull request from a branch of this repository, such as a + committer's or dependabot's, and a pull request that targets a release branch, keep the PR tier here. + A release branch has no queue to catch what a fork run misses. +- **Opening a pull request inside your own fork** runs the full PR tier there, as it did before. + +If you cannot enable Actions on your fork, say so on the pull request. Nothing else will run the PR +tier for you before the queue does, so a maintainer may ask you to reproduce a failure with +[`dev/local-ci.sh`](#reproducing-a-suite-failure-locally) first. + ## Opting a pull request into a suite the PR tier skips Each suite outside the PR tier has a label that runs it on a pull request: @@ -449,7 +502,8 @@ The umbrella workflow, the reusable workflows it calls, and the routing tables a `dev/ci/check-ci-config.py`, which runs in preflight. It enforces that every job feeds `Required Checks`, that the required check name in `.asf.yaml` matches the job that publishes it, that artifact names are unique per producer, that the routing policy matches its test cases, and -that every job in `pr_build_linux.yml` is either a cache writer or skipped on push. Run it locally +that every job in `pr_build_linux.yml` is either a cache writer or skipped on push, and that the fork +workflows (`ci_fork.yml`, `fork_ci_status.yml`) and `ci.yml`'s push trigger agree. Run it locally before pushing a CI change: ```sh diff --git a/docs/source/contributor-guide/development.md b/docs/source/contributor-guide/development.md index 199ec017d40..8efec3ea7c3 100644 --- a/docs/source/contributor-guide/development.md +++ b/docs/source/contributor-guide/development.md @@ -646,6 +646,10 @@ the risk of accidentally overwriting another maintainer's commits when multiple to the same PR branch. If the lease check rejects the push, inspect and integrate the remote changes before retrying; do not switch to `--force` to bypass the check. +For a pull request from a fork, the pull request tier of CI runs on your fork's GitHub runners +rather than the project's. Enable GitHub Actions on your fork and keep its `main` in sync before you +push; see [Pull requests from forks](ci.md#pull-requests-from-forks). + Before submitting a pull request, follow this checklist to ensure your changes are ready: ### 1. Format Your Code From 3f2135435877463cf78d1d9caae73bbe11c7415f Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 09:48:26 -0600 Subject: [PATCH 02/12] ci: keep scheduled workflows upstream and run nothing on a fork's main A fork that enables scheduled workflows runs them on its owner's minutes. With no earlier scheduled run to diff against, a fork's nightly runs the whole nightly tier, and a fork's Miri run costs about twenty minutes a day. The nightly in ci.yml, miri.yml, stale.yml and codeql.yml now carry a `github.repository == 'apache/datafusion-comet'` guard on a scheduled event, and check-ci-config.py fails any workflow with a schedule trigger that lacks one. A push to a fork's default branch is a sync of commits the merge queue already tested. It now runs no job, instead of the pull request tier for what the sync changed. The caches it would warm save a branch run about nine minutes of fifty, so a sync would cost about as much as it saved. A pull request opened from a fork's main has nothing testing it, so the bridge says to open it from a branch rather than mirror a run that tested nothing. --- .github/workflows/README.md | 25 ++++++++---- .github/workflows/ci.yml | 8 ++++ .github/workflows/ci_fork.yml | 10 +++-- .github/workflows/codeql.yml | 3 ++ .github/workflows/fork_ci_status.yml | 34 ++++++++++++---- .github/workflows/miri.yml | 2 + .github/workflows/stale.yml | 2 + dev/ci/check-ci-config.py | 61 ++++++++++++++++++++++++++-- dev/ci/compute-changes.py | 33 ++++++++++++--- dev/ci/test-fork-ci-status.cjs | 20 ++++----- docs/source/contributor-guide/ci.md | 16 +++++--- 11 files changed, 167 insertions(+), 47 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index ba3512b76b9..fedd5336314 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -221,14 +221,23 @@ Things that follow from that, and are easy to break: - **The fork result is advisory.** `Required Checks` does not depend on `CI (fork)`, because a contributor controls the workflow that produced it. A green fork run that never ran the tests costs one queue slot, not a bad merge. -- **The fork's push to `main` uses `ci.yml` directly,** since `ci.yml` already - triggers on push to `main`, and `ci_fork.yml` ignores `main` so the two never - both fire. That run is the PR tier for whatever the push changed, which also - writes the fork's cache entries. A branch restores caches saved on its own - ref or on the fork's default branch only, so a fork whose `main` has never - run starts every branch cold (about 27 minutes for the native build alone). - The bridge reads `ci.yml` for a pull request opened from a fork's `main`, and - `ci_fork.yml` for every other branch. +- **A push to a fork's `main` runs nothing.** It is a sync of commits the queue + already tested, and `ci.yml` triggers on it because it triggers on push to + `main`, so `is_fork_default_branch_push` drops every job. The cost is that a + fork never has warm caches, so each branch run rebuilds the native library: + 59 minutes of wall time and 240 runner-minutes cold against 50 and 213 warm, + measured 2026-10-28 on one fork. A sync that did warm them would cost about + as much as it saved on its first run. The bridge answers a pull request from + a fork's `main` with a neutral "open the pull request from a branch", because + nothing tests it. +- **Scheduled workflows stay upstream.** A fork that enables scheduled + workflows would run them on its owner's minutes, and with no earlier scheduled + run to diff against, a fork's nightly runs the whole nightly tier. + `ci.yml`'s `Preflight`, `miri.yml`, `stale.yml` and `codeql.yml` carry a + `github.repository == 'apache/datafusion-comet'` guard on a scheduled + event, and `check-ci-config.py` fails any workflow with a `schedule` trigger + that lacks one. A guard on `Preflight` skips the whole nightly, since every + other job needs `changes`, which needs it. - **`ci.yml` must not trigger on every push.** A skipped job still publishes a check run under its own name, so a push to a branch of this repository that lands on a pull request's commit would put a skipped `Required Checks` next to diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 57d43ed1387..642159944cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,9 +130,17 @@ jobs: # reported verdict truthful. Skipping the redundant work is the heavy jobs' # job; POLICY in dev/ci/compute-changes.py drops every job a `labeled` event # does not gate. + # + # The one `if:` it does carry cannot hide a verdict: a scheduled run belongs + # to no pull request, so skipping it only changes what the Actions tab shows. + # It keeps the nightly upstream. A fork with scheduled workflows enabled would + # otherwise run it, and with no earlier scheduled run to diff against in the + # fork, `Detect changes` runs the whole nightly tier. Everything else here + # needs `changes`, which needs this, so the whole run is skipped. # --------------------------------------------------------------------------- preflight: name: Preflight + if: github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet' runs-on: ubuntu-slim steps: - uses: actions/checkout@v7 diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index 24e3bb5ebbc..ce92f3fec74 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -32,10 +32,12 @@ # that can be skipped is named after this workflow's own run, and upstream it is # always skipped. # -# `main` is excluded because ci.yml already runs on push to main, in a fork as -# upstream. There it refreshes the fork's cache entries and runs the PR tier -# for what the push changed, which is what makes a fork's branch runs fast: a -# branch can restore caches saved on its own ref or on the default branch only. +# `main` is excluded because a push to a fork's main is a sync of upstream, which +# the merge queue already tested. ci.yml still triggers on it there, since it +# triggers on push to main, but POLICY runs no job for it (see +# `is_fork_default_branch_push` in dev/ci/compute-changes.py). That means a +# fork's caches are never warmed, and a branch run is cold: about nine minutes +# slower than a warm one, measured on 2026-10-28. name: Comet CI (fork run) # No `concurrency` here. ci.yml declares its own group, which for a push to a diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index c834da33407..00c38cb697d 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -36,6 +36,9 @@ permissions: jobs: analyze: name: Analyze Actions + # The weekly scan runs upstream only; a fork's own pushes and pull requests + # still get one. + if: github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet' runs-on: ubuntu-slim permissions: contents: read diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index 2c89eafdd1c..b6892374a2e 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -15,8 +15,7 @@ # specific language governing permissions and limitations # under the License. -# Mirror a contributor's fork run (ci_fork.yml, or ci.yml for a PR from the -# fork's main) onto its pull request as the informational `CI (fork)` check, +# 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 @@ -65,9 +64,7 @@ jobs: // Adapted from apache/datafusion#25591. Fork results are informational: // contributors control their workflows; only upstream merge-queue CI gates merging. const CHECK = 'CI (fork)'; - // A branch is built by ci_fork.yml; a fork's main, which that workflow - // skips, by ci.yml itself. - const workflowFor = (pr) => (pr.head.ref === 'main' ? 'ci.yml' : 'ci_fork.yml'); + 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'; @@ -110,8 +107,12 @@ jobs: // A missing run must keep being polled, even though its check is completed. if (!includeCompleted && check?.status === 'completed' && previous.run_id) return; - const data = await optional(() => github.rest.actions.listWorkflowRuns({ - ...fork, workflow_id: workflowFor(pr), head_sha: sha, branch: pr.head.ref, per_page: 100, + // A fork's main 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, a sync run that tested nothing and passed. + const fromMain = pr.head.ref === 'main'; + 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. @@ -139,9 +140,26 @@ jobs: text: JSON.stringify({ head_sha: sha, run_id: run.id, run_attempt: run.run_attempt }), }, }; + } else if (fromMain) { + payload = { + status: 'completed', + conclusion: 'neutral', + details_url: GUIDE, + output: { + title: 'Open the pull request from a branch', + summary: [ + 'This pull request comes from the main branch of ' + pr.head.repo.full_name + + ', and CI does not run on a fork’s main: a push there is a sync of upstream, ' + + 'so nothing tests this change before the merge queue does.', + 'Push the change to a branch of your fork and open the pull request from it.', + '[Fork CI setup](' + GUIDE + ')', + ].join('\n\n'), + text: JSON.stringify({ head_sha: sha, run_id: null }), + }, + }; } else { const workflow = await optional(() => github.rest.actions.getWorkflow({ - ...fork, workflow_id: workflowFor(pr), + ...fork, workflow_id: WORKFLOW, })); const disabled = !workflow || workflow.state.startsWith('disabled'); payload = { diff --git a/.github/workflows/miri.yml b/.github/workflows/miri.yml index 5310af2ee40..3d8cf9b1bc7 100644 --- a/.github/workflows/miri.yml +++ b/.github/workflows/miri.yml @@ -35,6 +35,8 @@ permissions: jobs: miri: name: Miri (${{ matrix.suite }}) + # The nightly runs upstream only. A fork can still run it by dispatch. + if: github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet' runs-on: ubuntu-24.04 strategy: fail-fast: false diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index a22c42043c8..e6d50fb82aa 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -22,6 +22,8 @@ on: jobs: close-stale-prs: + # Upstream only: in a fork this would close the fork owner's own stale PRs. + if: github.repository == 'apache/datafusion-comet' runs-on: ubuntu-slim permissions: issues: write diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 8a5ff9103ba..4e3760d2fdc 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -596,11 +596,35 @@ }, PR_TIER, ), - # A push to a fork is the contributor's pull request tier and nothing more: - # not main's cache refresh or site deploy (`docs` publishes to asf-site), - # and not the queue's or the nightly's suites. The same holds on the fork's - # own main, which is why a sync of the fork refreshes its caches. + # A push to a fork's branch is the contributor's pull request tier and + # nothing more: not main's cache refresh or site deploy (`docs` publishes to + # asf-site), and not the queue's or the nightly's suites. + ( + { + "name": "push", + "repository": "contributor/datafusion-comet", + "ref": "refs/heads/my-fix", + "default_branch": "main", + }, + PR_TIER, + ), + # A push to the fork's default branch is a sync of commits the queue already + # tested, and runs nothing. + ( + { + "name": "push", + "repository": "contributor/datafusion-comet", + "ref": "refs/heads/main", + "default_branch": "main", + }, + set(), + ), + # Without the default branch in the event, a fork push is read as a + # branch's, the conservative reading. ({"name": "push", "repository": "contributor/datafusion-comet"}, PR_TIER), + # The nightly runs against the upstream repository only; a job-level guard in + # ci.yml keeps it out of forks, so the policy here is the upstream's. + ({"name": "schedule", "repository": UPSTREAM}, NIGHTLY_TIER), # Upstream, push stays the cache refresh and the site deploy. ({"name": "push", "repository": UPSTREAM}, {"docs", "build_linux", "build_macos"}), ] @@ -929,6 +953,8 @@ def check_event_policy(): "PR_BASE_REF": "github.event.pull_request.base.ref", "PR_HEAD_REPO": "github.event.pull_request.head.repo.full_name", "REPOSITORY": "github.repository", + "PUSH_REF": "github.ref", + "DEFAULT_BRANCH": "github.event.repository.default_branch", } ENV_READ = re.compile(r'os\.environ\.get\("([A-Z_]+)"') ENV_SET = re.compile(r"^\s+([A-Z_]+):\s+\$\{\{\s*(.+?)\s*\}\}\s*$") @@ -1764,6 +1790,32 @@ def check_fork_runs(): return not failures +# Scheduled workflows. GitHub runs `schedule` in any fork that has scheduled +# workflows enabled, on the fork owner's minutes. With no earlier scheduled run +# of its own to diff against, a fork's nightly runs the whole nightly tier, and +# a fork's Miri run costs twenty minutes a day. Every workflow with a `schedule` +# trigger therefore needs a job-level guard on the repository. +SCHEDULE_TRIGGER = re.compile(r"^ schedule:\s*$", re.M) +UPSTREAM_GUARD = re.compile( + r"^ if:.*github\.repository\s*==\s*'apache/datafusion-comet'", re.M +) + + +def check_scheduled_workflows_stay_upstream(): + failures = [] + for path in sorted(WORKFLOWS.glob("*.y*ml")): + body = path.read_text(encoding="utf-8") + if SCHEDULE_TRIGGER.search(body) and not UPSTREAM_GUARD.search(body): + failures.append( + f"{path} has a `schedule` trigger but no job-level " + f"`if: ... github.repository == 'apache/datafusion-comet'`, so it " + f"runs in every fork that enables scheduled workflows" + ) + for failure in failures: + print(f"scheduled workflows: {failure}") + return not failures + + if __name__ == "__main__": ok = check_change_filters() ok = check_event_policy() and ok @@ -1776,6 +1828,7 @@ def check_fork_runs(): ok = check_required_checks() and ok ok = check_label_runs_separate() and ok ok = check_fork_runs() and ok + ok = check_scheduled_workflows_stay_upstream() and ok ok = check_cache_refresh_scope() and ok ok = check_nightly_scope() and ok ok = check_nightly_base_fallback() and ok diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 3f504c307cd..4e410001dbb 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -652,6 +652,19 @@ def is_fork_push(event): return event.get("name") == "push" and bool(repository) and repository != UPSTREAM +def is_fork_default_branch_push(event): + """Is this a push to the default branch of a contributor's fork? + + That is almost always a sync from upstream: commits the merge queue has + already tested. A pull request is opened from a branch, and a fork's branch + pushes go through ci_fork.yml, so nothing needs testing here. Without a + default branch in the event, treat the push as a branch's, which is the + conservative reading. + """ + default = event.get("default_branch", "") + return is_fork_push(event) and bool(default) and event.get("ref") == f"refs/heads/{default}" + + # Release branches are named `branch-.`, as in branch-0.17 and # branch-1.0. A pull request against one runs these tiers; see the end of the # comment above POLICY. @@ -667,10 +680,11 @@ def event_allows(job, event): """Does `event` permit `job` to run, ignoring which files changed? `event` is {"name", "action", "label", "labels", "base", "repository", - "head_repository"}: the workflow event name, the pull_request action, the + "head_repository", "ref", "default_branch"}: the workflow event name, the pull_request action, the label just added on a `labeled` event, the labels currently on the pull - request, the branch it targets, the repository the workflow runs in, and - the repository the pull request's branch lives in. + request, the branch it targets, the repository the workflow runs in, the + repository the pull request's branch lives in, and for a push the ref and + the repository's default branch. """ tiers = POLICY[job] name = event.get("name") @@ -678,9 +692,14 @@ def event_allows(job, event): if name == "workflow_dispatch": return True if name == "push": - # A push to a fork is the contributor's own pull request tier. The - # "push" tier is main's cache refresh and site deploy, and a fork's - # main has no site to deploy. + # A push to a fork's branch is the contributor's own pull request tier. + # The "push" tier is main's cache refresh and site deploy, and a fork + # has no site to deploy. A sync of the fork's default branch runs + # nothing at all: it brings in commits the queue already tested, and + # the caches it would warm save a branch run only about ten of its + # fifty minutes (measured 2026-10-28: 59 minutes cold, 50 warm). + if is_fork_default_branch_push(event): + return False if is_fork_push(event): return "pr" in tiers return "push" in tiers @@ -741,6 +760,8 @@ def event_from_env(): "base": os.environ.get("PR_BASE_REF", ""), "repository": os.environ.get("REPOSITORY", ""), "head_repository": os.environ.get("PR_HEAD_REPO", ""), + "ref": os.environ.get("PUSH_REF", ""), + "default_branch": os.environ.get("DEFAULT_BRANCH", ""), } diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 022fdbff8cc..dfff820b8b6 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -289,27 +289,23 @@ test('no open PRs means no fork API calls', async () => { assert.deepEqual(result.writes, []); }); -test('a PR from the fork main branch reads ci.yml, the workflow that builds main', async () => { +test('a PR from the fork main branch is told to use a branch, and no run is read', async () => { const mainPr = { ...pr, head: { ...pr.head, ref: 'main' } }; - const asked = []; + const writes = []; const github = { rest: { checks: {}, actions: {}, pulls: {} } }; github.rest.checks.listForRef = async () => {}; - github.rest.checks.create = async () => {}; + github.rest.checks.create = async (payload) => writes.push(payload); github.rest.pulls.list = async () => ({ data: [mainPr], headers: {} }); - github.rest.actions.listWorkflowRuns = async (params) => { - asked.push(params.workflow_id); - return { data: { workflow_runs: [] } }; - }; - github.rest.actions.getWorkflow = async (params) => { - asked.push(params.workflow_id); - return { data: { state: 'active' } }; - }; + github.rest.actions.listWorkflowRuns = async () => assert.fail('a fork main has no run to read'); + github.rest.actions.getWorkflow = async () => assert.fail('a fork main has no workflow to read'); github.paginate = async (fn) => (fn === github.rest.checks.listForRef ? [] : [mainPr]); github.paginate.iterator = async function* () { yield { data: [mainPr] }; }; await bridge(github, { repo: { owner: 'apache', repo: 'datafusion-comet' }, eventName: 'schedule', payload: {}, }, { info: () => {}, setFailed: (message) => assert.fail(message) }, Clock); - assert.deepEqual(asked, ['ci.yml', 'ci.yml']); + assert.equal(writes.length, 1); + assert.equal(writes[0].conclusion, 'neutral'); + assert.equal(writes[0].output.title, 'Open the pull request from a branch'); }); function group(github) { diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index ee6c27b8e13..4d0489af37f 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -147,11 +147,17 @@ Some things to know: controls the workflow that produced it. A green fork run is a good sign, not a merge gate. The queue is the gate, and it tests your change merged with `main` and everything queued ahead of it, where the fork run tests your branch head alone. -- **The first run is slow.** Caches are scoped to a branch or to the repository's default branch. - A fork whose `main` has never run starts every branch with empty caches. A cold run of the PR tier - on a fork took about an hour of wall time and 240 runner-minutes, of which the native build alone - was 23 minutes; with warm caches the native build takes about 4. Syncing your fork's `main` runs - the workflow there once, which writes the caches your branches then restore. +- **Runs are cold.** Caches are scoped to a branch or to the repository's default branch, and a + push to your fork's `main` runs nothing, because it is a sync of commits the merge queue already + tested. So the Cargo and Maven caches are never warm on a fork, and a run rebuilds the native + library: a run of the PR tier on a fork took about an hour of wall time and 240 runner-minutes, of + which the native build alone was 23 minutes. The same run with warm caches took 50 minutes and 213 + runner-minutes. +- **Open the pull request from a branch.** Nothing runs on your fork's `main`, so a pull request + from it gets no CI before the merge queue. `CI (fork)` says so. +- **Scheduled runs stay upstream.** The nightly run, Miri, the stale-PR closer and the weekly + CodeQL scan run in `apache/datafusion-comet` only. If your fork has scheduled workflows enabled they + skip. Miri can still be run on your fork by dispatching it. - **Opt-in suites still run here.** A fork cannot see the `run-*` labels on your pull request, so those suites, and the `labeled` runs that start when a maintainer applies one, are unchanged. See [Opting a pull request into a suite the PR tier skips](#opting-a-pull-request-into-a-suite-the-pr-tier-skips). From f797f145961af56a5fbacd2e0ac3b4c6187ae4b8 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 10:01:00 -0600 Subject: [PATCH 03/12] ci: key the ci.yml concurrency group on the pull request number github.head_ref is the branch name alone, so two pull requests from different forks whose branches share a name landed in one group, and a push to one cancelled the other's run. Nearly every pull request comes from a fork. Key on the pull request number instead, which still lets the next push to a pull request supersede its last run. Tests pin that same-named branches get separate groups, that a push to one pull request shares a group across commits, and that a label run has its own. The same pattern remains in codeql.yml, miri.yml and pr_title_check.yml. --- .github/workflows/ci.yml | 10 ++++++---- dev/ci/test-fork-ci-status.cjs | 24 ++++++++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 642159944cc..11a818184d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,11 +73,13 @@ name: Comet CI # each other. Every other run is keyed on its event name: opened, synchronize # and reopened all map to `pull_request`, so a new push still supersedes its # predecessor, while a scheduled run at the tip of main never cancels that -# commit's push run. A push to a fork (ci_fork.yml) is keyed on its branch -# instead of its commit, so the next push to the branch cancels the run in -# flight. +# commit's push run. A pull request is keyed on its number, and not on its +# branch name: nearly every pull request comes from a fork, and two forks' +# branches can share a name, which would have one cancel the other's run. A push +# to a fork (ci_fork.yml) is keyed on its branch instead of its commit, so the +# next push to the branch cancels the run in flight. concurrency: - group: ${{ github.repository }}-${{ github.head_ref || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} + group: ${{ github.repository }}-${{ github.event.pull_request.number || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} cancel-in-progress: true # No `labeled` here: ci_label.yml handles that event and calls this file diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index dfff820b8b6..25f3fb931af 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -314,21 +314,37 @@ function group(github) { } const forkPush = { // GitHub expressions resolve absent properties to an empty string. - repository: 'contributor/datafusion-comet', event_name: 'push', event: {}, head_ref: '', + repository: 'contributor/datafusion-comet', event_name: 'push', event: { pull_request: { number: '' } }, ref: 'refs/heads/feature', sha: 'first', workflow: 'Comet CI (fork run)', }; +const pullRequest = (number, extra = {}) => ({ + ...forkPush, repository: 'apache/datafusion-comet', event_name: 'pull_request', + event: { pull_request: { number } }, ref: 'refs/pull/' + number + '/merge', ...extra, +}); test('successive fork pushes share a cancellation group, separate branches do not', () => { assert.equal(ciConcurrency[2], 'true'); assert.equal(group(forkPush), group({ ...forkPush, sha: 'second' })); assert.notEqual(group(forkPush), group({ ...forkPush, ref: 'refs/heads/other' })); }); -test('upstream pushes, queue runs and pull requests stay separate per commit or head', () => { +test('upstream pushes and queue runs stay separate per commit', () => { for (const event_name of ['push', 'merge_group']) { const upstream = { ...forkPush, repository: 'apache/datafusion-comet', event_name }; assert.notEqual(group(upstream), group({ ...upstream, sha: 'second' })); } const upstream = { ...forkPush, repository: 'apache/datafusion-comet' }; assert.notEqual(group(upstream), group({ ...upstream, event_name: 'merge_group' })); - const pull = { ...upstream, event_name: 'pull_request', head_ref: 'feature' }; - assert.equal(group(pull), group({ ...pull, sha: 'second' })); +}); +test('a push to a pull request supersedes the last one, whatever the commit', () => { + assert.equal(group(pullRequest(7, { sha: 'first' })), group(pullRequest(7, { sha: 'second' }))); +}); +test('same-named branches from different forks cannot cancel each other', () => { + // github.head_ref is the branch name alone, so it is the same for both. + const one = pullRequest(7, { head_ref: 'patch-1' }); + const other = pullRequest(8, { head_ref: 'patch-1' }); + assert.notEqual(group(one), group(other)); +}); +test('a label run is not cancelled by the commit run, or by another label', () => { + const labeled = (name) => pullRequest(7, { event: { action: 'labeled', label: { name }, pull_request: { number: 7 } } }); + assert.notEqual(group(labeled('run-iceberg-tests')), group(pullRequest(7))); + assert.notEqual(group(labeled('run-iceberg-tests')), group(labeled('run-delta-tests'))); }); From d3e2be872e47d30515efa51a64e6524e9cccc45f Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 10:11:29 -0600 Subject: [PATCH 04/12] ci: describe the fork pull request tier in ci.yml's header and tidy a docstring --- .github/workflows/ci.yml | 7 +++++++ dev/ci/compute-changes.py | 12 ++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 11a818184d8..0da04c1f208 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,13 @@ # green nightly. A failure opens (or comments on) an issue # labelled `ci-nightly-failure`. # +# A pull request from a fork into main does not run the PR tier here. The +# contributor's fork runs it (ci_fork.yml), and fork_ci_status.yml mirrors that +# run onto the pull request as the informational `CI (fork)` check, which +# `Required Checks` does not depend on. The merge queue still runs everything on +# the merge result. Opt-in `run-*` suites, pull requests from a branch of this +# repository and pull requests to a release branch are unaffected. +# # Spark 3.4 is deprecated and sits outside all three tiers: it runs only when # a pull request carries `run-spark-3.4-tests`, or from a manual dispatch. # diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 4e410001dbb..e1e7ec87481 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -680,11 +680,11 @@ def event_allows(job, event): """Does `event` permit `job` to run, ignoring which files changed? `event` is {"name", "action", "label", "labels", "base", "repository", - "head_repository", "ref", "default_branch"}: the workflow event name, the pull_request action, the - label just added on a `labeled` event, the labels currently on the pull - request, the branch it targets, the repository the workflow runs in, the - repository the pull request's branch lives in, and for a push the ref and - the repository's default branch. + "head_repository", "ref", "default_branch"}: the workflow event name, the + pull_request action, the label just added on a `labeled` event, the labels + currently on the pull request, the branch it targets, the repository the + workflow runs in, the repository the pull request's branch lives in, and + for a push the ref and the repository's default branch. """ tiers = POLICY[job] name = event.get("name") @@ -696,7 +696,7 @@ def event_allows(job, event): # The "push" tier is main's cache refresh and site deploy, and a fork # has no site to deploy. A sync of the fork's default branch runs # nothing at all: it brings in commits the queue already tested, and - # the caches it would warm save a branch run only about ten of its + # the caches it would warm save a branch run only about nine of its # fifty minutes (measured 2026-10-28: 59 minutes cold, 50 warm). if is_fork_default_branch_push(event): return False From 2cceb0fc220b913cc3d21d8e7c24210fdb3ff9de Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 10:18:34 -0600 Subject: [PATCH 05/12] ci: address review of the fork run changes - A fork push that diffs to nothing, such as a branch whose commits are already on the fork's default branch, runs the whole tier instead of reporting a green run for a change nobody built. - Preflight skips a push to a fork's default branch, whatever it is called, so a sync starts no run work at all. ci_fork.yml and the bridge use the repository's default branch rather than `main`. This replaces the same rule in compute-changes.py. - A pull request opened inside a fork leaves the PR tier to its branch push, so the same commit is not tested twice. A `run-*` label still runs its suite. - Preflight's header comment no longer claims it has no `if:`. - The bridge uses actions/github-script@v9 like the other workflows. - AGENTS.md says a fork pull request's PR tier runs on the fork. --- .github/workflows/README.md | 14 +++-- .github/workflows/ci.yml | 48 ++++++++++------- .github/workflows/ci_fork.yml | 16 +++--- .github/workflows/fork_ci_status.yml | 14 ++--- AGENTS.md | 8 +-- dev/ci/check-ci-config.py | 81 ++++++++++++++++------------ dev/ci/compute-changes.py | 50 ++++++++--------- dev/ci/test-fork-ci-status.cjs | 11 ++-- docs/source/contributor-guide/ci.md | 10 ++-- 9 files changed, 140 insertions(+), 112 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index fedd5336314..211a43b8c99 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -223,13 +223,21 @@ Things that follow from that, and are easy to break: green fork run that never ran the tests costs one queue slot, not a bad merge. - **A push to a fork's `main` runs nothing.** It is a sync of commits the queue already tested, and `ci.yml` triggers on it because it triggers on push to - `main`, so `is_fork_default_branch_push` drops every job. The cost is that a + `main`, so its `Preflight` skips the run, and with it every job. The same + skip covers a fork whose default branch is not `main`. The cost is that a fork never has warm caches, so each branch run rebuilds the native library: 59 minutes of wall time and 240 runner-minutes cold against 50 and 213 warm, measured 2026-10-28 on one fork. A sync that did warm them would cost about as much as it saved on its first run. The bridge answers a pull request from - a fork's `main` with a neutral "open the pull request from a branch", because - nothing tests it. + a fork's default branch with a neutral "open the pull request from a branch", + because nothing tests it. +- **A pull request opened inside a fork skips the PR tier,** because the branch + push already ran it, and `is_fork_pull_request` covers both that and a fork + pull request into `main` here. A `run-*` label still runs its suite. +- **A fork push that diffs to nothing runs everything.** `Detect changes` diffs + against the fork's default branch, and a branch whose commits are already + there, such as `git push fork main:my-fix`, would otherwise skip every suite and + report a green run for a change nobody built. - **Scheduled workflows stay upstream.** A fork that enables scheduled workflows would run them on its owner's minutes, and with no earlier scheduled run to diff against, a fork's nightly runs the whole nightly tier. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0da04c1f208..27f89775c61 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -129,7 +129,8 @@ jobs: # pr_title_check stays a standalone workflow because it needs to fire on PR # `edited` events. # - # This job deliberately carries no `if:`. A job held back by `if:` still + # This job deliberately carries no `if:` that reads the pull request or its + # labels. A job held back by `if:` still # publishes a check run under its own name with conclusion `skipped`, and the # newest check run for a name is the one the merge box, `gh pr checks` and # required-status-check evaluation read. Guarding this job on the label name @@ -140,16 +141,20 @@ jobs: # job; POLICY in dev/ci/compute-changes.py drops every job a `labeled` event # does not gate. # - # The one `if:` it does carry cannot hide a verdict: a scheduled run belongs - # to no pull request, so skipping it only changes what the Actions tab shows. - # It keeps the nightly upstream. A fork with scheduled workflows enabled would - # otherwise run it, and with no earlier scheduled run to diff against in the - # fork, `Detect changes` runs the whole nightly tier. Everything else here - # needs `changes`, which needs this, so the whole run is skipped. + # The two events it does skip belong to no pull request, so skipping them only + # changes what a fork's Actions tab shows. A scheduled run outside this + # repository: a fork with scheduled workflows enabled would otherwise run the + # nightly, and with no earlier scheduled run to diff against in the fork, + # `Detect changes` runs the whole nightly tier. And a push to a fork's default + # branch, which is a sync of commits the queue already tested. Everything else + # here needs `changes`, which needs this, so either run is skipped whole. # --------------------------------------------------------------------------- preflight: name: Preflight - if: github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet' + if: >- + (github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet') + && (github.event_name != 'push' || github.repository == 'apache/datafusion-comet' + || github.ref_name != github.event.repository.default_branch) runs-on: ubuntu-slim steps: - uses: actions/checkout@v7 @@ -307,7 +312,6 @@ jobs: PR_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} REPOSITORY: ${{ github.repository }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} - PUSH_REF: ${{ github.ref }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} MQ_BASE_SHA: ${{ github.event.merge_group.base_sha }} @@ -355,22 +359,26 @@ jobs: # merge_group runs; the queue's ALLGREEN strategy is what makes # that enough (see the Merge Queue ruleset in .asf.yaml). git diff --name-only "$MQ_BASE_SHA"..."$MQ_HEAD_SHA" > changed_files.txt - elif [[ "$REPOSITORY" != "apache/datafusion-comet" && "$PUSH_REF" != "refs/heads/$DEFAULT_BRANCH" ]]; then - # A push to a feature branch of a contributor's fork (ci_fork.yml). - # There is no pull request to diff against, and the fork cannot know - # what upstream main is, so diff against the fork's own default - # branch. A fork that has not synced yet diffs against an older - # main, which can only select more suites than the pull request - # needs, never fewer. Without a default branch to diff against, or - # without a merge base, run everything the tier selects. + elif [[ "$REPOSITORY" != "apache/datafusion-comet" ]]; then + # A push to a branch of a contributor's fork (ci_fork.yml). There is + # no pull request to diff against, and the fork cannot know what + # upstream main is, so diff against the fork's own default branch. + # A fork that has not synced yet diffs against an older main, which + # selects more suites than the pull request needs, not fewer. + # + # Run everything the tier selects when there is nothing to go on: no + # default branch or merge base, or an empty diff. A branch whose + # commits are already on the fork's default branch, such as + # `git push fork main:my-fix`, diffs to nothing, and skipping every + # suite would report a green run for a change nobody built. if git merge-base "origin/$DEFAULT_BRANCH" HEAD >/dev/null 2>&1; then git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" > changed_files.txt - else + fi + if [[ ! -s changed_files.txt ]]; then git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt fi else - # push to main, upstream or a fork's own; the first push to a - # branch has an all-zero before sha + # push to main; the first push to a branch has an all-zero before sha if [[ "$PUSH_BEFORE" =~ ^0+$ ]]; then git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt else diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index ce92f3fec74..3d2c3bb72bb 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -32,12 +32,11 @@ # that can be skipped is named after this workflow's own run, and upstream it is # always skipped. # -# `main` is excluded because a push to a fork's main is a sync of upstream, which -# the merge queue already tested. ci.yml still triggers on it there, since it -# triggers on push to main, but POLICY runs no job for it (see -# `is_fork_default_branch_push` in dev/ci/compute-changes.py). That means a -# fork's caches are never warmed, and a branch run is cold: about nine minutes -# slower than a warm one, measured on 2026-10-28. +# A fork's default branch is excluded because a push to it is a sync of upstream, +# which the merge queue already tested. ci.yml still triggers on a push to `main` +# there, but its Preflight job skips that run. That means a fork's caches are +# never warmed, and a branch run is cold: about nine minutes slower than a warm +# one, measured on 2026-10-28. name: Comet CI (fork run) # No `concurrency` here. ci.yml declares its own group, which for a push to a @@ -60,8 +59,9 @@ jobs: name: Fork run # Upstream, a push to any branch other than main is a pull request's own # business: the pull request run tests it, and a second run here would - # test it twice. - if: github.repository != 'apache/datafusion-comet' + # test it twice. In a fork the default branch is skipped, whatever it is + # called; `branches-ignore` above can only name `main`. + if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch # The union of what ci.yml's jobs request, for the same reason as in # ci_label.yml: GitHub checks a called workflow's permissions against # this ceiling for every job when the run starts, including the ones this diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index b6892374a2e..e4f2bdaede8 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -58,7 +58,7 @@ jobs: timeout-minutes: 10 steps: - name: Create or update the check - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + uses: actions/github-script@v9 with: script: | // Adapted from apache/datafusion#25591. Fork results are informational: @@ -107,10 +107,10 @@ jobs: // 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 main 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, a sync run that tested nothing and passed. - const fromMain = pr.head.ref === 'main'; + // 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, })); @@ -148,8 +148,8 @@ jobs: output: { title: 'Open the pull request from a branch', summary: [ - 'This pull request comes from the main branch of ' + pr.head.repo.full_name + - ', and CI does not run on a fork’s main: a push there is a sync of upstream, ' + + 'This pull request comes from the ' + pr.head.ref + ' branch of ' + pr.head.repo.full_name + + ', and CI does not run on a fork’s default branch: a push there is a sync of upstream, ' + 'so nothing tests this change before the merge queue does.', 'Push the change to a branch of your fork and open the pull request from it.', '[Fork CI setup](' + GUIDE + ')', diff --git a/AGENTS.md b/AGENTS.md index b7a732268dc..8e6e8fda096 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,9 +46,11 @@ remote changes before retrying; do not bypass it with `--force`. See ## Checking a change against CI A green pull request does not mean a change is safe to queue. The pull request tier runs the Comet -suites against the default Spark profile only. Spark's own SQL suite and the Iceberg suites first -report in the merge queue, where a failure evicts the pull request and blocks everyone else's -merges, or in the nightly run, after the change has already landed. +suites against the default Spark profile only. For a pull request from a fork it runs on the fork, +where `CI (fork)` reports it as advisory, and `Required Checks` here passes without a build. Spark's +own SQL suite and the Iceberg suites first report in the merge queue, where a failure evicts the +pull request and blocks everyone else's merges, or in the nightly run, after the change has already +landed. So when a change touches the serde, the planner, a native operator, a Spark shim, an Iceberg code path, or anything under `dev/diffs/`, get a verdict first. Either run the suite locally: diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 4e3760d2fdc..4bf3e8e3552 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -562,7 +562,7 @@ # The exceptions keep the PR tier upstream: a pull request from a branch of # this repository (a committer's, or dependabot's), one that targets a # release branch, which has no queue or nightly to catch what a fork run - # misses, and one opened inside a fork, whose own CI is the only CI. + # misses. ( { "name": "pull_request", @@ -585,6 +585,15 @@ }, QUEUE_TIER | NIGHTLY_TIER, ), + # A push to a fork's branch is the contributor's pull request tier and + # nothing more: not main's cache refresh or site deploy (`docs` publishes to + # asf-site), and not the queue's or the nightly's suites. A push to the + # fork's default branch, a sync, never reaches this table: Preflight skips it + # (see check_fork_runs). + ({"name": "push", "repository": "contributor/datafusion-comet"}, PR_TIER), + # A pull request opened inside a fork leaves the PR tier to the branch push + # that already ran it, so the same commit is not tested twice, but a label + # still runs its suite there. ( { "name": "pull_request", @@ -594,37 +603,19 @@ "repository": "contributor/datafusion-comet", "head_repository": "contributor/datafusion-comet", }, - PR_TIER, - ), - # A push to a fork's branch is the contributor's pull request tier and - # nothing more: not main's cache refresh or site deploy (`docs` publishes to - # asf-site), and not the queue's or the nightly's suites. - ( - { - "name": "push", - "repository": "contributor/datafusion-comet", - "ref": "refs/heads/my-fix", - "default_branch": "main", - }, - PR_TIER, + set(), ), - # A push to the fork's default branch is a sync of commits the queue already - # tested, and runs nothing. ( { - "name": "push", + "name": "pull_request", + "action": "synchronize", + "labels": ["run-iceberg-tests"], + "base": "main", "repository": "contributor/datafusion-comet", - "ref": "refs/heads/main", - "default_branch": "main", + "head_repository": "contributor/datafusion-comet", }, - set(), + ICEBERG_OPT_IN, ), - # Without the default branch in the event, a fork push is read as a - # branch's, the conservative reading. - ({"name": "push", "repository": "contributor/datafusion-comet"}, PR_TIER), - # The nightly runs against the upstream repository only; a job-level guard in - # ci.yml keeps it out of forks, so the policy here is the upstream's. - ({"name": "schedule", "repository": UPSTREAM}, NIGHTLY_TIER), # Upstream, push stays the cache refresh and the site deploy. ({"name": "push", "repository": UPSTREAM}, {"docs", "build_linux", "build_macos"}), ] @@ -953,8 +944,6 @@ def check_event_policy(): "PR_BASE_REF": "github.event.pull_request.base.ref", "PR_HEAD_REPO": "github.event.pull_request.head.repo.full_name", "REPOSITORY": "github.repository", - "PUSH_REF": "github.ref", - "DEFAULT_BRANCH": "github.event.repository.default_branch", } ENV_READ = re.compile(r'os\.environ\.get\("([A-Z_]+)"') ENV_SET = re.compile(r"^\s+([A-Z_]+):\s+\$\{\{\s*(.+?)\s*\}\}\s*$") @@ -1726,6 +1715,16 @@ def check_fork_runs(): "ci.yml must trigger on `push` to `branches: [main]` only; " "fork branches go through ci_fork.yml" ) + # Preflight is where a push to a fork's default branch, a sync of commits the + # queue already tested, is dropped: ci.yml triggers on push to main, so it + # fires there too, and every other job needs Preflight through `changes`. + preflight = re.search(r"^ preflight:\n(?: (?!runs-on)[^\n]*\n|\n)*", ci, re.M) + if not preflight or "github.event.repository.default_branch" not in preflight.group(0): + failures.append( + "ci.yml's Preflight must skip a push to a fork's default branch " + "(`github.ref_name != github.event.repository.default_branch`); " + "otherwise every sync of a fork runs CI" + ) if FORK_TEST.name not in ci: failures.append(f"ci.yml's Preflight does not run {FORK_TEST}, so the bridge goes untested") @@ -1748,13 +1747,18 @@ def check_fork_runs(): for needed in ("main", "asf-site", "'gh-readonly-queue/**'"): if needed not in entries: failures.append(f"{FORK_WORKFLOW} must list {needed} under `push: branches-ignore`") - if not re.search( - r"^ if:\s*github\.repository\s*!=\s*'apache/datafusion-comet'\s*$", body, re.M - ): + conditions = " ".join(job_conditions(body)) + if "github.repository != 'apache/datafusion-comet'" not in conditions: failures.append( f"{FORK_WORKFLOW} has no `if: github.repository != 'apache/datafusion-comet'` " f"guard, so every upstream branch push would run the PR tier a second time" ) + if "github.event.repository.default_branch" not in conditions: + failures.append( + f"{FORK_WORKFLOW} does not skip the fork's default branch, so a sync of a fork " + f"whose default branch is not `main` would start a run; `branches-ignore` " + f"can only name `main`" + ) if re.search(r"^concurrency:", body, re.M): failures.append( f"{FORK_WORKFLOW} declares `concurrency`; ci.yml's own group is keyed on the " @@ -1796,16 +1800,23 @@ def check_fork_runs(): # a fork's Miri run costs twenty minutes a day. Every workflow with a `schedule` # trigger therefore needs a job-level guard on the repository. SCHEDULE_TRIGGER = re.compile(r"^ schedule:\s*$", re.M) -UPSTREAM_GUARD = re.compile( - r"^ if:.*github\.repository\s*==\s*'apache/datafusion-comet'", re.M -) +UPSTREAM_GUARD = re.compile(r"github\.repository\s*==\s*'apache/datafusion-comet'") +# A job-level `if:` at four spaces of indent, with its continuation lines when it +# is a folded block (`if: >-`). +JOB_CONDITION = re.compile(r"^ if:[^\n]*\n(?: [^\n]*\n)*", re.M) + + +def job_conditions(body): + return JOB_CONDITION.findall(body) def check_scheduled_workflows_stay_upstream(): failures = [] for path in sorted(WORKFLOWS.glob("*.y*ml")): body = path.read_text(encoding="utf-8") - if SCHEDULE_TRIGGER.search(body) and not UPSTREAM_GUARD.search(body): + if SCHEDULE_TRIGGER.search(body) and not any( + UPSTREAM_GUARD.search(c) for c in job_conditions(body) + ): failures.append( f"{path} has a `schedule` trigger but no job-level " f"`if: ... github.repository == 'apache/datafusion-comet'`, so it " diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index e1e7ec87481..c167f5fa990 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -635,11 +635,21 @@ def is_fork_pull_request(event): - """Is this a pull request from a fork of UPSTREAM into the default branch?""" + """Does the pull request's PR tier run on a fork, not here? + + That is a pull request from a fork of UPSTREAM into the default branch. It + is also any pull request opened inside a fork, whose own branch push has + already run the tier through ci_fork.yml: running it again on the + `pull_request` event would test the same commit twice. + """ + if event.get("name") != "pull_request": + return False + repository = event.get("repository", "") + if repository and repository != UPSTREAM: + return True head = event.get("head_repository", "") return ( - event.get("name") == "pull_request" - and event.get("repository") == UPSTREAM + repository == UPSTREAM and bool(head) and head != UPSTREAM and event.get("base") == "main" @@ -652,19 +662,6 @@ def is_fork_push(event): return event.get("name") == "push" and bool(repository) and repository != UPSTREAM -def is_fork_default_branch_push(event): - """Is this a push to the default branch of a contributor's fork? - - That is almost always a sync from upstream: commits the merge queue has - already tested. A pull request is opened from a branch, and a fork's branch - pushes go through ci_fork.yml, so nothing needs testing here. Without a - default branch in the event, treat the push as a branch's, which is the - conservative reading. - """ - default = event.get("default_branch", "") - return is_fork_push(event) and bool(default) and event.get("ref") == f"refs/heads/{default}" - - # Release branches are named `branch-.`, as in branch-0.17 and # branch-1.0. A pull request against one runs these tiers; see the end of the # comment above POLICY. @@ -680,11 +677,10 @@ def event_allows(job, event): """Does `event` permit `job` to run, ignoring which files changed? `event` is {"name", "action", "label", "labels", "base", "repository", - "head_repository", "ref", "default_branch"}: the workflow event name, the - pull_request action, the label just added on a `labeled` event, the labels - currently on the pull request, the branch it targets, the repository the - workflow runs in, the repository the pull request's branch lives in, and - for a push the ref and the repository's default branch. + "head_repository"}: the workflow event name, the pull_request action, the + label just added on a `labeled` event, the labels currently on the pull + request, the branch it targets, the repository the workflow runs in, and + the repository the pull request's branch lives in. """ tiers = POLICY[job] name = event.get("name") @@ -694,12 +690,10 @@ def event_allows(job, event): if name == "push": # A push to a fork's branch is the contributor's own pull request tier. # The "push" tier is main's cache refresh and site deploy, and a fork - # has no site to deploy. A sync of the fork's default branch runs - # nothing at all: it brings in commits the queue already tested, and - # the caches it would warm save a branch run only about nine of its - # fifty minutes (measured 2026-10-28: 59 minutes cold, 50 warm). - if is_fork_default_branch_push(event): - return False + # has no site to deploy. A push to a fork's default branch never gets + # here: Preflight skips it. It is a sync of commits the queue already + # tested, and the caches it would warm save a branch run only about nine + # of its fifty minutes (measured 2026-10-28: 59 minutes cold, 50 warm). if is_fork_push(event): return "pr" in tiers return "push" in tiers @@ -760,8 +754,6 @@ def event_from_env(): "base": os.environ.get("PR_BASE_REF", ""), "repository": os.environ.get("REPOSITORY", ""), "head_repository": os.environ.get("PR_HEAD_REPO", ""), - "ref": os.environ.get("PUSH_REF", ""), - "default_branch": os.environ.get("DEFAULT_BRANCH", ""), } diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 25f3fb931af..317098daf79 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -52,7 +52,10 @@ const pr = { number: 10, state: 'open', updated_at: new Date(now).toISOString(), head: { sha, ref: 'feature', - repo: { owner: { login: 'contributor' }, name: 'datafusion-comet', full_name: 'contributor/datafusion-comet' }, + repo: { + owner: { login: 'contributor' }, name: 'datafusion-comet', + full_name: 'contributor/datafusion-comet', default_branch: 'main', + }, }, base: { ref: 'main' }, }; @@ -289,8 +292,9 @@ test('no open PRs means no fork API calls', async () => { assert.deepEqual(result.writes, []); }); -test('a PR from the fork main branch is told to use a branch, and no run is read', async () => { - const mainPr = { ...pr, head: { ...pr.head, ref: 'main' } }; +for (const defaultBranch of ['main', 'develop']) { +test('a PR from the fork default branch ' + defaultBranch + ' is told to use a branch, and no run is read', async () => { + const mainPr = { ...pr, head: { ...pr.head, ref: defaultBranch, repo: { ...pr.head.repo, default_branch: defaultBranch } } }; const writes = []; const github = { rest: { checks: {}, actions: {}, pulls: {} } }; github.rest.checks.listForRef = async () => {}; @@ -307,6 +311,7 @@ test('a PR from the fork main branch is told to use a branch, and no run is read assert.equal(writes[0].conclusion, 'neutral'); assert.equal(writes[0].output.title, 'Open the pull request from a branch'); }); +} function group(github) { return ciConcurrency[1].replace(/\$\{\{\s*(.*?)\s*\}\}/g, diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index 4d0489af37f..8278dc12c39 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -128,12 +128,13 @@ To get it working: 1. **Enable GitHub Actions on your fork.** GitHub disables workflows on a new fork. Open the fork's **Actions** tab and choose **I understand my workflows, go ahead and enable them**. 2. **Sync your fork and rebase your branch onto `main`.** The workflows that run on your fork are - the ones in your branch, so a branch cut before this arrangement existed runs nothing. Keep your - fork's own `main` in sync as well, for the reason under "The first run is slow" below. + the ones in your branch, so a branch cut before this arrangement existed runs nothing. 3. **Push your branch.** Each push to a branch other than `main` starts a `Comet CI (fork run)` in your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs the PR tier for the files your branch changes relative to your fork's `main`: the Linux build, - lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites against Spark 4.1. + lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites against Spark 4.1. If that + comparison finds nothing, because the commits are already on your fork's `main`, it runs the + whole tier. On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is updated when the upstream run finishes and every 15 minutes after, so it can lag the fork. Read the details in @@ -164,7 +165,8 @@ Some things to know: - **Not everything moves.** A pull request from a branch of this repository, such as a committer's or dependabot's, and a pull request that targets a release branch, keep the PR tier here. A release branch has no queue to catch what a fork run misses. -- **Opening a pull request inside your own fork** runs the full PR tier there, as it did before. +- **Opening a pull request inside your own fork** leaves the PR tier to your branch push, so the + same commit is not tested twice. A `run-*` label applied there still runs its suite. If you cannot enable Actions on your fork, say so on the pull request. Nothing else will run the PR tier for you before the queue does, so a maintainer may ask you to reproduce a failure with From 997c723818f9ef993a2ecda483d15308167c9c6d Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 10:24:38 -0600 Subject: [PATCH 06/12] ci: simplify the fork run changes - The bridge job skips a workflow_run for a pull request from a branch of this repository, which has no fork run to mirror, and drops a repository guard the job already had. Its fork-default-branch test reuses the shared harness. - Detect changes folds the fork push fallbacks into one empty-file check. - check-ci-config.py asserts its UPSTREAM matches compute-changes.py, shares the regex for a job that calls ci.yml, builds the new policy cases with a helper, checks that the bridge reads runs of ci_fork.yml, and lists its ninth invariant in the header. - ci_fork.yml no longer ignores queue branches, which the repository guard already skips. - Comments and the workflows README no longer repeat the cold-cache measurements or the same bullet twice. --- .github/workflows/README.md | 16 ++- .github/workflows/ci.yml | 56 +++++------ .github/workflows/ci_fork.yml | 9 +- .github/workflows/fork_ci_status.yml | 14 ++- dev/ci/check-ci-config.py | 139 ++++++++++----------------- dev/ci/compute-changes.py | 4 +- dev/ci/test-fork-ci-status.cjs | 34 +++---- 7 files changed, 103 insertions(+), 169 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 211a43b8c99..01a7452ac29 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -234,10 +234,13 @@ Things that follow from that, and are easy to break: - **A pull request opened inside a fork skips the PR tier,** because the branch push already ran it, and `is_fork_pull_request` covers both that and a fork pull request into `main` here. A `run-*` label still runs its suite. -- **A fork push that diffs to nothing runs everything.** `Detect changes` diffs - against the fork's default branch, and a branch whose commits are already - there, such as `git push fork main:my-fix`, would otherwise skip every suite and - report a green run for a change nobody built. +- **A fork push diffs against the fork's default branch.** There is no pull + request to diff against, and a fork does not know what upstream `main` is, so + `Detect changes` uses `git diff origin/...`. A fork that + has not synced selects more suites, not fewer. With nothing to go on, or a diff + that is empty, it runs everything: a branch whose commits are already on the + default branch, such as `git push fork main:my-fix`, would otherwise skip every + suite and report a green run for a change nobody built. - **Scheduled workflows stay upstream.** A fork that enables scheduled workflows would run them on its owner's minutes, and with no earlier scheduled run to diff against, a fork's nightly runs the whole nightly tier. @@ -252,11 +255,6 @@ Things that follow from that, and are easy to break: the real one. `ci_fork.yml` is a separate workflow so the only name that can be skipped is its own, and `check-ci-config.py` pins its trigger and the `github.repository != 'apache/datafusion-comet'` guard. -- **A fork push diffs against the fork's default branch.** There is no pull - request to diff against, and a fork does not know what upstream `main` is, so - `Detect changes` uses `git diff origin/...`. A fork that - has not synced diffs against an older `main`, which selects more suites, never - fewer. - **Exceptions stay upstream:** a pull request from a branch of this repository (a committer's, dependabot's), one that targets a release branch, and the opt-in `run-*` suites. A fork cannot see the labels on its pull diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 27f89775c61..6a719accdb7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,24 +130,21 @@ jobs: # `edited` events. # # This job deliberately carries no `if:` that reads the pull request or its - # labels. A job held back by `if:` still - # publishes a check run under its own name with conclusion `skipped`, and the - # newest check run for a name is the one the merge box, `gh pr checks` and - # required-status-check evaluation read. Guarding this job on the label name - # therefore let any non-gating label overwrite the commit run's real - # `Preflight` verdict with `skipped` (issue #5007). Running it unconditionally - # costs about a minute of ubuntu-slim time per label event and keeps the - # reported verdict truthful. Skipping the redundant work is the heavy jobs' - # job; POLICY in dev/ci/compute-changes.py drops every job a `labeled` event - # does not gate. + # labels. A job held back by `if:` still publishes a check run under its own + # name with conclusion `skipped`, and the newest check run for a name is the + # one the merge box, `gh pr checks` and required-status-check evaluation read. + # Guarding this job on the label name therefore let any non-gating label + # overwrite the commit run's real `Preflight` verdict with `skipped` (issue + # #5007). Running it unconditionally costs about a minute of ubuntu-slim time + # per label event and keeps the reported verdict truthful. Skipping the + # redundant work is the heavy jobs' job; POLICY in dev/ci/compute-changes.py + # drops every job a `labeled` event does not gate. # - # The two events it does skip belong to no pull request, so skipping them only - # changes what a fork's Actions tab shows. A scheduled run outside this - # repository: a fork with scheduled workflows enabled would otherwise run the - # nightly, and with no earlier scheduled run to diff against in the fork, - # `Detect changes` runs the whole nightly tier. And a push to a fork's default - # branch, which is a sync of commits the queue already tested. Everything else - # here needs `changes`, which needs this, so either run is skipped whole. + # The `if:` it does carry skips two runs that belong to no pull request, and + # with it the whole run, since every other job needs `changes`, which needs + # this: a scheduled run in a fork, which would otherwise run the whole nightly + # tier for want of an earlier run to diff against, and a push to a fork's + # default branch, which is a sync of commits the queue already tested. # --------------------------------------------------------------------------- preflight: name: Preflight @@ -360,23 +357,14 @@ jobs: # that enough (see the Merge Queue ruleset in .asf.yaml). git diff --name-only "$MQ_BASE_SHA"..."$MQ_HEAD_SHA" > changed_files.txt elif [[ "$REPOSITORY" != "apache/datafusion-comet" ]]; then - # A push to a branch of a contributor's fork (ci_fork.yml). There is - # no pull request to diff against, and the fork cannot know what - # upstream main is, so diff against the fork's own default branch. - # A fork that has not synced yet diffs against an older main, which - # selects more suites than the pull request needs, not fewer. - # - # Run everything the tier selects when there is nothing to go on: no - # default branch or merge base, or an empty diff. A branch whose - # commits are already on the fork's default branch, such as - # `git push fork main:my-fix`, diffs to nothing, and skipping every - # suite would report a green run for a change nobody built. - if git merge-base "origin/$DEFAULT_BRANCH" HEAD >/dev/null 2>&1; then - git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" > changed_files.txt - fi - if [[ ! -s changed_files.txt ]]; then - git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt - fi + # A push to a branch of a contributor's fork (ci_fork.yml). The + # fork cannot know what upstream main is, so diff against its own + # default branch; a fork that has not synced selects more suites, + # not fewer. With nothing to go on, run everything the tier selects: + # a branch already on the default branch (`git push fork main:fix`) + # diffs to nothing, and that would report green for an unbuilt change. + git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" > changed_files.txt 2>/dev/null || true + [[ -s changed_files.txt ]] || git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt else # push to main; the first push to a branch has an all-zero before sha if [[ "$PUSH_BEFORE" =~ ^0+$ ]]; then diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index 3d2c3bb72bb..80fe1d9b004 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -34,9 +34,9 @@ # # A fork's default branch is excluded because a push to it is a sync of upstream, # which the merge queue already tested. ci.yml still triggers on a push to `main` -# there, but its Preflight job skips that run. That means a fork's caches are -# never warmed, and a branch run is cold: about nine minutes slower than a warm -# one, measured on 2026-10-28. +# there, but its Preflight job skips that run. A fork's caches are therefore never +# warm; see "Pull requests from forks" in docs/source/contributor-guide/ci.md for +# what that costs. name: Comet CI (fork run) # No `concurrency` here. ci.yml declares its own group, which for a push to a @@ -47,9 +47,6 @@ on: branches-ignore: - main - asf-site - # Merge queue branches exist only in this repository; the guard below - # would skip them anyway. - - 'gh-readonly-queue/**' permissions: contents: read diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index e4f2bdaede8..319d58d3a36 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -52,7 +52,13 @@ concurrency: jobs: bridge: - if: github.repository == 'apache/datafusion-comet' && (github.event_name != 'workflow_run' || github.event.workflow_run.event == 'pull_request') + # 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 @@ -250,7 +256,5 @@ jobs: if (errors.length) core.setFailed(errors.join('\n')); } - if (repository === 'apache/datafusion-comet') { - if (context.eventName === 'workflow_run') await notify(); - else await update(); - } + if (context.eventName === 'workflow_run') await notify(); + else await update(); diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 4bf3e8e3552..a67fc5d9f73 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -# Guards eight CI invariants that are silent when broken: +# Guards nine CI invariants that are silent when broken: # # 1. Change-filter routing. dev/ci/compute-changes.py decides which heavy # jobs run. A file that a job depends on but that no filter lists makes @@ -78,6 +78,9 @@ # as an informational check. The caller, the bridge and ci.yml's own push # trigger have to agree, or the PR tier runs twice, or nowhere. # +# 9. Scheduled workflows. A fork that enables scheduled workflows runs them on +# its owner's minutes, so each one needs a guard on the repository. +# # Run from the repository root: python3 dev/ci/check-ci-config.py import importlib.util @@ -248,8 +251,26 @@ ALL_JOBS = QUEUE_TIER | NIGHTLY_TIER | SPARK_DEPRECATED | {"docs"} assert not QUEUE_TIER & NIGHTLY_TIER, "a job is queue or nightly, never both" -# The repository whose queue gates main; see UPSTREAM in compute-changes.py. +# The repository whose queue gates main; check_event_policy asserts this matches +# UPSTREAM in compute-changes.py. UPSTREAM = "apache/datafusion-comet" +FORK = "contributor/datafusion-comet" + + +def fork_pr(action="synchronize", label=None, labels=(), base="main", repository=UPSTREAM, head=FORK): + """A pull_request event for POLICY_CASES, from a fork of UPSTREAM by default.""" + event = { + "name": "pull_request", + "action": action, + "labels": list(labels), + "base": base, + "repository": repository, + "head_repository": head, + } + if label: + event["label"] = label + return event + POLICY_CASES = [ # A manual run may exercise anything. @@ -523,99 +544,34 @@ # contributor's fork runs it (ci_fork.yml) and fork_ci_status.yml mirrors it # as the informational `CI (fork)` check. Every opt-in label still works, # because a fork cannot see the labels on the pull request. + (fork_pr(), set()), ( - { - "name": "pull_request", - "action": "synchronize", - "labels": [], - "base": "main", - "repository": UPSTREAM, - "head_repository": "contributor/datafusion-comet", - }, - set(), - ), - ( - { - "name": "pull_request", - "action": "synchronize", - "labels": ["run-spark-4.1-tests", "run-delta-tests"], - "base": "main", - "repository": UPSTREAM, - "head_repository": "contributor/datafusion-comet", - }, + fork_pr(labels=["run-spark-4.1-tests", "run-delta-tests"]), {"spark_4_1", "spark_4_1_hive"} | DELTA_OPT_IN, ), # The label run adds the non-default Spark profiles to the fork's default # profile run, exactly as it does for any other pull request. ( - { - "name": "pull_request", - "action": "labeled", - "label": "run-all-spark-profiles", - "labels": ["run-all-spark-profiles"], - "base": "main", - "repository": UPSTREAM, - "head_repository": "contributor/datafusion-comet", - }, + fork_pr(action="labeled", label="run-all-spark-profiles", labels=["run-all-spark-profiles"]), {"build_linux_all_profiles"}, ), # The exceptions keep the PR tier upstream: a pull request from a branch of - # this repository (a committer's, or dependabot's), one that targets a + # this repository (a committer's, or dependabot's), and one that targets a # release branch, which has no queue or nightly to catch what a fork run # misses. - ( - { - "name": "pull_request", - "action": "synchronize", - "labels": [], - "base": "main", - "repository": UPSTREAM, - "head_repository": UPSTREAM, - }, - PR_TIER, - ), - ( - { - "name": "pull_request", - "action": "synchronize", - "labels": [], - "base": "branch-1.1", - "repository": UPSTREAM, - "head_repository": "contributor/datafusion-comet", - }, - QUEUE_TIER | NIGHTLY_TIER, - ), + (fork_pr(head=UPSTREAM), PR_TIER), + (fork_pr(base="branch-1.1"), QUEUE_TIER | NIGHTLY_TIER), # A push to a fork's branch is the contributor's pull request tier and # nothing more: not main's cache refresh or site deploy (`docs` publishes to # asf-site), and not the queue's or the nightly's suites. A push to the # fork's default branch, a sync, never reaches this table: Preflight skips it # (see check_fork_runs). - ({"name": "push", "repository": "contributor/datafusion-comet"}, PR_TIER), + ({"name": "push", "repository": FORK}, PR_TIER), # A pull request opened inside a fork leaves the PR tier to the branch push # that already ran it, so the same commit is not tested twice, but a label # still runs its suite there. - ( - { - "name": "pull_request", - "action": "synchronize", - "labels": [], - "base": "main", - "repository": "contributor/datafusion-comet", - "head_repository": "contributor/datafusion-comet", - }, - set(), - ), - ( - { - "name": "pull_request", - "action": "synchronize", - "labels": ["run-iceberg-tests"], - "base": "main", - "repository": "contributor/datafusion-comet", - "head_repository": "contributor/datafusion-comet", - }, - ICEBERG_OPT_IN, - ), + (fork_pr(repository=FORK, head=FORK), set()), + (fork_pr(repository=FORK, head=FORK, labels=["run-iceberg-tests"]), ICEBERG_OPT_IN), # Upstream, push stays the cache refresh and the site deploy. ({"name": "push", "repository": UPSTREAM}, {"docs", "build_linux", "build_macos"}), ] @@ -884,6 +840,11 @@ def check_change_filters(): def check_event_policy(): module = load_filters() failures = [] + if module.UPSTREAM != UPSTREAM: + failures.append( + f"UPSTREAM is {module.UPSTREAM!r} in compute-changes.py but {UPSTREAM!r} here; " + f"the fork cases below would test the wrong repository" + ) for event, expected in POLICY_CASES: actual = {job for job in module.POLICY if module.event_allows(job, event)} if actual != expected: @@ -1311,6 +1272,10 @@ def ci_triggers(): return triggers +# A job that calls ci.yml as a reusable workflow. +CALLS_CI = re.compile(r"^\s+uses:\s*\./\.github/workflows/ci\.yml\s*$", re.M) + + def permission_ceiling_failures(caller): """Failures for each ci.yml permission that `caller` does not grant. @@ -1368,7 +1333,7 @@ def check_label_runs_separate(): failures.append(f"{label_workflow} is missing; nothing runs on `labeled`") else: body = label_workflow.read_text(encoding="utf-8") - if not re.search(r"^\s+uses:\s*\./\.github/workflows/ci\.yml\s*$", body, re.M): + if not CALLS_CI.search(body): failures.append(f"{label_workflow} does not call ./.github/workflows/ci.yml") if not re.search(r"^\s+types:\s*\[\s*labeled\s*\]\s*$", body, re.M): failures.append( @@ -1732,20 +1697,11 @@ def check_fork_runs(): failures.append(f"{FORK_WORKFLOW} is missing; a fork pull request would run no PR tier anywhere") else: body = FORK_WORKFLOW.read_text(encoding="utf-8") - if not re.search(r"^\s+uses:\s*\./\.github/workflows/ci\.yml\s*$", body, re.M): + if not CALLS_CI.search(body): failures.append(f"{FORK_WORKFLOW} does not call ./.github/workflows/ci.yml") ignored = re.search(r"^ branches-ignore:\n((?: .*\n)+)", body, re.M) - entries = ( - [ - line.strip()[2:].strip() - for line in ignored.group(1).splitlines() - if line.strip().startswith("- ") - ] - if ignored - else [] - ) - for needed in ("main", "asf-site", "'gh-readonly-queue/**'"): - if needed not in entries: + for needed in ("main", "asf-site"): + if not ignored or f"- {needed}\n" not in ignored.group(1): failures.append(f"{FORK_WORKFLOW} must list {needed} under `push: branches-ignore`") conditions = " ".join(job_conditions(body)) if "github.repository != 'apache/datafusion-comet'" not in conditions: @@ -1781,6 +1737,11 @@ def check_fork_runs(): ) if "github.repository == 'apache/datafusion-comet'" not in body: failures.append(f"{BRIDGE_WORKFLOW} has no repository guard, so it would run in every fork") + if f"const WORKFLOW = '{FORK_WORKFLOW.name}';" not in body: + failures.append( + f"{BRIDGE_WORKFLOW} must read runs of {FORK_WORKFLOW.name}; any other name " + f"reports \"no fork CI\" on every pull request" + ) if re.search(r"uses:\s*actions/checkout@", body): failures.append( f"{BRIDGE_WORKFLOW} checks out code. It runs with `checks: write` on data from a " diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index c167f5fa990..d1ad802420f 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -691,9 +691,7 @@ def event_allows(job, event): # A push to a fork's branch is the contributor's own pull request tier. # The "push" tier is main's cache refresh and site deploy, and a fork # has no site to deploy. A push to a fork's default branch never gets - # here: Preflight skips it. It is a sync of commits the queue already - # tested, and the caches it would warm save a branch run only about nine - # of its fifty minutes (measured 2026-10-28: 59 minutes cold, 50 warm). + # here: Preflight skips it, as a sync of commits the queue already tested. if is_fork_push(event): return "pr" in tiers return "push" in tiers diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 317098daf79..278bde75ccf 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -153,11 +153,6 @@ test('upstream pushes and merge-queue completions cannot create fork checks', as assert.deepEqual((await execute({ event: 'workflow_run', notification: { event } })).writes, []); } }); -test('the bridge does nothing when copied into a fork', async () => { - const result = await execute({ owner: 'contributor' }); - assert.deepEqual(result.calls, []); - assert.deepEqual(result.writes, []); -}); test('only runs for the exact branch head on push/manual events are mirrored', async () => { const result = await execute({ runs: [ run(8, { head_sha: 'b'.repeat(40) }), @@ -293,24 +288,17 @@ test('no open PRs means no fork API calls', async () => { }); for (const defaultBranch of ['main', 'develop']) { -test('a PR from the fork default branch ' + defaultBranch + ' is told to use a branch, and no run is read', async () => { - const mainPr = { ...pr, head: { ...pr.head, ref: defaultBranch, repo: { ...pr.head.repo, default_branch: defaultBranch } } }; - const writes = []; - const github = { rest: { checks: {}, actions: {}, pulls: {} } }; - github.rest.checks.listForRef = async () => {}; - github.rest.checks.create = async (payload) => writes.push(payload); - github.rest.pulls.list = async () => ({ data: [mainPr], headers: {} }); - github.rest.actions.listWorkflowRuns = async () => assert.fail('a fork main has no run to read'); - github.rest.actions.getWorkflow = async () => assert.fail('a fork main has no workflow to read'); - github.paginate = async (fn) => (fn === github.rest.checks.listForRef ? [] : [mainPr]); - github.paginate.iterator = async function* () { yield { data: [mainPr] }; }; - await bridge(github, { - repo: { owner: 'apache', repo: 'datafusion-comet' }, eventName: 'schedule', payload: {}, - }, { info: () => {}, setFailed: (message) => assert.fail(message) }, Clock); - assert.equal(writes.length, 1); - assert.equal(writes[0].conclusion, 'neutral'); - assert.equal(writes[0].output.title, 'Open the pull request from a branch'); -}); + test('a PR from the fork default branch ' + defaultBranch + ' is told to use a branch', async () => { + const head = { + ...pr.head, ref: defaultBranch, repo: { ...pr.head.repo, default_branch: defaultBranch }, + }; + const mainPr = { ...pr, head }; + const { writes, calls } = await execute({ recent: [mainPr], slice: [mainPr] }); + assert.equal(calls.filter(([kind]) => kind === 'runs').length, 0, 'a default branch has no run to read'); + assert.equal(writes.length, 1); + assert.equal(writes[0].conclusion, 'neutral'); + assert.equal(writes[0].output.title, 'Open the pull request from a branch'); + }); } function group(github) { From f1a3040db7e1fea69164d8cc2330e6cab9c086f8 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 11:34:50 -0600 Subject: [PATCH 07/12] ci: diff a fork push against upstream main and describe the queue accurately A push to a fork's branch was diffed against the fork's own default branch. A fork that had not synced was behind upstream, so a branch cut from newer upstream code that reverts one of its files matched the fork's old copy, and the diff left the file out: the pull request showed it, the fork run did not test it. dev/ci/fork-push-changed-files.sh now fetches upstream main without the fork's credentials and diffs against that, which is the diff the pull request will have. It lists every file when the fetch fails, there is no merge base or the diff is empty. A test builds real repositories for the stale baseline revert, code followed by docs, a branch already on the fork's main, an unreachable upstream and an empty diff. The docs and workflow comments said the merge queue runs every suite. It runs the queue tier, with path filters; the nightly-only and label-only suites are not part of it. --- .github/workflows/README.md | 25 ++++-- .github/workflows/ci.yml | 19 ++-- .github/workflows/ci_fork.yml | 2 +- .github/workflows/fork_ci_status.yml | 2 +- dev/ci/check-ci-config.py | 7 +- dev/ci/compute-changes.py | 2 +- dev/ci/fork-push-changed-files.sh | 56 ++++++++++++ dev/ci/test-fork-push-changed-files.py | 119 +++++++++++++++++++++++++ docs/source/contributor-guide/ci.md | 13 +-- 9 files changed, 214 insertions(+), 31 deletions(-) create mode 100755 dev/ci/fork-push-changed-files.sh create mode 100755 dev/ci/test-fork-push-changed-files.py diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 01a7452ac29..88b5caaaabb 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -206,8 +206,10 @@ Nearly every pull request comes from a fork, and the PR tier is the largest share of the runner-hours CI spends (about 60% in the week to 2026-10-28, label runs included). A fork pull request therefore does not run the PR tier on ASF's runners. The contributor's fork runs it, and the merge -queue, which stays on ASF, runs everything on the merge result before anything -lands. The design follows `apache/datafusion` (#25591) and `apache/arrow-rs` +queue, which stays on ASF, runs the queue tier on the merge result before +anything lands. That is the PR tier plus Spark SQL on 4.1, Iceberg 1.11, macOS and +the other queue-tier jobs, with path filters still applied; the nightly-only and +label-only suites are not part of it. The design follows `apache/datafusion` (#25591) and `apache/arrow-rs` (#11788). Three files carry it: | File | Role | @@ -234,13 +236,18 @@ Things that follow from that, and are easy to break: - **A pull request opened inside a fork skips the PR tier,** because the branch push already ran it, and `is_fork_pull_request` covers both that and a fork pull request into `main` here. A `run-*` label still runs its suite. -- **A fork push diffs against the fork's default branch.** There is no pull - request to diff against, and a fork does not know what upstream `main` is, so - `Detect changes` uses `git diff origin/...`. A fork that - has not synced selects more suites, not fewer. With nothing to go on, or a diff - that is empty, it runs everything: a branch whose commits are already on the - default branch, such as `git push fork main:my-fix`, would otherwise skip every - suite and report a green run for a change nobody built. +- **A fork push is diffed against upstream main, as its pull request will be.** + 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 cut from newer upstream code that reverts one of its files matches the + fork's old copy and a diff against it omits the file. + `dev/ci/fork-push-changed-files.sh` fetches upstream `main` and diffs against + that. It lists every file when it has nothing to go on (the fetch fails, there + is no merge base, or the diff is empty), because a branch already on the fork's + default branch, such as `git push fork main:my-fix` before upstream has the + commits, would otherwise skip every suite and report a green run for a change + nobody built. `dev/ci/test-fork-push-changed-files.py` runs it against real + repositories, including the stale-baseline revert. - **Scheduled workflows stay upstream.** A fork that enables scheduled workflows would run them on its owner's minutes, and with no earlier scheduled run to diff against, a fork's nightly runs the whole nightly tier. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6a719accdb7..d598e1b6ff5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,8 +40,8 @@ # A pull request from a fork into main does not run the PR tier here. The # contributor's fork runs it (ci_fork.yml), and fork_ci_status.yml mirrors that # run onto the pull request as the informational `CI (fork)` check, which -# `Required Checks` does not depend on. The merge queue still runs everything on -# the merge result. Opt-in `run-*` suites, pull requests from a branch of this +# `Required Checks` does not depend on. The merge queue still runs the queue tier +# on the merge result. Opt-in `run-*` suites, pull requests from a branch of this # repository and pull requests to a release branch are unaffected. # # Spark 3.4 is deprecated and sits outside all three tiers: it runs only when @@ -218,6 +218,9 @@ jobs: - name: Check fork CI status bridge run: node --test dev/ci/test-fork-ci-status.cjs + - name: Check the changed files of a fork push + run: python3 dev/ci/test-fork-push-changed-files.py + - name: Check Iceberg shard inventory validation run: python3 dev/ci/test-iceberg-shards.py @@ -308,7 +311,6 @@ jobs: PR_BASE_REF: ${{ github.event.pull_request.base.ref }} PR_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} REPOSITORY: ${{ github.repository }} - DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} MQ_BASE_SHA: ${{ github.event.merge_group.base_sha }} @@ -357,14 +359,9 @@ jobs: # that enough (see the Merge Queue ruleset in .asf.yaml). git diff --name-only "$MQ_BASE_SHA"..."$MQ_HEAD_SHA" > changed_files.txt elif [[ "$REPOSITORY" != "apache/datafusion-comet" ]]; then - # A push to a branch of a contributor's fork (ci_fork.yml). The - # fork cannot know what upstream main is, so diff against its own - # default branch; a fork that has not synced selects more suites, - # not fewer. With nothing to go on, run everything the tier selects: - # a branch already on the default branch (`git push fork main:fix`) - # diffs to nothing, and that would report green for an unbuilt change. - git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" > changed_files.txt 2>/dev/null || true - [[ -s changed_files.txt ]] || git ls-tree -r --name-only "$PUSH_AFTER" > changed_files.txt + # A push to a branch of a contributor's fork (ci_fork.yml): diff it + # against upstream main, as its pull request will be. + bash dev/ci/fork-push-changed-files.sh "$PUSH_AFTER" > changed_files.txt else # push to main; the first push to a branch has an all-zero before sha if [[ "$PUSH_BEFORE" =~ ^0+$ ]]; then diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index 80fe1d9b004..6e2203d2298 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -21,7 +21,7 @@ # fork pull request into main (see `is_fork_pull_request` in # dev/ci/compute-changes.py), keeping only the preflight checks and the opt-in # `run-*` suites, and fork_ci_status.yml mirrors this run onto the pull request -# as the informational `CI (fork)` check. The merge queue runs every suite on +# as the informational `CI (fork)` check. The merge queue runs the queue tier on # the merge result, so a fork run is feedback and never a gate. # # This is a separate workflow, and not a wider `push` trigger on ci.yml, for the diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index 319d58d3a36..7fc33abbbe8 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -19,7 +19,7 @@ # 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 every suite on the merge result. +# 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. diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index a67fc5d9f73..18aa9134f41 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -1661,6 +1661,8 @@ def check_cache_save_scope(): FORK_WORKFLOW = WORKFLOWS / "ci_fork.yml" BRIDGE_WORKFLOW = WORKFLOWS / "fork_ci_status.yml" FORK_TEST = Path("dev/ci/test-fork-ci-status.cjs") +FORK_PUSH_SCRIPT = Path("dev/ci/fork-push-changed-files.sh") +FORK_PUSH_TEST = Path("dev/ci/test-fork-push-changed-files.py") def workflow_name(path): @@ -1690,8 +1692,9 @@ def check_fork_runs(): "(`github.ref_name != github.event.repository.default_branch`); " "otherwise every sync of a fork runs CI" ) - if FORK_TEST.name not in ci: - failures.append(f"ci.yml's Preflight does not run {FORK_TEST}, so the bridge goes untested") + for needed in (FORK_TEST.name, FORK_PUSH_SCRIPT.name, FORK_PUSH_TEST.name): + if needed not in ci: + failures.append(f"ci.yml does not use or run {needed}, so a fork push goes untested") if not FORK_WORKFLOW.exists(): failures.append(f"{FORK_WORKFLOW} is missing; a fork pull request would run no PR tier anywhere") diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index d1ad802420f..6c9d742f768 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -627,7 +627,7 @@ # The contributor's fork runs it instead, on a push to the PR branch (see # ci_fork.yml), and a bridge workflow (fork_ci_status.yml) mirrors that run onto # the pull request as the informational `CI (fork)` check. The merge queue still -# runs everything on the merge result, so the fork's verdict is feedback and +# runs the queue tier on the merge result, so the fork's verdict is feedback and # never a gate: a contributor controls their own workflows. A fork PR keeps # every opt-in label job, and a PR from a branch of this repository, or one that # targets a release branch, keeps the PR tier upstream. diff --git a/dev/ci/fork-push-changed-files.sh b/dev/ci/fork-push-changed-files.sh new file mode 100755 index 00000000000..33bf17dda6e --- /dev/null +++ b/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. +# +# Usage: fork-push-changed-files.sh +# UPSTREAM_URL overrides the repository to diff against, for the tests. + +set -euo pipefail + +head="${1:?usage: fork-push-changed-files.sh }" +upstream="${UPSTREAM_URL:-https://github.com/apache/datafusion-comet.git}" +ref="refs/remotes/upstream/main" + +changed="" +if git -c "http.https://github.com/.extraheader=" fetch --quiet --no-tags "$upstream" "+main:$ref" \ + && git merge-base "$ref" "$head" >/dev/null 2>&1; then + changed=$(git diff --name-only "$ref"..."$head") +fi + +if [[ -n "$changed" ]]; then + printf '%s\n' "$changed" +else + git ls-tree -r --name-only "$head" +fi diff --git a/dev/ci/test-fork-push-changed-files.py b/dev/ci/test-fork-push-changed-files.py new file mode 100755 index 00000000000..12eab44860c --- /dev/null +++ b/dev/ci/test-fork-push-changed-files.py @@ -0,0 +1,119 @@ +#!/usr/bin/env python3 +# +# 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. + +"""Regression tests for dev/ci/fork-push-changed-files.sh, on real git repositories.""" + +import os +from pathlib import Path +import subprocess +import tempfile +import unittest + +SCRIPT = Path(__file__).with_name("fork-push-changed-files.sh").resolve() +GIT_ENV = { + "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@example.com", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@example.com", + "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_SYSTEM": os.devnull, +} + + +def git(cwd, *args): + result = subprocess.run( + ["git", *args], cwd=cwd, check=True, capture_output=True, text=True, + env={**os.environ, **GIT_ENV}) + return result.stdout.strip() + + +def write(repo, path, text): + file = Path(repo, path) + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text(text) + + +def commit(repo, message, files): + for path, text in files.items(): + write(repo, path, text) + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", message) + return git(repo, "rev-parse", "HEAD") + + +class ForkPushChangedFiles(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + root = Path(self.tmp.name) + self.upstream = root / "upstream" + self.fork = root / "fork" + git(root, "init", "-q", "-b", "main", str(self.upstream)) + commit(self.upstream, "base", {"README.md": "a\n", "native/lib.rs": "1\n", "spark/Plan.scala": "1\n"}) + git(root, "clone", "-q", str(self.upstream), str(self.fork)) + git(self.fork, "checkout", "-q", "-b", "work") + + def changed(self, head=None, url=None): + head = head or git(self.fork, "rev-parse", "HEAD") + result = subprocess.run( + [str(SCRIPT), head], cwd=self.fork, check=True, capture_output=True, text=True, + env={**os.environ, **GIT_ENV, "UPSTREAM_URL": url or str(self.upstream)}) + return set(result.stdout.split()) + + def test_lists_what_the_branch_changed(self): + commit(self.fork, "native", {"native/lib.rs": "2\n"}) + self.assertEqual(self.changed(), {"native/lib.rs"}) + + def test_docs_only_branch_selects_no_code(self): + commit(self.fork, "docs", {"README.md": "b\n"}) + self.assertEqual(self.changed(), {"README.md"}) + + def test_revert_of_newer_upstream_code_is_kept_when_the_fork_is_stale(self): + # The fork's main stays at the base while upstream moves on and changes a + # native file. A branch cut from the newer upstream that reverts the file + # matches the fork's stale main, so a diff against it would omit the file. + commit(self.upstream, "upstream native change", {"native/lib.rs": "2\n"}) + # Fetch by URL so the fork's own origin/main stays at the base. + git(self.fork, "fetch", "-q", str(self.upstream), "main") + git(self.fork, "checkout", "-q", "-b", "revert", "FETCH_HEAD") + commit(self.fork, "revert", {"native/lib.rs": "1\n", "README.md": "b\n"}) + self.assertEqual(self.changed(), {"native/lib.rs", "README.md"}) + + def test_commits_made_on_the_fork_main_are_kept_when_pushed_to_a_branch(self): + # `git push fork main:my-fix`: the fork's main already holds the commits, + # so a diff against it is empty. Upstream does not have them. + commit(self.fork, "native", {"native/lib.rs": "2\n"}) + git(self.fork, "checkout", "-q", "main") + git(self.fork, "merge", "-q", "--ff-only", "work") + self.assertEqual(self.changed(), {"native/lib.rs"}) + + def test_code_then_docs_commits_keep_the_code_change(self): + commit(self.fork, "native", {"native/lib.rs": "2\n"}) + commit(self.fork, "docs", {"README.md": "b\n"}) + self.assertEqual(self.changed(), {"native/lib.rs", "README.md"}) + + def test_an_unreachable_upstream_runs_everything(self): + commit(self.fork, "docs", {"README.md": "b\n"}) + self.assertEqual( + self.changed(url=str(Path(self.tmp.name, "missing"))), + {"README.md", "native/lib.rs", "spark/Plan.scala"}) + + def test_a_branch_that_adds_nothing_runs_everything(self): + self.assertEqual(self.changed(), {"README.md", "native/lib.rs", "spark/Plan.scala"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index 8278dc12c39..e044ba54946 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -120,8 +120,10 @@ Nearly every pull request comes from a fork, and the PR tier is the largest shar the project spends on CI. So for a pull request from a fork into `main`, the PR tier runs on **your fork's** GitHub runners, not on the Apache Software Foundation's shared ones. What stays here is the preflight checks, `Required Checks`, and any `run-*` suite a maintainer opts the pull request -into. The merge queue, which also stays here, runs every suite on the merge result before anything -lands. This is the arrangement `apache/datafusion` and `apache/arrow-rs` use. +into. The merge queue, which also stays here, runs the queue tier on the merge result before anything +lands: the PR tier plus Spark SQL on 4.1, Iceberg 1.11, macOS and the other queue-tier suites in the +table above. The nightly-only suites and the opt-in `run-*` suites are not part of it, and path +filters still apply. This is the arrangement `apache/datafusion` and `apache/arrow-rs` use. To get it working: @@ -131,10 +133,9 @@ To get it working: the ones in your branch, so a branch cut before this arrangement existed runs nothing. 3. **Push your branch.** Each push to a branch other than `main` starts a `Comet CI (fork run)` in your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs - the PR tier for the files your branch changes relative to your fork's `main`: the Linux build, - lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites against Spark 4.1. If that - comparison finds nothing, because the commits are already on your fork's `main`, it runs the - whole tier. + the PR tier for the files your branch changes relative to upstream `main`, which is what the pull + request will show: the Linux build, lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites + against Spark 4.1. If that comparison cannot be made, or finds nothing, it runs the whole tier. On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is updated when the upstream run finishes and every 15 minutes after, so it can lag the fork. Read the details in From 40eb6cf2e9c5386359fe942923bad7f2dd89ae8e Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 13:21:50 -0600 Subject: [PATCH 08/12] ci: run fork pushes through one entry point and check the guards by evaluation - ci_fork.yml now runs every fork branch except the fork's default branch, `main` included. ci.yml also triggers on push to `main`, so its Preflight skips a fork push unless ci_fork.yml called it, which it tells by the caller's `github.workflow`. A branch called `main` in a fork whose default branch is another name is therefore tested once, by the workflow the bridge looks up, instead of by ci.yml directly where the bridge reported no run. - required_checks skips itself when Preflight is skipped, so a fork's schedule or sync starts no runner, and nightly_report no longer reads a skipped aggregator as a failure. - check-ci-config.py evaluates the `if:` of each root job of every scheduled workflow for a scheduled run in a fork, and Preflight and ci_fork.yml's job for the pushes and events that matter, instead of matching text. A guard that mentions the repository in the wrong clause no longer passes. - The contributor guide says how often the bridge rechecks a result that is already complete. --- .github/workflows/README.md | 29 +++-- .github/workflows/ci.yml | 35 ++++-- .github/workflows/ci_fork.yml | 16 +-- dev/ci/check-ci-config.py | 173 ++++++++++++++++++++++------ dev/ci/test-fork-ci-status.cjs | 22 +++- docs/source/contributor-guide/ci.md | 14 ++- 6 files changed, 215 insertions(+), 74 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 88b5caaaabb..a46c4163623 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -214,7 +214,7 @@ label-only suites are not part of it. The design follows `apache/datafusion` (#2 | File | Role | | -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `ci_fork.yml` | Fires on a push to any branch of a fork except `main` and calls `ci.yml`. Upstream its only job is skipped. | +| `ci_fork.yml` | Fires on a push to any branch of a fork except `asf-site` and calls `ci.yml`. Its job is skipped upstream and for a fork's default branch. | | `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | | `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | @@ -223,10 +223,15 @@ Things that follow from that, and are easy to break: - **The fork result is advisory.** `Required Checks` does not depend on `CI (fork)`, because a contributor controls the workflow that produced it. A green fork run that never ran the tests costs one queue slot, not a bad merge. -- **A push to a fork's `main` runs nothing.** It is a sync of commits the queue - already tested, and `ci.yml` triggers on it because it triggers on push to - `main`, so its `Preflight` skips the run, and with it every job. The same - skip covers a fork whose default branch is not `main`. The cost is that a +- **A push to a fork's default branch runs nothing.** It is a sync of commits + the queue already tested. `ci_fork.yml`'s job skips it, whatever the branch is + called. `ci.yml` also triggers on push to `main`, in a fork as upstream, so + its `Preflight` skips any fork push that did not come through `ci_fork.yml`, + which it tells apart by the caller's `github.workflow`, and `required_checks` + skips itself when `Preflight` does. That starts no runner for a sync, for a + fork's schedule, or for a second copy of a push. A branch called `main` in a + fork whose default is another branch is an ordinary branch and runs through + `ci_fork.yml`, so the bridge has one workflow to look up. The cost is that a fork never has warm caches, so each branch run rebuilds the native library: 59 minutes of wall time and 240 runner-minutes cold against 50 and 213 warm, measured 2026-10-28 on one fork. A sync that did warm them would cost about @@ -250,12 +255,14 @@ Things that follow from that, and are easy to break: repositories, including the stale-baseline revert. - **Scheduled workflows stay upstream.** A fork that enables scheduled workflows would run them on its owner's minutes, and with no earlier scheduled - run to diff against, a fork's nightly runs the whole nightly tier. - `ci.yml`'s `Preflight`, `miri.yml`, `stale.yml` and `codeql.yml` carry a - `github.repository == 'apache/datafusion-comet'` guard on a scheduled - event, and `check-ci-config.py` fails any workflow with a `schedule` trigger - that lacks one. A guard on `Preflight` skips the whole nightly, since every - other job needs `changes`, which needs it. + run to diff against, a fork's nightly runs the whole nightly tier. `ci.yml`'s + `Preflight`, `miri.yml`, `stale.yml`, `codeql.yml` and the bridge skip a + scheduled run outside this repository. Skipping `Preflight` skips the whole + nightly, since every other job needs `changes`, which needs it, and + `nightly_report` treats a skipped aggregator as nothing to report. + `check-ci-config.py` does not match text for this. It evaluates each root job's + `if:` for a scheduled run in a fork, and for the pushes and events above, and + fails when a job would run that should not or the other way round. - **`ci.yml` must not trigger on every push.** A skipped job still publishes a check run under its own name, so a push to a branch of this repository that lands on a pull request's commit would put a skipped `Required Checks` next to diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d598e1b6ff5..f1cb6f87e5f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,18 +140,26 @@ jobs: # redundant work is the heavy jobs' job; POLICY in dev/ci/compute-changes.py # drops every job a `labeled` event does not gate. # - # The `if:` it does carry skips two runs that belong to no pull request, and - # with it the whole run, since every other job needs `changes`, which needs - # this: a scheduled run in a fork, which would otherwise run the whole nightly - # tier for want of an earlier run to diff against, and a push to a fork's - # default branch, which is a sync of commits the queue already tested. + # The `if:` it does carry skips runs that belong to no pull request, and with + # them the whole run, since every other job needs `changes`, which needs this + # (`required_checks` skips itself when this is skipped, below): + # + # - A scheduled run in a fork, which would otherwise run the whole nightly + # tier for want of an earlier run to diff against. + # - A push to a fork that did not come through ci_fork.yml. ci.yml triggers + # on push to main, so it fires in a fork for a branch called `main` and for + # a sync of the fork's default branch. Neither is the contributor's pull + # request tier: ci_fork.yml runs every other branch, `main` included, + # except the fork's default branch, which is a sync of commits the queue + # already tested. The workflow name is what tells the two entry points + # apart, since a called workflow sees its caller's `github.workflow`. # --------------------------------------------------------------------------- preflight: name: Preflight if: >- (github.event_name != 'schedule' || github.repository == 'apache/datafusion-comet') && (github.event_name != 'push' || github.repository == 'apache/datafusion-comet' - || github.ref_name != github.event.repository.default_branch) + || github.workflow == 'Comet CI (fork run)') runs-on: ubuntu-slim steps: - uses: actions/checkout@v7 @@ -658,7 +666,9 @@ jobs: # nested name would block every docs-only change. Both hang rather than fail, # and a required context that never reports also locks `.asf.yaml` itself, # which then needs an INFRA ticket to unwedge. Aggregating into one flat job, - # whose name is published on every event, avoids the whole class of problem. + # whose name is published on every event that can reach the merge box, avoids + # the whole class of problem. The exception is a fork run that is no pull + # request's tier, which Preflight skips and this job follows. # # `if: always()` is what makes this work: without it the job inherits the # default `success()` and is itself skipped the moment any dependency fails. @@ -676,7 +686,10 @@ jobs: # --------------------------------------------------------------------------- required_checks: name: ${{ github.event.action == 'labeled' && 'Required Checks (label run)' || 'Required Checks' }} - if: always() + # `always()` so a failed or cancelled dependency still reaches it. It skips + # itself only when Preflight did, which is a fork run that is not a pull + # request's tier (see Preflight), so those start no runner at all. + if: always() && needs.preflight.result != 'skipped' # Reads only the `needs` context, so it needs no token access at all. permissions: {} needs: @@ -732,7 +745,11 @@ jobs: nightly_report: name: Nightly failure report needs: required_checks - if: always() && github.event_name == 'schedule' && needs.required_checks.result != 'success' + # `skipped` is not a failure here: the aggregator skips itself on a fork's + # scheduled run, and that run has nothing to report. + if: >- + always() && github.event_name == 'schedule' + && (needs.required_checks.result == 'failure' || needs.required_checks.result == 'cancelled') permissions: actions: read issues: write diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index 6e2203d2298..f82d8242b50 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -32,11 +32,14 @@ # that can be skipped is named after this workflow's own run, and upstream it is # always skipped. # -# A fork's default branch is excluded because a push to it is a sync of upstream, -# which the merge queue already tested. ci.yml still triggers on a push to `main` -# there, but its Preflight job skips that run. A fork's caches are therefore never -# warm; see "Pull requests from forks" in docs/source/contributor-guide/ci.md for -# what that costs. +# A fork's default branch is skipped by the job below, whatever it is called, +# because a push to it is a sync of upstream, which the merge queue already +# tested. A branch called `main` that is not the default is run like any other. +# ci.yml also triggers on a push to `main`, in a fork as upstream, but its +# Preflight job skips that run unless it came through this workflow, so a fork +# push is tested once and the bridge has one workflow to look up. A fork's caches +# are never warm; see "Pull requests from forks" in +# docs/source/contributor-guide/ci.md for what that costs. name: Comet CI (fork run) # No `concurrency` here. ci.yml declares its own group, which for a push to a @@ -45,7 +48,6 @@ name: Comet CI (fork run) on: push: branches-ignore: - - main - asf-site permissions: @@ -57,7 +59,7 @@ jobs: # Upstream, a push to any branch other than main is a pull request's own # business: the pull request run tests it, and a second run here would # test it twice. In a fork the default branch is skipped, whatever it is - # called; `branches-ignore` above can only name `main`. + # called; `branches-ignore` above can only name a fixed branch. if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch # The union of what ci.yml's jobs request, for the same reason as in # ci_label.yml: GitHub checks a called workflow's permissions against diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 18aa9134f41..aad48a2fc1b 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -83,6 +83,7 @@ # # Run from the repository root: python3 dev/ci/check-ci-config.py +import ast import importlib.util import re import subprocess @@ -1682,15 +1683,36 @@ def check_fork_runs(): "ci.yml must trigger on `push` to `branches: [main]` only; " "fork branches go through ci_fork.yml" ) - # Preflight is where a push to a fork's default branch, a sync of commits the - # queue already tested, is dropped: ci.yml triggers on push to main, so it - # fires there too, and every other job needs Preflight through `changes`. - preflight = re.search(r"^ preflight:\n(?: (?!runs-on)[^\n]*\n|\n)*", ci, re.M) - if not preflight or "github.event.repository.default_branch" not in preflight.group(0): + # Preflight is where a fork run that is not a pull request's tier is dropped, + # and with it the whole run, since every other job needs Preflight through + # `changes`. ci.yml triggers on push to main, so it fires in a fork for a + # branch called `main` and for a sync of the default branch; only a push that + # came through ci_fork.yml, whose `github.workflow` is its own name, is tested. + ci_name = workflow_name(WORKFLOWS / "ci.yml") + fork_name = workflow_name(FORK_WORKFLOW) + failures.extend( + condition_failures( + WORKFLOWS / "ci.yml", + "preflight", + [ + (actions_context("schedule", UPSTREAM, ci_name), True, "the upstream nightly"), + (actions_context("schedule", FORK, ci_name), False, "a scheduled run in a fork"), + (actions_context("push", UPSTREAM, ci_name), True, "a push to upstream main"), + (actions_context("push", FORK, ci_name), False, "ci.yml's own push trigger in a fork"), + (actions_context("push", FORK, fork_name), True, "a push through ci_fork.yml"), + (actions_context("pull_request", UPSTREAM, ci_name), True, "a pull request"), + (actions_context("pull_request", FORK, ci_name), True, "a pull request inside a fork"), + (actions_context("merge_group", UPSTREAM, ci_name), True, "the merge queue"), + (actions_context("workflow_dispatch", FORK, ci_name), True, "a dispatch in a fork"), + ], + ) + ) + # `needs.preflight.result` is how the aggregator follows Preflight. + aggregator = job_condition(job_blocks(WORKFLOWS / "ci.yml").get(AGGREGATOR_JOB, "")) + if not aggregator or "needs.preflight.result" not in aggregator: failures.append( - "ci.yml's Preflight must skip a push to a fork's default branch " - "(`github.ref_name != github.event.repository.default_branch`); " - "otherwise every sync of a fork runs CI" + f"ci.yml's {AGGREGATOR_JOB} must skip itself when Preflight does " + f"(`needs.preflight.result != 'skipped'`), or a fork's skipped run still starts a runner" ) for needed in (FORK_TEST.name, FORK_PUSH_SCRIPT.name, FORK_PUSH_TEST.name): if needed not in ci: @@ -1703,21 +1725,22 @@ def check_fork_runs(): if not CALLS_CI.search(body): failures.append(f"{FORK_WORKFLOW} does not call ./.github/workflows/ci.yml") ignored = re.search(r"^ branches-ignore:\n((?: .*\n)+)", body, re.M) - for needed in ("main", "asf-site"): - if not ignored or f"- {needed}\n" not in ignored.group(1): - failures.append(f"{FORK_WORKFLOW} must list {needed} under `push: branches-ignore`") - conditions = " ".join(job_conditions(body)) - if "github.repository != 'apache/datafusion-comet'" not in conditions: - failures.append( - f"{FORK_WORKFLOW} has no `if: github.repository != 'apache/datafusion-comet'` " - f"guard, so every upstream branch push would run the PR tier a second time" - ) - if "github.event.repository.default_branch" not in conditions: - failures.append( - f"{FORK_WORKFLOW} does not skip the fork's default branch, so a sync of a fork " - f"whose default branch is not `main` would start a run; `branches-ignore` " - f"can only name `main`" + if not ignored or "- asf-site\n" not in ignored.group(1): + failures.append(f"{FORK_WORKFLOW} must list asf-site under `push: branches-ignore`") + failures.extend( + condition_failures( + FORK_WORKFLOW, + "ci", + [ + (actions_context("push", UPSTREAM, fork_name, "feature"), False, "an upstream branch push"), + (actions_context("push", UPSTREAM, fork_name, "main"), False, "an upstream push to main"), + (actions_context("push", FORK, fork_name, "feature"), True, "a fork branch"), + (actions_context("push", FORK, fork_name, "main", "develop"), True, "a branch called main in a fork whose default is develop"), + (actions_context("push", FORK, fork_name, "main", "main"), False, "a sync of a fork's main"), + (actions_context("push", FORK, fork_name, "develop", "develop"), False, "a sync of a fork whose default is develop"), + ], ) + ) if re.search(r"^concurrency:", body, re.M): failures.append( f"{FORK_WORKFLOW} declares `concurrency`; ci.yml's own group is keyed on the " @@ -1758,33 +1781,107 @@ def check_fork_runs(): return not failures -# Scheduled workflows. GitHub runs `schedule` in any fork that has scheduled -# workflows enabled, on the fork owner's minutes. With no earlier scheduled run -# of its own to diff against, a fork's nightly runs the whole nightly tier, and -# a fork's Miri run costs twenty minutes a day. Every workflow with a `schedule` -# trigger therefore needs a job-level guard on the repository. +# Job-level `if:` conditions, evaluated for concrete events. A text match on a +# condition cannot tell the clause that guards an event from one that merely +# mentions the same repository, so the checks below run the expression instead. SCHEDULE_TRIGGER = re.compile(r"^ schedule:\s*$", re.M) -UPSTREAM_GUARD = re.compile(r"github\.repository\s*==\s*'apache/datafusion-comet'") # A job-level `if:` at four spaces of indent, with its continuation lines when it # is a folded block (`if: >-`). JOB_CONDITION = re.compile(r"^ if:[^\n]*\n(?: [^\n]*\n)*", re.M) +JOB_START = re.compile(r"^ ([A-Za-z0-9_-]+):\s*$", re.M) +CONTEXT_PROPERTY = re.compile(r"\b(?:github|inputs|needs)(?:\.[A-Za-z_][A-Za-z0-9_-]*)+") +EXPRESSION_NODES = ( + ast.Expression, ast.BoolOp, ast.And, ast.Or, ast.Compare, ast.Eq, ast.NotEq, + ast.Constant, ast.Call, ast.Name, ast.Load, +) + + +def job_blocks(path): + """{job: the text of its block} for the `jobs:` section of a workflow.""" + section = path.read_text(encoding="utf-8").split("\njobs:\n", 1)[1] + parts = JOB_START.split(section) + return dict(zip(parts[1::2], parts[2::2])) -def job_conditions(body): - return JOB_CONDITION.findall(body) +def job_condition(block): + """The text of a job's `if:` with the YAML scalar markers dropped, or None.""" + match = JOB_CONDITION.search(block) + if not match: + return None + text = match.group(0).split("if:", 1)[1] + return re.sub(r"^\s*[>|][-+]?\s*", "", text) + + +def evaluate_condition(text, context): + """Evaluate an `if:` made of ==, !=, &&, ||, parentheses, string literals and + context properties. A property absent from `context` is the empty string, as + in Actions. Anything else, a function call say, is an error: better to say a + condition cannot be checked than to pass it.""" + expression = " ".join(text.split()) + expression = re.sub(r"^\$\{\{\s*|\s*\}\}$", "", expression) + expression = CONTEXT_PROPERTY.sub(lambda m: f"_v('{m.group(0)}')", expression) + expression = expression.replace("&&", " and ").replace("||", " or ") + tree = ast.parse(expression, mode="eval") + for node in ast.walk(tree): + if not isinstance(node, EXPRESSION_NODES): + raise ValueError(f"unsupported syntax {type(node).__name__} in `{text.strip()}`") + if isinstance(node, ast.Call) and not ( + isinstance(node.func, ast.Name) and node.func.id == "_v" + ): + raise ValueError(f"unsupported call in `{text.strip()}`") + values = {"_v": lambda name: context.get(name, "")} + return bool(eval(compile(tree, "", "eval"), {"__builtins__": {}}, values)) # noqa: S307 + + +def actions_context(event_name, repository, workflow="", ref_name="feature", default_branch="main"): + return { + "github.event_name": event_name, + "github.repository": repository, + "github.workflow": workflow, + "github.ref_name": ref_name, + "github.event.repository.default_branch": default_branch, + } + +def condition_failures(path, job, cases): + """Failures for each (context, expected, description) a job's `if:` gets wrong. + A job with no `if:` always runs. + """ + block = job_blocks(path).get(job) + if block is None: + return [f"{path} has no job `{job}`"] + condition = job_condition(block) + failures = [] + for context, expected, description in cases: + try: + actual = True if condition is None else evaluate_condition(condition, context) + except (ValueError, SyntaxError) as error: + return [f"{path} job `{job}`: cannot check its `if:`: {error}"] + if actual != expected: + failures.append( + f"{path} job `{job}` {'runs' if actual else 'is skipped'} for {description}, " + f"and it should be {'run' if expected else 'skipped'}" + ) + return failures + + +# Scheduled workflows. GitHub runs `schedule` in any fork that has scheduled +# workflows enabled, on the fork owner's minutes. With no earlier scheduled run +# of its own to diff against, a fork's nightly runs the whole nightly tier, and +# a fork's Miri run costs twenty minutes a day. So every job of a workflow with +# a `schedule` trigger that does not wait on another must be skipped there. def check_scheduled_workflows_stay_upstream(): failures = [] for path in sorted(WORKFLOWS.glob("*.y*ml")): - body = path.read_text(encoding="utf-8") - if SCHEDULE_TRIGGER.search(body) and not any( - UPSTREAM_GUARD.search(c) for c in job_conditions(body) - ): - failures.append( - f"{path} has a `schedule` trigger but no job-level " - f"`if: ... github.repository == 'apache/datafusion-comet'`, so it " - f"runs in every fork that enables scheduled workflows" + if not SCHEDULE_TRIGGER.search(path.read_text(encoding="utf-8")): + continue + fork_schedule = actions_context("schedule", FORK, workflow=workflow_name(path)) + for job, block in job_blocks(path).items(): + if re.search(r"^ needs:", block, re.M): + continue + failures.extend( + condition_failures(path, job, [(fork_schedule, False, "a scheduled run in a fork")]) ) for failure in failures: print(f"scheduled workflows: {failure}") diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 278bde75ccf..84da5fea52e 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -67,7 +67,8 @@ const run = (id = 1, extra = {}) => ({ }); async function execute({ - event = 'schedule', checks = [], runs = [run()], workflowState = 'active', + event = 'schedule', checks = [], branch = 'feature', runs = [run(1, { head_branch: branch })], + workflowState = 'active', recent = [pr], slice = [pr], candidates = [pr], repo = 'datafusion-comet', owner = 'apache', runError, count = 1, notification = {}, checkErrors = {}, } = {}) { @@ -90,7 +91,7 @@ async function execute({ assert.equal(params.owner, 'contributor'); assert.equal(params.workflow_id, 'ci_fork.yml'); assert.equal(params.head_sha, sha); - assert.equal(params.branch, 'feature'); + assert.equal(params.branch, branch); if (runError) throw Object.assign(new Error('API failure'), { status: runError }); return { data: { workflow_runs: runs } }; }; @@ -107,7 +108,7 @@ async function execute({ return checks; } assert.equal(fn, github.rest.pulls.list); - assert.equal(params.head, 'contributor:feature'); + assert.equal(params.head, 'contributor:' + branch); return candidates; }; github.paginate.iterator = async function* () { @@ -116,7 +117,7 @@ async function execute({ await bridge(github, { repo: { owner, repo }, eventName: event, payload: { workflow_run: { - event: 'pull_request', head_sha: sha, head_branch: 'feature', + event: 'pull_request', head_sha: sha, head_branch: branch, head_repository: pr.head.repo, pull_requests: [], ...notification, } }, }, { info: () => {}, setFailed: (message) => failures.push(message) }, Clock); @@ -301,6 +302,19 @@ for (const defaultBranch of ['main', 'develop']) { }); } +test('a branch called main in a fork whose default branch is not main is mirrored', async () => { + // ci.yml triggers on push to a branch called main as well, but it skips that run + // unless ci_fork.yml called it, so ci_fork.yml is the one workflow to look up. + const head = { ...pr.head, ref: 'main', repo: { ...pr.head.repo, default_branch: 'develop' } }; + const mainPr = { ...pr, head }; + const { writes } = await execute({ + branch: 'main', recent: [mainPr], slice: [mainPr], candidates: [mainPr], + }); + assert.equal(writes.length, 1); + assert.equal(writes[0].conclusion, 'success'); + assert.equal(writes[0].details_url, run().html_url); +}); + function group(github) { return ciConcurrency[1].replace(/\$\{\{\s*(.*?)\s*\}\}/g, (_, expression) => new Function('github', 'return (' + expression + ');')(github)); diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index e044ba54946..f925e8976c5 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -131,15 +131,19 @@ To get it working: fork's **Actions** tab and choose **I understand my workflows, go ahead and enable them**. 2. **Sync your fork and rebase your branch onto `main`.** The workflows that run on your fork are the ones in your branch, so a branch cut before this arrangement existed runs nothing. -3. **Push your branch.** Each push to a branch other than `main` starts a `Comet CI (fork run)` in +3. **Push your branch.** Each push to a branch other than your fork's default branch starts a `Comet CI (fork run)` in your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs the PR tier for the files your branch changes relative to upstream `main`, which is what the pull request will show: the Linux build, lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites against Spark 4.1. If that comparison cannot be made, or finds nothing, it runs the whole tier. -On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is updated when -the upstream run finishes and every 15 minutes after, so it can lag the fork. Read the details in -your fork and re-run failed jobs there. If the check says **Enable CI on your fork** or **No fork CI +On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is first +written when the upstream run finishes. After that, a pull request updated in the last six hours +whose check is still running or has no run yet is rechecked every 15 minutes. A check that already +shows a result is rechecked only by a sweep that takes 25 open pull requests every 15 minutes, which +is about two hours to go round 200 of them. So after you re-run a failed job in your fork, the pull +request can keep showing the old result for hours. Your fork's **Actions** tab is the source of truth: +read the details and re-run failed jobs there. If the check says **Enable CI on your fork** or **No fork CI run for this commit**, no run was found for the pull request's head commit: Actions is disabled, the branch does not include the fork workflows, or the run has not appeared yet. @@ -159,7 +163,7 @@ Some things to know: from it gets no CI before the merge queue. `CI (fork)` says so. - **Scheduled runs stay upstream.** The nightly run, Miri, the stale-PR closer and the weekly CodeQL scan run in `apache/datafusion-comet` only. If your fork has scheduled workflows enabled they - skip. Miri can still be run on your fork by dispatching it. + start no job. Miri can still be run on your fork by dispatching it. - **Opt-in suites still run here.** A fork cannot see the `run-*` labels on your pull request, so those suites, and the `labeled` runs that start when a maintainer applies one, are unchanged. See [Opting a pull request into a suite the PR tier skips](#opting-a-pull-request-into-a-suite-the-pr-tier-skips). From 86f1baedfc874f55dd9d1af27565c2127fd6e8f5 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 14:20:23 -0600 Subject: [PATCH 09/12] ci: cancel superseded fork runs by exact branch and compare names as Actions does GitHub Actions compares strings, branch names and concurrency group names without regard to case, and a fork can hold both `feature` and `Feature`. - A fork push no longer shares a concurrency group per branch, which would let a push to one cancel the other's run, and no expression can make the two keys differ. ci_fork.yml has a `supersede` job that cancels the older queued and running runs of exactly this branch instead, comparing with case. - The bridge treats a case variant of the default branch, such as `Main` with default `main`, as the default branch, because ci_fork.yml's job guard skips it and there is no run to find. - check-ci-config.py's condition evaluator compares strings without regard to case, checks both ci_fork.yml jobs against the same table including the case variant, and checks that the push trigger admits `main`, `Main` and ordinary branches, so restoring a `main` exclusion fails. - The contributor guide says the default branch skips the Comet PR tier, not that nothing runs on `main`. --- .github/workflows/README.md | 18 +++++-- .github/workflows/ci.yml | 8 ++-- .github/workflows/ci_fork.yml | 50 +++++++++++++++++-- .github/workflows/fork_ci_status.yml | 4 +- dev/ci/check-ci-config.py | 72 ++++++++++++++++++++-------- dev/ci/test-fork-ci-status.cjs | 68 ++++++++++++++++++++++++-- docs/source/contributor-guide/ci.md | 10 ++-- 7 files changed, 188 insertions(+), 42 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index a46c4163623..dd1294d44e7 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -212,11 +212,11 @@ the other queue-tier jobs, with path filters still applied; the nightly-only and label-only suites are not part of it. The design follows `apache/datafusion` (#25591) and `apache/arrow-rs` (#11788). Three files carry it: -| File | Role | -| -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `ci_fork.yml` | Fires on a push to any branch of a fork except `asf-site` and calls `ci.yml`. Its job is skipped upstream and for a fork's default branch. | -| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | -| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | +| File | Role | +| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `ci_fork.yml` | Fires on a push to any branch of a fork except `asf-site` and calls `ci.yml`, after cancelling the branch's older runs. Its jobs are skipped upstream and for a fork's default branch. | +| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | +| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | Things that follow from that, and are easy to break: @@ -238,6 +238,14 @@ Things that follow from that, and are easy to break: as much as it saved on its first run. The bridge answers a pull request from a fork's default branch with a neutral "open the pull request from a branch", because nothing tests it. +- **Branch names compare without regard to case in Actions,** in `==`, `!=` and + concurrency group names alike, and a fork can hold both `feature` and + `Feature`. So a fork push has no shared concurrency group: it is keyed on its + commit, and `ci_fork.yml`'s `supersede` job cancels the older runs of exactly + that branch, which it can compare with case. `Main` in a fork whose default + branch is `main` is skipped like `main`, and the bridge treats it the same way. + `check-ci-config.py` evaluates conditions with the same case-insensitive + comparison, and checks that the push trigger admits `main`. - **A pull request opened inside a fork skips the PR tier,** because the branch push already ran it, and `is_fork_pull_request` covers both that and a fork pull request into `main` here. A `run-*` label still runs its suite. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1cb6f87e5f..f641dd265a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -83,10 +83,12 @@ name: Comet CI # commit's push run. A pull request is keyed on its number, and not on its # branch name: nearly every pull request comes from a fork, and two forks' # branches can share a name, which would have one cancel the other's run. A push -# to a fork (ci_fork.yml) is keyed on its branch instead of its commit, so the -# next push to the branch cancels the run in flight. +# to a fork (ci_fork.yml) is keyed on its commit, like any push: a newer push to +# the branch cancels the run in flight through ci_fork.yml's `supersede` job +# instead, because group names compare without regard to case and a fork can +# hold both `feature` and `Feature`. concurrency: - group: ${{ github.repository }}-${{ github.event.pull_request.number || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.ref) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} + group: ${{ github.repository }}-${{ github.event.pull_request.number || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} cancel-in-progress: true # No `labeled` here: ci_label.yml handles that event and calls this file diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index f82d8242b50..e0d92598fa4 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -42,9 +42,9 @@ # docs/source/contributor-guide/ci.md for what that costs. name: Comet CI (fork run) -# No `concurrency` here. ci.yml declares its own group, which for a push to a -# fork is keyed on the branch, so the next push to the branch supersedes this -# one. The same group on the caller as well would deadlock the two. +# No `concurrency` here, and none for a fork push in ci.yml either: see the +# `supersede` job. A group on the caller as well as in ci.yml would deadlock +# the two. on: push: branches-ignore: @@ -54,13 +54,53 @@ permissions: contents: read jobs: - ci: - name: Fork run + # A newer push to a branch supersedes the run in flight, as it does for a pull + # request upstream, but not through a concurrency group. Group names compare + # without regard to case, and a fork can hold both `feature` and `Feature`, + # which are two branches: keying a group on the branch would let a push to one + # cancel the other's run, and no expression can make the two keys differ. So the + # runs get no shared group, and this job cancels the older runs of exactly this + # branch, which it can compare with case. + supersede: + name: Cancel superseded runs # Upstream, a push to any branch other than main is a pull request's own # business: the pull request run tests it, and a second run here would # test it twice. In a fork the default branch is skipped, whatever it is # called; `branches-ignore` above can only name a fixed branch. if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch + runs-on: ubuntu-slim + # Failing to cancel only leaves an older run to finish, so it never blocks the run. + continue-on-error: true + permissions: + actions: write + steps: + - uses: actions/github-script@v9 + with: + script: | + const { owner, repo } = context.repo; + const branch = context.ref.replace(/^refs\/heads\//, ''); + for (const status of ['queued', 'in_progress']) { + const runs = await github.paginate(github.rest.actions.listWorkflowRuns, { + owner, repo, workflow_id: 'ci_fork.yml', event: 'push', status, per_page: 100, + }); + for (const run of runs) { + // The API's branch filter ignores case, so filter here, where `feature` + // and `Feature` stay two branches. + if (run.head_branch !== branch || run.run_number >= context.runNumber) continue; + try { + await github.rest.actions.cancelWorkflowRun({ owner, repo, run_id: run.id }); + core.info('Cancelled run ' + run.run_number + ' of ' + branch); + } catch (error) { + // 409: it finished between the listing and the cancel. + if (error.status !== 409) throw error; + } + } + } + + ci: + name: Fork run + needs: supersede + if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch # The union of what ci.yml's jobs request, for the same reason as in # ci_label.yml: GitHub checks a called workflow's permissions against # this ceiling for every job when the run starts, including the ones this diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index 7fc33abbbe8..a7975e89c73 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -116,7 +116,9 @@ jobs: // 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; + // Actions compares branch names without regard to case, so `Main` is + // the default branch `main` to ci_fork.yml's job guard. Compare the same way. + const fromMain = pr.head.ref.toLowerCase() === String(pr.head.repo.default_branch).toLowerCase(); const data = fromMain ? null : await optional(() => github.rest.actions.listWorkflowRuns({ ...fork, workflow_id: WORKFLOW, head_sha: sha, branch: pr.head.ref, per_page: 100, })); diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index aad48a2fc1b..2e646242f86 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -1724,23 +1724,34 @@ def check_fork_runs(): body = FORK_WORKFLOW.read_text(encoding="utf-8") if not CALLS_CI.search(body): failures.append(f"{FORK_WORKFLOW} does not call ./.github/workflows/ci.yml") + # The trigger comes first: a branch it ignores never reaches a job's `if:`, + # so a condition table alone cannot see an exclusion that is too wide. ignored = re.search(r"^ branches-ignore:\n((?: .*\n)+)", body, re.M) - if not ignored or "- asf-site\n" not in ignored.group(1): - failures.append(f"{FORK_WORKFLOW} must list asf-site under `push: branches-ignore`") - failures.extend( - condition_failures( - FORK_WORKFLOW, - "ci", - [ - (actions_context("push", UPSTREAM, fork_name, "feature"), False, "an upstream branch push"), - (actions_context("push", UPSTREAM, fork_name, "main"), False, "an upstream push to main"), - (actions_context("push", FORK, fork_name, "feature"), True, "a fork branch"), - (actions_context("push", FORK, fork_name, "main", "develop"), True, "a branch called main in a fork whose default is develop"), - (actions_context("push", FORK, fork_name, "main", "main"), False, "a sync of a fork's main"), - (actions_context("push", FORK, fork_name, "develop", "develop"), False, "a sync of a fork whose default is develop"), - ], - ) - ) + patterns = re.findall(r"^\s+- (\S+)\s*$", ignored.group(1), re.M) if ignored else [] + patterns = [p.strip("'\"") for p in patterns] + compute = load_filters() + for branch, admitted in ( + ("feature", True), ("main", True), ("Main", True), ("release/1.1", True), ("asf-site", False), + ): + actual = not any(re.match(compute.glob_to_regex(p), branch) for p in patterns) + if actual != admitted: + failures.append( + f"{FORK_WORKFLOW}'s `push: branches-ignore` {patterns} " + f"{'admits' if actual else 'ignores'} a push to `{branch}`, and it should " + f"{'ignore' if actual else 'admit'} it; a fork's `main` is an ordinary branch " + f"unless it is the default, which the jobs' `if:` decides" + ) + cases = [ + (actions_context("push", UPSTREAM, fork_name, "feature"), False, "an upstream branch push"), + (actions_context("push", UPSTREAM, fork_name, "main"), False, "an upstream push to main"), + (actions_context("push", FORK, fork_name, "feature"), True, "a fork branch"), + (actions_context("push", FORK, fork_name, "main", "develop"), True, "a branch called main in a fork whose default is develop"), + (actions_context("push", FORK, fork_name, "main", "main"), False, "a sync of a fork's main"), + (actions_context("push", FORK, fork_name, "develop", "develop"), False, "a sync of a fork whose default is develop"), + (actions_context("push", FORK, fork_name, "Main", "main"), False, "a case variant of the default branch, which Actions compares as equal"), + ] + for job in ("supersede", "ci"): + failures.extend(condition_failures(FORK_WORKFLOW, job, cases)) if re.search(r"^concurrency:", body, re.M): failures.append( f"{FORK_WORKFLOW} declares `concurrency`; ci.yml's own group is keyed on the " @@ -1812,11 +1823,23 @@ def job_condition(block): return re.sub(r"^\s*[>|][-+]?\s*", "", text) +class _Comparisons(ast.NodeTransformer): + """Rewrite `a == b` and `a != b` into calls that ignore case, as Actions does.""" + + def visit_Compare(self, node): + self.generic_visit(node) + name = "_eq" if isinstance(node.ops[0], ast.Eq) else "_ne" + call = ast.Call(func=ast.Name(id=name, ctx=ast.Load()), + args=[node.left, node.comparators[0]], keywords=[]) + return ast.copy_location(call, node) + + def evaluate_condition(text, context): """Evaluate an `if:` made of ==, !=, &&, ||, parentheses, string literals and - context properties. A property absent from `context` is the empty string, as - in Actions. Anything else, a function call say, is an error: better to say a - condition cannot be checked than to pass it.""" + context properties. A property absent from `context` is the empty string, and + strings compare without regard to case, as in Actions: `Main` equals `main`. + Anything else, a function call say, is an error: better to say a condition + cannot be checked than to pass it.""" expression = " ".join(text.split()) expression = re.sub(r"^\$\{\{\s*|\s*\}\}$", "", expression) expression = CONTEXT_PROPERTY.sub(lambda m: f"_v('{m.group(0)}')", expression) @@ -1829,7 +1852,16 @@ def evaluate_condition(text, context): isinstance(node.func, ast.Name) and node.func.id == "_v" ): raise ValueError(f"unsupported call in `{text.strip()}`") - values = {"_v": lambda name: context.get(name, "")} + if isinstance(node, ast.Compare) and ( + len(node.ops) != 1 or not isinstance(node.ops[0], (ast.Eq, ast.NotEq)) + ): + raise ValueError(f"unsupported comparison in `{text.strip()}`") + tree = ast.fix_missing_locations(_Comparisons().visit(tree)) + values = { + "_v": lambda name: context.get(name, ""), + "_eq": lambda a, b: str(a).lower() == str(b).lower(), + "_ne": lambda a, b: str(a).lower() != str(b).lower(), + } return bool(eval(compile(tree, "", "eval"), {"__builtins__": {}}, values)) # noqa: S307 diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 84da5fea52e..498d3e988c8 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -302,6 +302,15 @@ for (const defaultBranch of ['main', 'develop']) { }); } +test('a PR from a case variant of the default branch is told to use a branch', async () => { + // Actions compares branch names without regard to case, so ci_fork.yml skips Main + // in a fork whose default branch is main, and there is no run to find. + const head = { ...pr.head, ref: 'Main' }; + const variantPr = { ...pr, head }; + const { writes, calls } = await execute({ recent: [variantPr], slice: [variantPr] }); + assert.equal(calls.filter(([kind]) => kind === 'runs').length, 0); + assert.equal(writes[0].output.title, 'Open the pull request from a branch'); +}); test('a branch called main in a fork whose default branch is not main is mirrored', async () => { // ci.yml triggers on push to a branch called main as well, but it skips that run // unless ci_fork.yml called it, so ci_fork.yml is the one workflow to look up. @@ -328,10 +337,14 @@ const pullRequest = (number, extra = {}) => ({ ...forkPush, repository: 'apache/datafusion-comet', event_name: 'pull_request', event: { pull_request: { number } }, ref: 'refs/pull/' + number + '/merge', ...extra, }); -test('successive fork pushes share a cancellation group, separate branches do not', () => { - assert.equal(ciConcurrency[2], 'true'); - assert.equal(group(forkPush), group({ ...forkPush, sha: 'second' })); - assert.notEqual(group(forkPush), group({ ...forkPush, ref: 'refs/heads/other' })); +test('fork pushes get a group each, so none is cancelled by a case variant of its branch', () => { + // Group names compare without regard to case, so a group keyed on the branch would + // let a push to `Feature` cancel `feature`. ci_fork.yml's supersede job cancels + // older runs of exactly one branch instead. + assert.notEqual(group(forkPush), group({ ...forkPush, sha: 'second' })); + assert.equal( + group({ ...forkPush, ref: 'refs/heads/feature' }).toLowerCase(), + group({ ...forkPush, ref: 'refs/heads/Feature' }).toLowerCase()); }); test('upstream pushes and queue runs stay separate per commit', () => { for (const event_name of ['push', 'merge_group']) { @@ -355,3 +368,50 @@ test('a label run is not cancelled by the commit run, or by another label', () = assert.notEqual(group(labeled('run-iceberg-tests')), group(pullRequest(7))); assert.notEqual(group(labeled('run-iceberg-tests')), group(labeled('run-delta-tests'))); }); + +// The supersede job in ci_fork.yml cancels older runs of exactly one branch. +const supersedeScript = scriptOf(workflowText('ci_fork.yml')); +const supersede = new AsyncFunction('github', 'context', 'core', supersedeScript); +async function runSupersede({ runs, cancelError, branch = 'feature', runNumber = 5 }) { + const cancelled = []; + const github = { rest: { actions: {} } }; + github.rest.actions.listWorkflowRuns = async () => {}; + github.rest.actions.cancelWorkflowRun = async ({ run_id }) => { + if (cancelError) throw Object.assign(new Error('conflict'), { status: cancelError }); + cancelled.push(run_id); + }; + let calls = 0; + github.paginate = async (fn, params) => { + assert.equal(fn, github.rest.actions.listWorkflowRuns); + assert.equal(params.workflow_id, 'ci_fork.yml'); + // One listing per status, so each run appears once. + return runs.filter((r) => r.status === params.status); + }; + await supersede(github, { repo: { owner: 'contributor', repo: 'datafusion-comet' }, + ref: 'refs/heads/' + branch, runNumber }, { info: () => { calls += 1; } }); + return cancelled; +} +const branchRun = (id, extra = {}) => ({ + id, run_number: id, head_branch: 'feature', status: 'in_progress', ...extra, +}); +test('a push cancels the older queued and running runs of its branch', async () => { + const cancelled = await runSupersede({ runs: [ + branchRun(2), branchRun(3, { status: 'queued' }), branchRun(4), + ] }); + assert.deepEqual(cancelled.sort(), [2, 3, 4]); +}); +test('a push does not cancel itself, a newer run or a rerun of itself', async () => { + const cancelled = await runSupersede({ runs: [branchRun(5), branchRun(6), branchRun(7)] }); + assert.deepEqual(cancelled, []); +}); +test('a push cancels no run of a branch that differs only by case', async () => { + const cancelled = await runSupersede({ runs: [ + branchRun(2, { head_branch: 'Feature' }), branchRun(3, { head_branch: 'other' }), branchRun(4), + ] }); + assert.deepEqual(cancelled, [4]); +}); +test('a run that finished before it was cancelled is not an error', async () => { + const cancelled = await runSupersede({ runs: [branchRun(2)], cancelError: 409 }); + assert.deepEqual(cancelled, []); + await assert.rejects(runSupersede({ runs: [branchRun(2)], cancelError: 500 }), /conflict/); +}); diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index f925e8976c5..1bae8651f82 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -154,13 +154,15 @@ Some things to know: queue is the gate, and it tests your change merged with `main` and everything queued ahead of it, where the fork run tests your branch head alone. - **Runs are cold.** Caches are scoped to a branch or to the repository's default branch, and a - push to your fork's `main` runs nothing, because it is a sync of commits the merge queue already - tested. So the Cargo and Maven caches are never warm on a fork, and a run rebuilds the native + push to your fork's default branch, usually `main`, does not run the Comet PR tier, because it is a + sync of commits the merge queue already tested. So the Cargo and Maven caches are never warm on a fork, and a run rebuilds the native library: a run of the PR tier on a fork took about an hour of wall time and 240 runner-minutes, of which the native build alone was 23 minutes. The same run with warm caches took 50 minutes and 213 runner-minutes. -- **Open the pull request from a branch.** Nothing runs on your fork's `main`, so a pull request - from it gets no CI before the merge queue. `CI (fork)` says so. +- **Open the pull request from a branch.** The Comet PR tier does not run on your fork's default + branch, usually `main`, so a pull request from it gets no CI before the merge queue. `CI (fork)` + says so. A branch that differs from the default only in capitalization, such as `Main`, counts as + the default branch, because GitHub Actions compares branch names without regard to case. - **Scheduled runs stay upstream.** The nightly run, Miri, the stale-PR closer and the weekly CodeQL scan run in `apache/datafusion-comet` only. If your fork has scheduled workflows enabled they start no job. Miri can still be run on your fork by dispatching it. From 876d95938aa165c0f13b254dca9a16bdd5bc3185 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 14:33:47 -0600 Subject: [PATCH 10/12] ci: tighten the bridge permissions and select nothing for an empty fork diff - fork_ci_status.yml starts with `permissions: {}` and the bridge job asks for `checks: write` and `pull-requests: read` only. `actions: read` was never used: every call to the fork's runs and workflows reads public data of another repository. check-ci-config.py pins both. - A fork push whose diff against upstream main is empty, with a merge base, selects nothing: the pull request will show no change. Only a failed fetch or a missing merge base lists every file. The earlier example for the empty case was wrong, since commits only the fork's main has are listed. - The bridge matches only a `push` run of ci_fork.yml, which has no other trigger, and its two manual-run test cases go. - ci.yml's profile table and the policy cases cover a fork pull request with `run-all-spark-profiles`: only the other profiles run here, and the fork's run covers the default profile and the lints. --- .github/workflows/README.md | 13 +++++++------ .github/workflows/ci.yml | 4 ++++ .github/workflows/fork_ci_status.yml | 17 ++++++++++------- dev/ci/check-ci-config.py | 21 +++++++++++++++++---- dev/ci/fork-push-changed-files.sh | 17 ++++++----------- dev/ci/test-fork-ci-status.cjs | 11 ++--------- dev/ci/test-fork-push-changed-files.py | 10 ++++++---- docs/source/contributor-guide/ci.md | 6 ++++-- 8 files changed, 56 insertions(+), 43 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index dd1294d44e7..d5e3225fbc0 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -255,12 +255,13 @@ Things that follow from that, and are easy to break: branch cut from newer upstream code that reverts one of its files matches the fork's old copy and a diff against it omits the file. `dev/ci/fork-push-changed-files.sh` fetches upstream `main` and diffs against - that. It lists every file when it has nothing to go on (the fetch fails, there - is no merge base, or the diff is empty), because a branch already on the fork's - default branch, such as `git push fork main:my-fix` before upstream has the - commits, would otherwise skip every suite and report a green run for a change - nobody built. `dev/ci/test-fork-push-changed-files.py` runs it against real - repositories, including the stale-baseline revert. + that. It lists every file when it has nothing to go on (the fetch fails or + there is no merge base), because selecting no suites then would report a green + run for a change nobody could see. An empty diff with a merge base is a branch + that adds nothing to upstream, which selects nothing. + `dev/ci/test-fork-push-changed-files.py` runs it against real repositories, + including the stale-baseline revert and a branch whose commits only the fork's + `main` has. - **Scheduled workflows stay upstream.** A fork that enables scheduled workflows would run them on its owner's minutes, and with no earlier scheduled run to diff against, a fork's nightly runs the whole nightly tier. `ci.yml`'s diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f641dd265a0..5472137c619 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -409,6 +409,10 @@ jobs: # pull request linux, full -> profiles: pr # ... with the label linux, full, all -> profiles: all # ... to branch-N.M linux, full, all -> profiles: all + # ... from a fork (nothing) -> no call: the fork runs the tier + # ... from a fork, with the label + # all -> profiles: nightly, the ones the + # fork's default-profile run skips # `labeled` run all -> profiles: nightly # merge queue linux, full -> profiles: pr # nightly all -> profiles: nightly diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index a7975e89c73..71bcbdb0014 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -39,10 +39,10 @@ on: - cron: '7,22,37,52 * * * *' workflow_dispatch: -permissions: - actions: read - checks: write - pull-requests: read +# No scopes by default: the job below asks for the two it needs. Every call to the +# fork's runs and workflows reads public data of another repository, which a token +# does not need a scope for. +permissions: {} # Like DataFusion, group notifications separately from scheduled/manual sweeps. # workflow_run identifies the fork and branch rather than the PR number. @@ -60,6 +60,9 @@ jobs: || (github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.head_repository.full_name != github.repository)) name: Mirror the fork run + permissions: + checks: write + pull-requests: read runs-on: ubuntu-slim timeout-minutes: 10 steps: @@ -122,11 +125,11 @@ jobs: 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. + // Ignore PR merge-ref runs. ci_fork.yml runs on push only, and 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) + item.event === 'push' ).sort((a, b) => b.id - a.id)[0]; let payload; if (run) { diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 2e646242f86..46e88c117dd 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -550,8 +550,11 @@ def fork_pr(action="synchronize", label=None, labels=(), base="main", repository fork_pr(labels=["run-spark-4.1-tests", "run-delta-tests"]), {"spark_4_1", "spark_4_1_hive"} | DELTA_OPT_IN, ), - # The label run adds the non-default Spark profiles to the fork's default - # profile run, exactly as it does for any other pull request. + # The label brings the non-default Spark profiles forward here, while the + # fork's run covers the default profile and the lints, so between them every + # profile runs. Only the profiles are on, which ci.yml turns into + # `profiles: nightly`; the PR tier's default-profile row is the fork's. + (fork_pr(labels=["run-all-spark-profiles"]), {"build_linux_all_profiles"}), ( fork_pr(action="labeled", label="run-all-spark-profiles", labels=["run-all-spark-profiles"]), {"build_linux_all_profiles"}, @@ -1784,8 +1787,18 @@ def check_fork_runs(): f"{BRIDGE_WORKFLOW} checks out code. It runs with `checks: write` on data from a " f"fork and must never check out or run anything from the pull request" ) - if re.search(r"^\s+(contents|pull-requests|issues):\s*write\s*$", body, re.M): - failures.append(f"{BRIDGE_WORKFLOW} asks for more than `checks: write` and read access") + # It writes check runs from a trigger a fork can reach, so it starts with no + # scopes and the one job asks for exactly what it uses. `checks: write` + # creates the check; `pull-requests: read` lists the open pull requests. + # Everything it reads from the fork is public. + if not re.search(r"^permissions:\s*\{\}\s*$", body, re.M): + failures.append(f"{BRIDGE_WORKFLOW} must set `permissions: {{}}` at the top level") + scopes = permission_blocks(job_blocks(BRIDGE_WORKFLOW).get("bridge", "").splitlines()) + if scopes != [{"checks": "write", "pull-requests": "read"}]: + failures.append( + f"{BRIDGE_WORKFLOW}'s bridge job must ask for exactly `checks: write` and " + f"`pull-requests: read`, not {scopes}" + ) for failure in failures: print(f"fork runs: {failure}") diff --git a/dev/ci/fork-push-changed-files.sh b/dev/ci/fork-push-changed-files.sh index 33bf17dda6e..ff89332f907 100755 --- a/dev/ci/fork-push-changed-files.sh +++ b/dev/ci/fork-push-changed-files.sh @@ -28,11 +28,11 @@ # 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. +# Prints every tracked file when there is nothing to go on: the fetch fails or +# there is no merge base. Selecting no suites then would report a green run for a +# change nobody could see. An empty diff is not that case. With a merge base it +# means the branch adds nothing to upstream main, such as a branch pushed before +# its first commit, and the pull request will show nothing either. # # Usage: fork-push-changed-files.sh # UPSTREAM_URL overrides the repository to diff against, for the tests. @@ -43,14 +43,9 @@ head="${1:?usage: fork-push-changed-files.sh }" upstream="${UPSTREAM_URL:-https://github.com/apache/datafusion-comet.git}" ref="refs/remotes/upstream/main" -changed="" if git -c "http.https://github.com/.extraheader=" fetch --quiet --no-tags "$upstream" "+main:$ref" \ && git merge-base "$ref" "$head" >/dev/null 2>&1; then - changed=$(git diff --name-only "$ref"..."$head") -fi - -if [[ -n "$changed" ]]; then - printf '%s\n' "$changed" + git diff --name-only "$ref"..."$head" else git ls-tree -r --name-only "$head" fi diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 498d3e988c8..7e97d990854 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -154,7 +154,7 @@ test('upstream pushes and merge-queue completions cannot create fork checks', as assert.deepEqual((await execute({ event: 'workflow_run', notification: { event } })).writes, []); } }); -test('only runs for the exact branch head on push/manual events are mirrored', async () => { +test('only a push run for the exact branch head is mirrored', async () => { const result = await execute({ runs: [ run(8, { head_sha: 'b'.repeat(40) }), run(7, { head_branch: 'other' }), @@ -162,7 +162,7 @@ test('only runs for the exact branch head on push/manual events are mirrored', a run(5, { event: 'workflow_dispatch' }), run(1), ] }); - assert.equal(result.writes[0].details_url, run(5).html_url); + assert.equal(result.writes[0].details_url, run(1).html_url); }); for (const [state, expected] of [ ['active', 'neutral'], ['disabled_fork', 'action_required'], [null, 'action_required'], @@ -226,13 +226,6 @@ test('a rerun of the same workflow run updates the existing check', async () => assert.equal(writes[0].conclusion, 'success'); assert.equal(JSON.parse(writes[0].output.text).run_attempt, 2); }); -test('a manual run can replace a failed push run', async () => { - const check = await existing(run(1, { conclusion: 'failure' })); - const { writes } = await execute({ - checks: [check], runs: [run(2, { event: 'workflow_dispatch' }), run(1)], - }); - assert.equal(writes[0].details_url, run(2).html_url); -}); test('a deleted run clears its stale success result', async () => { const { writes } = await execute({ checks: [await existing()], runs: [] }); assert.equal(writes[0].method, 'update'); diff --git a/dev/ci/test-fork-push-changed-files.py b/dev/ci/test-fork-push-changed-files.py index 12eab44860c..11671c397bd 100755 --- a/dev/ci/test-fork-push-changed-files.py +++ b/dev/ci/test-fork-push-changed-files.py @@ -93,8 +93,8 @@ def test_revert_of_newer_upstream_code_is_kept_when_the_fork_is_stale(self): self.assertEqual(self.changed(), {"native/lib.rs", "README.md"}) def test_commits_made_on_the_fork_main_are_kept_when_pushed_to_a_branch(self): - # `git push fork main:my-fix`: the fork's main already holds the commits, - # so a diff against it is empty. Upstream does not have them. + # `git push fork main:my-fix`: the fork's main holds commits upstream does + # not have, and the pull request from the branch will show them. commit(self.fork, "native", {"native/lib.rs": "2\n"}) git(self.fork, "checkout", "-q", "main") git(self.fork, "merge", "-q", "--ff-only", "work") @@ -111,8 +111,10 @@ def test_an_unreachable_upstream_runs_everything(self): self.changed(url=str(Path(self.tmp.name, "missing"))), {"README.md", "native/lib.rs", "spark/Plan.scala"}) - def test_a_branch_that_adds_nothing_runs_everything(self): - self.assertEqual(self.changed(), {"README.md", "native/lib.rs", "spark/Plan.scala"}) + def test_a_branch_that_adds_nothing_to_upstream_selects_nothing(self): + # A branch pushed before its first commit: the pull request will show no + # change, so there is nothing to test. + self.assertEqual(self.changed(), set()) if __name__ == "__main__": diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index 1bae8651f82..f044731c308 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -135,7 +135,7 @@ To get it working: your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs the PR tier for the files your branch changes relative to upstream `main`, which is what the pull request will show: the Linux build, lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites - against Spark 4.1. If that comparison cannot be made, or finds nothing, it runs the whole tier. + against Spark 4.1. If that comparison cannot be made, it runs the whole tier. On the pull request, a `CI (fork)` check links to that run and mirrors its result. It is first written when the upstream run finishes. After that, a pull request updated in the last six hours @@ -167,7 +167,9 @@ Some things to know: CodeQL scan run in `apache/datafusion-comet` only. If your fork has scheduled workflows enabled they start no job. Miri can still be run on your fork by dispatching it. - **Opt-in suites still run here.** A fork cannot see the `run-*` labels on your pull request, so - those suites, and the `labeled` runs that start when a maintainer applies one, are unchanged. See + those suites, and the `labeled` runs that start when a maintainer applies one, run here. For + `run-all-spark-profiles` that is the other four Spark profiles, while your fork runs the default + profile and the lints, so together they cover every profile. See [Opting a pull request into a suite the PR tier skips](#opting-a-pull-request-into-a-suite-the-pr-tier-skips). - **Not everything moves.** A pull request from a branch of this repository, such as a committer's or dependabot's, and a pull request that targets a release branch, keep the PR tier here. From dd828c0f0c576baf4be9e9c8978071c496266b39 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 29 Oct 2026 15:09:36 -0600 Subject: [PATCH 11/12] ci: run a push to a fork's default branch like any other branch Skipping the fork's default branch, and with it the tests of a sync, needed the default branch to be told apart by name in two job guards and the bridge, and Actions compares branch names without regard to case. Run it like every other branch instead. - ci_fork.yml's jobs skip only upstream. Its trigger ignores `asf-site`. - The bridge looks up the ci_fork.yml run for every pull request, and no longer answers a pull request from the default branch with a neutral status. - A push that adds nothing to upstream main, such as a sync, selects no suite because its diff against upstream is empty, so it starts only Preflight and Detect changes. It writes no cache either, and the guide says so. - ci.yml's Preflight still skips a fork push that did not come through ci_fork.yml, since ci.yml also triggers on push to `main`. - check-ci-config.py's ci_fork.yml table now expects a push to a fork's main to run, and the docs no longer describe a default-branch skip. --- .github/workflows/README.md | 44 +++++++++++++--------------- .github/workflows/ci.yml | 8 ++--- .github/workflows/ci_fork.yml | 21 ++++++------- .github/workflows/fork_ci_status.yml | 25 +--------------- dev/ci/check-ci-config.py | 27 +++++++---------- dev/ci/compute-changes.py | 3 +- dev/ci/test-fork-ci-status.cjs | 33 +++------------------ docs/source/contributor-guide/ci.md | 18 +++++------- 8 files changed, 56 insertions(+), 123 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index d5e3225fbc0..0dbc7679ec2 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -212,40 +212,36 @@ the other queue-tier jobs, with path filters still applied; the nightly-only and label-only suites are not part of it. The design follows `apache/datafusion` (#25591) and `apache/arrow-rs` (#11788). Three files carry it: -| File | Role | -| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `ci_fork.yml` | Fires on a push to any branch of a fork except `asf-site` and calls `ci.yml`, after cancelling the branch's older runs. Its jobs are skipped upstream and for a fork's default branch. | -| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | -| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | +| File | Role | +| -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `ci_fork.yml` | Fires on a push to any branch of a fork except `asf-site` and calls `ci.yml`, after cancelling the branch's older runs. Its jobs are skipped upstream. | +| `compute-changes.py` | `is_fork_push` gives a fork the `pr` tier. `is_fork_pull_request` takes the `pr` tier away upstream, and leaves every `run-*` label and the `labeled` run. | +| `fork_ci_status.yml` | Mirrors the fork's run onto the pull request as the `CI (fork)` check, on `workflow_run` and every 15 minutes. | Things that follow from that, and are easy to break: - **The fork result is advisory.** `Required Checks` does not depend on `CI (fork)`, because a contributor controls the workflow that produced it. A green fork run that never ran the tests costs one queue slot, not a bad merge. -- **A push to a fork's default branch runs nothing.** It is a sync of commits - the queue already tested. `ci_fork.yml`'s job skips it, whatever the branch is - called. `ci.yml` also triggers on push to `main`, in a fork as upstream, so - its `Preflight` skips any fork push that did not come through `ci_fork.yml`, - which it tells apart by the caller's `github.workflow`, and `required_checks` - skips itself when `Preflight` does. That starts no runner for a sync, for a - fork's schedule, or for a second copy of a push. A branch called `main` in a - fork whose default is another branch is an ordinary branch and runs through - `ci_fork.yml`, so the bridge has one workflow to look up. The cost is that a - fork never has warm caches, so each branch run rebuilds the native library: - 59 minutes of wall time and 240 runner-minutes cold against 50 and 213 warm, - measured 2026-10-28 on one fork. A sync that did warm them would cost about - as much as it saved on its first run. The bridge answers a pull request from - a fork's default branch with a neutral "open the pull request from a branch", - because nothing tests it. +- **Every branch of a fork runs through `ci_fork.yml`, `main` included.** + `ci.yml` also triggers on push to `main`, in a fork as upstream, so its + `Preflight` skips any fork push that did not come through `ci_fork.yml`, which + it tells apart by the caller's `github.workflow`, and `required_checks` skips + itself when `Preflight` does. That starts no runner for a fork's schedule or + for a second copy of a push, and leaves the bridge one workflow to look up. A + push that adds nothing to upstream `main`, such as a sync of the fork, selects + no suite, so it starts only `Preflight` and `Detect changes` and warms no + cache. A fork's caches are therefore cold unless a run on its `main` builds, + and each branch run rebuilds the native library: 59 minutes of wall time and + 240 runner-minutes cold against 50 and 213 warm, measured 2026-10-28 on one + fork. - **Branch names compare without regard to case in Actions,** in `==`, `!=` and concurrency group names alike, and a fork can hold both `feature` and `Feature`. So a fork push has no shared concurrency group: it is keyed on its commit, and `ci_fork.yml`'s `supersede` job cancels the older runs of exactly - that branch, which it can compare with case. `Main` in a fork whose default - branch is `main` is skipped like `main`, and the bridge treats it the same way. - `check-ci-config.py` evaluates conditions with the same case-insensitive - comparison, and checks that the push trigger admits `main`. + that branch, which it can compare with case. `check-ci-config.py` evaluates + conditions with the same case-insensitive comparison, and checks that the push + trigger admits `main`. - **A pull request opened inside a fork skips the PR tier,** because the branch push already ran it, and `is_fork_pull_request` covers both that and a fork pull request into `main` here. A `run-*` label still runs its suite. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5472137c619..449132c28f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -149,11 +149,9 @@ jobs: # - A scheduled run in a fork, which would otherwise run the whole nightly # tier for want of an earlier run to diff against. # - A push to a fork that did not come through ci_fork.yml. ci.yml triggers - # on push to main, so it fires in a fork for a branch called `main` and for - # a sync of the fork's default branch. Neither is the contributor's pull - # request tier: ci_fork.yml runs every other branch, `main` included, - # except the fork's default branch, which is a sync of commits the queue - # already tested. The workflow name is what tells the two entry points + # on push to main, so it fires in a fork for its `main` as well, while + # ci_fork.yml runs every branch of a fork, `main` included. Running both + # would test the push twice. The workflow name tells the two entry points # apart, since a called workflow sees its caller's `github.workflow`. # --------------------------------------------------------------------------- preflight: diff --git a/.github/workflows/ci_fork.yml b/.github/workflows/ci_fork.yml index e0d92598fa4..a7b67b5b48f 100644 --- a/.github/workflows/ci_fork.yml +++ b/.github/workflows/ci_fork.yml @@ -32,14 +32,12 @@ # that can be skipped is named after this workflow's own run, and upstream it is # always skipped. # -# A fork's default branch is skipped by the job below, whatever it is called, -# because a push to it is a sync of upstream, which the merge queue already -# tested. A branch called `main` that is not the default is run like any other. -# ci.yml also triggers on a push to `main`, in a fork as upstream, but its -# Preflight job skips that run unless it came through this workflow, so a fork -# push is tested once and the bridge has one workflow to look up. A fork's caches -# are never warm; see "Pull requests from forks" in -# docs/source/contributor-guide/ci.md for what that costs. +# Every branch of a fork is run the same way, the default branch included. ci.yml +# also triggers on a push to `main`, in a fork as upstream, but its Preflight job +# skips that run unless it came through this workflow, so a fork push is tested +# once and the bridge has one workflow to look up. A push that adds nothing to +# upstream main, such as a sync of the fork, selects no suite; see +# dev/ci/fork-push-changed-files.sh. name: Comet CI (fork run) # No `concurrency` here, and none for a fork push in ci.yml either: see the @@ -65,9 +63,8 @@ jobs: name: Cancel superseded runs # Upstream, a push to any branch other than main is a pull request's own # business: the pull request run tests it, and a second run here would - # test it twice. In a fork the default branch is skipped, whatever it is - # called; `branches-ignore` above can only name a fixed branch. - if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch + # test it twice. + if: github.repository != 'apache/datafusion-comet' runs-on: ubuntu-slim # Failing to cancel only leaves an older run to finish, so it never blocks the run. continue-on-error: true @@ -100,7 +97,7 @@ jobs: ci: name: Fork run needs: supersede - if: github.repository != 'apache/datafusion-comet' && github.ref_name != github.event.repository.default_branch + if: github.repository != 'apache/datafusion-comet' # The union of what ci.yml's jobs request, for the same reason as in # ci_label.yml: GitHub checks a called workflow's permissions against # this ceiling for every job when the run starts, including the ones this diff --git a/.github/workflows/fork_ci_status.yml b/.github/workflows/fork_ci_status.yml index 71bcbdb0014..b01787d100c 100644 --- a/.github/workflows/fork_ci_status.yml +++ b/.github/workflows/fork_ci_status.yml @@ -116,13 +116,7 @@ jobs: // 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. - // Actions compares branch names without regard to case, so `Main` is - // the default branch `main` to ci_fork.yml's job guard. Compare the same way. - const fromMain = pr.head.ref.toLowerCase() === String(pr.head.repo.default_branch).toLowerCase(); - const data = fromMain ? null : await optional(() => github.rest.actions.listWorkflowRuns({ + const data = 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. ci_fork.yml runs on push only, and tests @@ -151,23 +145,6 @@ jobs: text: JSON.stringify({ head_sha: sha, run_id: run.id, run_attempt: run.run_attempt }), }, }; - } else if (fromMain) { - payload = { - status: 'completed', - conclusion: 'neutral', - details_url: GUIDE, - output: { - title: 'Open the pull request from a branch', - summary: [ - 'This pull request comes from the ' + pr.head.ref + ' branch of ' + pr.head.repo.full_name + - ', and CI does not run on a fork’s default branch: a push there is a sync of upstream, ' + - 'so nothing tests this change before the merge queue does.', - 'Push the change to a branch of your fork and open the pull request from it.', - '[Fork CI setup](' + GUIDE + ')', - ].join('\n\n'), - text: JSON.stringify({ head_sha: sha, run_id: null }), - }, - }; } else { const workflow = await optional(() => github.rest.actions.getWorkflow({ ...fork, workflow_id: WORKFLOW, diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 46e88c117dd..9823869bf8c 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -567,9 +567,9 @@ def fork_pr(action="synchronize", label=None, labels=(), base="main", repository (fork_pr(base="branch-1.1"), QUEUE_TIER | NIGHTLY_TIER), # A push to a fork's branch is the contributor's pull request tier and # nothing more: not main's cache refresh or site deploy (`docs` publishes to - # asf-site), and not the queue's or the nightly's suites. A push to the - # fork's default branch, a sync, never reaches this table: Preflight skips it - # (see check_fork_runs). + # asf-site), and not the queue's or the nightly's suites. That holds for the + # fork's default branch too; a push that adds nothing to upstream selects no + # file (see dev/ci/fork-push-changed-files.sh). ({"name": "push", "repository": FORK}, PR_TIER), # A pull request opened inside a fork leaves the PR tier to the branch push # that already ran it, so the same commit is not tested twice, but a label @@ -1688,9 +1688,9 @@ def check_fork_runs(): ) # Preflight is where a fork run that is not a pull request's tier is dropped, # and with it the whole run, since every other job needs Preflight through - # `changes`. ci.yml triggers on push to main, so it fires in a fork for a - # branch called `main` and for a sync of the default branch; only a push that - # came through ci_fork.yml, whose `github.workflow` is its own name, is tested. + # `changes`. ci.yml triggers on push to main, so it fires in a fork for its + # `main` as well as ci_fork.yml does; only a push that came through + # ci_fork.yml, whose `github.workflow` is its own name, is tested, once. ci_name = workflow_name(WORKFLOWS / "ci.yml") fork_name = workflow_name(FORK_WORKFLOW) failures.extend( @@ -1741,24 +1741,20 @@ def check_fork_runs(): failures.append( f"{FORK_WORKFLOW}'s `push: branches-ignore` {patterns} " f"{'admits' if actual else 'ignores'} a push to `{branch}`, and it should " - f"{'ignore' if actual else 'admit'} it; a fork's `main` is an ordinary branch " - f"unless it is the default, which the jobs' `if:` decides" + f"{'ignore' if actual else 'admit'} it; a fork's `main` is an ordinary branch" ) cases = [ (actions_context("push", UPSTREAM, fork_name, "feature"), False, "an upstream branch push"), (actions_context("push", UPSTREAM, fork_name, "main"), False, "an upstream push to main"), (actions_context("push", FORK, fork_name, "feature"), True, "a fork branch"), - (actions_context("push", FORK, fork_name, "main", "develop"), True, "a branch called main in a fork whose default is develop"), - (actions_context("push", FORK, fork_name, "main", "main"), False, "a sync of a fork's main"), - (actions_context("push", FORK, fork_name, "develop", "develop"), False, "a sync of a fork whose default is develop"), - (actions_context("push", FORK, fork_name, "Main", "main"), False, "a case variant of the default branch, which Actions compares as equal"), + (actions_context("push", FORK, fork_name, "main"), True, "a push to a fork's main"), ] for job in ("supersede", "ci"): failures.extend(condition_failures(FORK_WORKFLOW, job, cases)) if re.search(r"^concurrency:", body, re.M): failures.append( - f"{FORK_WORKFLOW} declares `concurrency`; ci.yml's own group is keyed on the " - f"branch for a fork push, and a second group on the caller deadlocks the pair" + f"{FORK_WORKFLOW} declares `concurrency`; ci.yml has its own group, and a " + f"second group on the caller deadlocks the pair" ) if re.search(r"^ (pull_request|pull_request_target|workflow_dispatch|schedule):", body, re.M): failures.append(f"{FORK_WORKFLOW} must trigger on `push` and nothing else") @@ -1878,13 +1874,12 @@ def evaluate_condition(text, context): return bool(eval(compile(tree, "", "eval"), {"__builtins__": {}}, values)) # noqa: S307 -def actions_context(event_name, repository, workflow="", ref_name="feature", default_branch="main"): +def actions_context(event_name, repository, workflow="", ref_name="feature"): return { "github.event_name": event_name, "github.repository": repository, "github.workflow": workflow, "github.ref_name": ref_name, - "github.event.repository.default_branch": default_branch, } diff --git a/dev/ci/compute-changes.py b/dev/ci/compute-changes.py index 6c9d742f768..f07abdb8329 100644 --- a/dev/ci/compute-changes.py +++ b/dev/ci/compute-changes.py @@ -690,8 +690,7 @@ def event_allows(job, event): if name == "push": # A push to a fork's branch is the contributor's own pull request tier. # The "push" tier is main's cache refresh and site deploy, and a fork - # has no site to deploy. A push to a fork's default branch never gets - # here: Preflight skips it, as a sync of commits the queue already tested. + # has no site to deploy. if is_fork_push(event): return "pr" in tiers return "push" in tiers diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index 7e97d990854..fa90783891d 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -54,7 +54,7 @@ const pr = { sha, ref: 'feature', repo: { owner: { login: 'contributor' }, name: 'datafusion-comet', - full_name: 'contributor/datafusion-comet', default_branch: 'main', + full_name: 'contributor/datafusion-comet', }, }, base: { ref: 'main' }, @@ -281,34 +281,9 @@ test('no open PRs means no fork API calls', async () => { assert.deepEqual(result.writes, []); }); -for (const defaultBranch of ['main', 'develop']) { - test('a PR from the fork default branch ' + defaultBranch + ' is told to use a branch', async () => { - const head = { - ...pr.head, ref: defaultBranch, repo: { ...pr.head.repo, default_branch: defaultBranch }, - }; - const mainPr = { ...pr, head }; - const { writes, calls } = await execute({ recent: [mainPr], slice: [mainPr] }); - assert.equal(calls.filter(([kind]) => kind === 'runs').length, 0, 'a default branch has no run to read'); - assert.equal(writes.length, 1); - assert.equal(writes[0].conclusion, 'neutral'); - assert.equal(writes[0].output.title, 'Open the pull request from a branch'); - }); -} - -test('a PR from a case variant of the default branch is told to use a branch', async () => { - // Actions compares branch names without regard to case, so ci_fork.yml skips Main - // in a fork whose default branch is main, and there is no run to find. - const head = { ...pr.head, ref: 'Main' }; - const variantPr = { ...pr, head }; - const { writes, calls } = await execute({ recent: [variantPr], slice: [variantPr] }); - assert.equal(calls.filter(([kind]) => kind === 'runs').length, 0); - assert.equal(writes[0].output.title, 'Open the pull request from a branch'); -}); -test('a branch called main in a fork whose default branch is not main is mirrored', async () => { - // ci.yml triggers on push to a branch called main as well, but it skips that run - // unless ci_fork.yml called it, so ci_fork.yml is the one workflow to look up. - const head = { ...pr.head, ref: 'main', repo: { ...pr.head.repo, default_branch: 'develop' } }; - const mainPr = { ...pr, head }; +test('a PR from a branch called main, the default branch included, is mirrored', async () => { + // ci_fork.yml runs every branch of a fork, so there is always one workflow to look up. + const mainPr = { ...pr, head: { ...pr.head, ref: 'main' } }; const { writes } = await execute({ branch: 'main', recent: [mainPr], slice: [mainPr], candidates: [mainPr], }); diff --git a/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index f044731c308..9088ddbcf22 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -131,7 +131,7 @@ To get it working: fork's **Actions** tab and choose **I understand my workflows, go ahead and enable them**. 2. **Sync your fork and rebase your branch onto `main`.** The workflows that run on your fork are the ones in your branch, so a branch cut before this arrangement existed runs nothing. -3. **Push your branch.** Each push to a branch other than your fork's default branch starts a `Comet CI (fork run)` in +3. **Push your branch.** Each push to a branch of your fork starts a `Comet CI (fork run)` in your fork's **Actions** tab, and a newer push to the same branch cancels the run in flight. It runs the PR tier for the files your branch changes relative to upstream `main`, which is what the pull request will show: the Linux build, lints, Rust tests, TPC-H and TPC-DS, and the Comet test suites @@ -153,16 +153,12 @@ Some things to know: controls the workflow that produced it. A green fork run is a good sign, not a merge gate. The queue is the gate, and it tests your change merged with `main` and everything queued ahead of it, where the fork run tests your branch head alone. -- **Runs are cold.** Caches are scoped to a branch or to the repository's default branch, and a - push to your fork's default branch, usually `main`, does not run the Comet PR tier, because it is a - sync of commits the merge queue already tested. So the Cargo and Maven caches are never warm on a fork, and a run rebuilds the native - library: a run of the PR tier on a fork took about an hour of wall time and 240 runner-minutes, of - which the native build alone was 23 minutes. The same run with warm caches took 50 minutes and 213 - runner-minutes. -- **Open the pull request from a branch.** The Comet PR tier does not run on your fork's default - branch, usually `main`, so a pull request from it gets no CI before the merge queue. `CI (fork)` - says so. A branch that differs from the default only in capitalization, such as `Main`, counts as - the default branch, because GitHub Actions compares branch names without regard to case. +- **Caches are cold unless `main` builds.** A branch restores caches saved on its own ref or on your + fork's default branch, and a run on your fork's `main` saves them only when it builds. A push to + `main` that adds nothing to upstream, such as syncing your fork, selects no suite, so it warms + nothing, and a branch run rebuilds the native library. A run of the PR tier on a fork with cold + caches took about an hour of wall time and 240 runner-minutes, of which the native build alone was + 23 minutes. The same run with warm caches took 50 minutes and 213 runner-minutes. - **Scheduled runs stay upstream.** The nightly run, Miri, the stale-PR closer and the weekly CodeQL scan run in `apache/datafusion-comet` only. If your fork has scheduled workflows enabled they start no job. Miri can still be run on your fork by dispatching it. From 6dc32f21d823f4d1e98a0037dc6a39573f8753ff Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Fri, 30 Oct 2026 07:16:05 -0600 Subject: [PATCH 12/12] ci: key a fork push's concurrency group on its run A fork push was keyed on its commit. A backup branch created at the commit a pull request is building has the same commit, so its run shared the group and, with cancel-in-progress, cancelled the pull request's build, and the bridge then reported the cancelled run. Key a fork push on its run id instead, so no other push shares its group. ci_fork.yml's supersede job still cancels older runs of exactly the same branch. The workflows README no longer says a sync starts only Preflight and Detect changes. It also starts the supersede job and the aggregator, so it says what the saving is: no build or test suite and no cache write. --- .github/workflows/README.md | 11 ++++++----- .github/workflows/ci.yml | 12 +++++++----- dev/ci/test-fork-ci-status.cjs | 18 +++++++++--------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 0dbc7679ec2..d50ef7cd58a 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -230,16 +230,17 @@ Things that follow from that, and are easy to break: itself when `Preflight` does. That starts no runner for a fork's schedule or for a second copy of a push, and leaves the bridge one workflow to look up. A push that adds nothing to upstream `main`, such as a sync of the fork, selects - no suite, so it starts only `Preflight` and `Detect changes` and warms no - cache. A fork's caches are therefore cold unless a run on its `main` builds, + no build or test suite and writes to no build cache. A fork's caches are therefore cold unless a run on its `main` builds, and each branch run rebuilds the native library: 59 minutes of wall time and 240 runner-minutes cold against 50 and 213 warm, measured 2026-10-28 on one fork. - **Branch names compare without regard to case in Actions,** in `==`, `!=` and concurrency group names alike, and a fork can hold both `feature` and - `Feature`. So a fork push has no shared concurrency group: it is keyed on its - commit, and `ci_fork.yml`'s `supersede` job cancels the older runs of exactly - that branch, which it can compare with case. `check-ci-config.py` evaluates + `Feature`. So a fork push has no concurrency group shared with another push: it + is keyed on its run, which also stops a backup branch created at the commit a + pull request is building from cancelling that build. `ci_fork.yml`'s `supersede` + job cancels the older runs of exactly the same branch instead, which it can + compare with case. `check-ci-config.py` evaluates conditions with the same case-insensitive comparison, and checks that the push trigger admits `main`. - **A pull request opened inside a fork skips the PR tier,** because the branch diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 449132c28f8..0fc66aba7f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -83,12 +83,14 @@ name: Comet CI # commit's push run. A pull request is keyed on its number, and not on its # branch name: nearly every pull request comes from a fork, and two forks' # branches can share a name, which would have one cancel the other's run. A push -# to a fork (ci_fork.yml) is keyed on its commit, like any push: a newer push to -# the branch cancels the run in flight through ci_fork.yml's `supersede` job -# instead, because group names compare without regard to case and a fork can -# hold both `feature` and `Feature`. +# to a fork (ci_fork.yml) is keyed on its run, so no other push shares its group: +# a backup branch created at the commit a pull request is building would +# otherwise cancel that build. A newer push to the same branch cancels the run in +# flight through ci_fork.yml's `supersede` job instead, because group names +# compare without regard to case and a fork can hold both `feature` and +# `Feature`. concurrency: - group: ${{ github.repository }}-${{ github.event.pull_request.number || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} + group: ${{ github.repository }}-${{ github.event.pull_request.number || (github.event_name == 'push' && github.repository != 'apache/datafusion-comet' && github.run_id) || github.sha }}-${{ github.workflow }}-${{ github.event.action == 'labeled' && github.event.label.name || github.event_name }} cancel-in-progress: true # No `labeled` here: ci_label.yml handles that event and calls this file diff --git a/dev/ci/test-fork-ci-status.cjs b/dev/ci/test-fork-ci-status.cjs index fa90783891d..a8f16bbf70e 100644 --- a/dev/ci/test-fork-ci-status.cjs +++ b/dev/ci/test-fork-ci-status.cjs @@ -299,20 +299,20 @@ function group(github) { const forkPush = { // GitHub expressions resolve absent properties to an empty string. repository: 'contributor/datafusion-comet', event_name: 'push', event: { pull_request: { number: '' } }, - ref: 'refs/heads/feature', sha: 'first', workflow: 'Comet CI (fork run)', + ref: 'refs/heads/feature', sha: 'first', run_id: 1, workflow: 'Comet CI (fork run)', }; const pullRequest = (number, extra = {}) => ({ ...forkPush, repository: 'apache/datafusion-comet', event_name: 'pull_request', event: { pull_request: { number } }, ref: 'refs/pull/' + number + '/merge', ...extra, }); -test('fork pushes get a group each, so none is cancelled by a case variant of its branch', () => { - // Group names compare without regard to case, so a group keyed on the branch would - // let a push to `Feature` cancel `feature`. ci_fork.yml's supersede job cancels - // older runs of exactly one branch instead. - assert.notEqual(group(forkPush), group({ ...forkPush, sha: 'second' })); - assert.equal( - group({ ...forkPush, ref: 'refs/heads/feature' }).toLowerCase(), - group({ ...forkPush, ref: 'refs/heads/Feature' }).toLowerCase()); +test('every fork push has a group of its own, so none cancels another', () => { + // A backup branch created at the commit a pull request is building has the same + // sha, so a group keyed on the commit would let its run cancel the build. + // ci_fork.yml's supersede job cancels older runs of exactly one branch instead, + // which a group cannot: names compare without regard to case. + assert.notEqual(group(forkPush), group({ ...forkPush, run_id: 2 })); + assert.notEqual(group(forkPush), group({ ...forkPush, run_id: 2, ref: 'refs/heads/backup' })); + assert.notEqual(group(forkPush), group({ ...forkPush, sha: 'second', run_id: 2 })); }); test('upstream pushes and queue runs stay separate per commit', () => { for (const event_name of ['push', 'merge_group']) {