andygrove commented on code in PR #7311: URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4416928541
########## .github/workflows/fork_ci_status.yml: ########## @@ -0,0 +1,260 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Mirror a contributor's fork run (ci_fork.yml) onto its pull request as the informational `CI (fork)` check, +# after upstream PR CI completes and through scheduled polling for late runs and +# reruns. The check is feedback only: `Required Checks` does not depend on it, +# because a contributor controls the workflow that produced it, and the merge +# queue runs the queue tier on the merge result. +# +# Reads fork runs and writes upstream checks using reviewed default-branch code. +# No checkout, artifacts, or code from the pull request are used here. +# +# Adapted from apache/arrow-rs#11788 and apache/datafusion#25591. +# Test with Node.js 24: +# node --test dev/ci/test-fork-ci-status.cjs +# Tests mock GitHub APIs. End-to-end testing needs a fork pull request after this +# workflow is installed on the default branch. +name: Fork CI status + +on: + workflow_run: + workflows: [Comet CI] + types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + actions: read + checks: write + pull-requests: read + +# Like DataFusion, group notifications separately from scheduled/manual sweeps. +# workflow_run identifies the fork and branch rather than the PR number. +concurrency: + group: ${{ github.workflow }}-${{ github.event.workflow_run.head_repository.full_name || 'update' }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: ${{ github.event_name == 'workflow_run' }} + +jobs: + bridge: + # A workflow_run for a pull request from a branch of this repository has no + # fork run to mirror, so it never starts a job. + if: >- + github.repository == 'apache/datafusion-comet' + && (github.event_name != 'workflow_run' + || (github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.head_repository.full_name != github.repository)) + name: Mirror the fork run + runs-on: ubuntu-slim + timeout-minutes: 10 + steps: + - name: Create or update the check + uses: actions/github-script@v9 + with: + script: | + // Adapted from apache/datafusion#25591. Fork results are informational: + // contributors control their workflows; only upstream merge-queue CI gates merging. + const CHECK = 'CI (fork)'; + const WORKFLOW = 'ci_fork.yml'; + const upstream = { owner: context.repo.owner, repo: context.repo.repo }; + const repository = upstream.owner + '/' + upstream.repo; + const GUIDE = 'https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/ci.md#pull-requests-from-forks'; + const SWEEP_SIZE = 25; + const listing = { ...upstream, state: 'open', base: 'main', sort: 'updated', direction: 'desc' }; + const isForkPr = (pr) => pr.state === 'open' && pr.head.repo && + pr.head.repo.full_name.toLowerCase() !== repository.toLowerCase() && pr.base.ref === 'main'; + const externalId = (pr) => 'comet-fork-ci:' + pr.number; + const conclusions = new Set([ + 'action_required', 'cancelled', 'failure', 'neutral', 'skipped', 'success', 'timed_out', + ]); + const UPDATE_NOTE = + 'This check is informational: the contributor controls the fork workflow. ' + + 'The upstream merge queue runs the required CI checks on the proposed merge result. ' + + 'Active or missing runs on recently updated PRs are polled every 15 minutes; ' + + 'a rotating sweep also discovers reruns on older PRs. Scheduled updates may be delayed.'; + + // An absent workflow/run is expected on a fork with Actions disabled. Other + // API errors must fail the bridge rather than falsely reporting missing CI. + async function optional(request) { + try { + return (await request()).data; + } catch (error) { + if (error.status === 404) return null; + throw error; + } + } + + async function syncCheck(pr, includeCompleted = true) { + if (!isForkPr(pr)) return; + const fork = { owner: pr.head.repo.owner.login, repo: pr.head.repo.name }; + const sha = pr.head.sha; + const checks = await github.paginate(github.rest.checks.listForRef, { + ...upstream, ref: sha, check_name: CHECK, per_page: 100, + }); + const check = checks.find((item) => + item.app?.slug === 'github-actions' && item.external_id === externalId(pr)); + let previous = {}; + try { previous = JSON.parse(check?.output?.text || '{}'); } catch {} + // A missing run must keep being polled, even though its check is completed. + if (!includeCompleted && check?.status === 'completed' && previous.run_id) return; + + // A fork's default branch is never tested: ci_fork.yml skips it, and a + // push there is a sync, which runs nothing. Looking for a run would + // find none, or worse, one that tested nothing and passed. + const fromMain = pr.head.ref === pr.head.repo.default_branch; + const data = fromMain ? null : await optional(() => github.rest.actions.listWorkflowRuns({ + ...fork, workflow_id: WORKFLOW, head_sha: sha, branch: pr.head.ref, per_page: 100, Review Comment: Good catch. ci.yml's own push trigger was starting a second, unbridged run for a branch called `main` in a fork whose default is another branch. Fixed in 40eb6cf2e9 by giving fork pushes one entry point: `ci_fork.yml` now runs every fork branch except the fork's default branch (`main` included), and `Preflight` skips a fork push unless `ci_fork.yml` called it, which it tells by the caller's `github.workflow`. So that push is tested once, by the workflow the bridge looks up. I added the regression test you described (default `develop`, source `main`, successful `ci_fork.yml` push), and checked it fails if the bridge goes back to treating a branch named `main` as the default. I also confirmed on a fork that `Preflight` runs when called through `ci_fork.yml`. ########## dev/ci/check-ci-config.py: ########## @@ -1560,6 +1650,147 @@ 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") +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): + 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" + ) + # 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" + ) + 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") + else: + 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") + 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 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 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 " + 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 + + +# 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"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 any( + UPSTREAM_GUARD.search(c) for c in job_conditions(body) Review Comment: You're right, the check found the repository comparison in the push clause and called it the schedule guard. It no longer matches text. `check-ci-config.py` now evaluates the `if:` of every root job of each scheduled workflow for a scheduled run in a fork, and `Preflight` and `ci_fork.yml`'s job for a table of events: upstream and fork schedules, pushes through and around `ci_fork.yml`, a sync, a branch called `main`, pull requests, the queue and a dispatch. I reproduced your case, removing only the schedule clause and keeping the push clause, and it now fails. I also removed the guard from `miri.yml`, the aggregator and `ci_fork.yml` in turn, and each fails. ########## .github/workflows/ci.yml: ########## @@ -118,19 +129,29 @@ 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 - # 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. + # 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. + # + # 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 + 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) Review Comment: Yes, the aggregator still started a runner, and the docs claimed nothing runs. `required_checks` is now `always() && needs.preflight.result != 'skipped'`, so a failed or cancelled dependency still reaches it and only a skipped `Preflight` skips it. `nightly_report` now fires on `failure` or `cancelled` of the aggregator rather than anything that is not `success`, so a fork's skipped schedule reports nothing. The config check pins the aggregator condition. On every event that can reach the merge box `Preflight` is never skipped, so `Required Checks` still reports there. ########## docs/source/contributor-guide/ci.md: ########## @@ -111,6 +114,65 @@ 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 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: + +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. +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 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 Review Comment: Fair, the guide overstated it. It now says the check is first written when the upstream run finishes, that a pull request updated in the last six hours with a running or missing check is rechecked every 15 minutes, and that a check already showing a result is only rechecked by the sweep of 25 pull requests per 15 minutes, about two hours for 200 open ones. It tells contributors that after a rerun the pull request can show the old result for hours and that the fork's Actions tab is the source of truth. I did not change the bridge's cadence. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
