sunchao commented on code in PR #7311:
URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4415793087
##########
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:
I removed only the schedule clause from Preflight and kept the
default-branch push condition. `python3 dev/ci/check-ci-config.py` still
passed. On a fork schedule the surviving `event_name != 'push'` would let
Preflight and the nightly run. This test finds the upstream repository
comparison in the unrelated push clause and accepts it as the schedule guard.
Could the check verify that the condition actually rejects a fork `schedule`
event? Keeping the push clause in that regression case is what exposes the
false positive.
##########
.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:
Skipping Preflight does suppress the builds, but `Required Checks` still has
`if: always()` and starts an `ubuntu-slim` runner with all prerequisites
skipped. A fork’s `main` sync (when `main` is its default) and an enabled fork
schedule therefore still start one job. If the intent is zero runner use for
these events, could you guard the aggregator too, while keeping it active for
real failures? The nightly reporter also needs to exclude fork schedules if the
aggregator is skipped, since it currently treats `skipped` as unsuccessful. If
this one summary job is intentional, please qualify the claims that nothing
runs.
##########
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:
A contributor can rerun a failed fork job and still see the old failure here
after the next poll. Once the mirrored check is complete, the bridge only
rechecks it in the rotating slice of 25 open PRs. With 200 open PRs that takes
about two hours for a full rotation even if each scheduled run starts on time.
Could the rerun instructions distinguish that cadence from the 15-minute
polling of recently updated PRs with active or missing runs?
##########
.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:
When a fork’s default branch is `develop` and its PR branch is named `main`,
the push starts `ci.yml` directly. `ci_fork.yml` ignores literal `main`, and
the `ci.yml` Preflight guard allows the push because `main` differs from the
fork’s default branch. This lookup only asks for `ci_fork.yml`. With those
branch names the bridge returned “No fork CI run for this commit” in a local
mock, even though `ci.yml` is the workflow responsible for testing that push.
Could the bridge look up the matching `ci.yml` push for this case, or could the
triggers give it a single workflow to look up? A regression test with default
`develop`, source `main`, and a successful `ci.yml` push would cover it.
--
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]
