This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6161-5eed2015bdcd798f45b0f800d73e98b2112d9d72 in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
commit e927f1fed3a48342d4c0a105622eff1b945d77be Author: Andy Grove <[email protected]> AuthorDate: Wed Sep 23 20:53:07 2026 +0000 ci: run label-triggered CI as a separate workflow (#6161) A pull request opened with a label already applied fires opened and labeled together, so ci.yml ran twice at the same commit. GitHub evaluates the required checks against only one of those runs, and when it picked the label run, Required Checks stayed Expected and the merge queue never accepted the pull request. Move the labeled trigger to ci_label.yml, which calls ci.yml through workflow_call, so label runs get their own check suite. Add a check-ci-config guard for the invariant. Closes #6159 --- .github/workflows/README.md | 27 +++++++++---- .github/workflows/ci.yml | 43 +++++++++++++------- .github/workflows/ci_label.yml | 56 +++++++++++++++++++++++++ dev/ci/check-ci-config.py | 81 +++++++++++++++++++++++++++++++++++-- docs/source/contributor-guide/ci.md | 7 ++-- 5 files changed, 186 insertions(+), 28 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index e0afce047b..619e99f8b7 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -171,13 +171,26 @@ safe to make a required check. ### Label events -`ci.yml` also fires on `pull_request.types: [labeled]`, so applying -`run-spark-3.4-tests`, `run-spark-4.0-tests` or `run-iceberg-tests` starts the -job that label gates without needing a new push. GitHub cannot filter a -`pull_request` trigger by label name, so **every** label added to a PR starts a -run, including labels that gate nothing. - -Two rules keep those runs from corrupting the PR's status: +`ci_label.yml` fires on `pull_request.types: [labeled]` and calls `ci.yml` +through `workflow_call`, so applying `run-spark-3.4-tests`, +`run-spark-4.0-tests` or `run-iceberg-tests` starts the job that label gates +without needing a new push. GitHub cannot filter a `pull_request` trigger by +label name, so **every** label added to a PR starts a run, including labels +that gate nothing. A called workflow sees its caller's event context, so +`POLICY` still reads the `labeled` action and the label name. + +These rules keep those runs from corrupting the PR's status: + +- Label runs come from their own workflow, not from a `labeled` type on + `ci.yml`'s trigger. When one workflow runs twice at the same commit, GitHub + evaluates the PR's required checks against only one of the two runs. A PR + opened with a label already applied fires `opened` and `labeled` together, + and when GitHub picked the label run, `Required Checks` showed as "Expected" + forever and the merge queue never accepted the PR, see + [#6159](https://github.com/apache/datafusion-comet/issues/6159). A run of + `ci_label.yml` has its own check suite, and every check it publishes is + nested under its `Label run` job, so it cannot hide or replace a commit + run's check. - `preflight` and `changes` carry no event guard and run every time. A job held back by `if:` still publishes a check run under its own name with conclusion diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e4415959bd..7f87d59199 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,18 +62,30 @@ name: Comet CI # A `labeled` event (e.g. the run-spark-*-tests gates, or dependabot's automatic # `dependencies` label added ~1s after open) fires at the same commit as the -# opened/synchronize run. Keying the group on the label name keeps labeled runs -# in their own subgroup so they never cancel the real commit run. 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. +# opened/synchronize run. Label runs reach this file through ci_label.yml, so +# `github.workflow` already separates them from the commit run; keying the +# group on the label name as well keeps two different labels from cancelling +# 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. concurrency: group: ${{ github.repository }}-${{ github.head_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 +# through `workflow_call`. When one workflow runs twice at the same commit, +# GitHub evaluates the pull request's required checks against only one of the +# two runs. A PR opened with a label already applied gets both an `opened` and +# a `labeled` run, and when the label run was the one GitHub picked, the +# `Required Checks` context showed as "Expected" forever and the merge queue +# never took the PR (issue #6159). A separate workflow gets its own check +# suite, so a label run can never hide the commit run's verdict. +# dev/ci/check-ci-config.py enforces this. on: pull_request: - types: [opened, synchronize, reopened, labeled] + types: [opened, synchronize, reopened] + workflow_call: merge_group: push: branches: @@ -530,15 +542,16 @@ jobs: # `if: always()` is what makes this work: without it the job inherits the # default `success()` and is itself skipped the moment any dependency fails. # - # The name is an expression because `ci.yml` also fires on `labeled`, and on - # that event POLICY deliberately skips the PR tier (it already ran at this - # commit). A label run's verdict therefore says nothing about the commit's - # applicable suites, yet GitHub keeps only the most recent check run per name - # per commit, so publishing it as `Required Checks` would let a `dependencies` - # label turn a still-running or red commit run green (issue #5007). Label - # runs publish under a name nothing requires instead. Skipping the job here - # would not help: a skipped check run still carries the name and still counts - # as passing. dev/ci/check-ci-config.py enforces both halves of this. + # The name is an expression because ci_label.yml runs this file on `labeled` + # too, and on that event POLICY deliberately skips the PR tier (it already + # ran at this commit). A label run's verdict therefore says nothing about the + # commit's applicable suites, so publishing it as `Required Checks` could let + # a `dependencies` label turn a still-running or red commit run green (issue + # #5007). Label runs publish under a name nothing requires instead. Skipping + # the job here would not help: a skipped check run still carries the name and + # still counts as passing. Running label runs as a separate workflow (issue + # #6159) is the other half: it stops a label run from hiding this job's + # verdict from the merge box. dev/ci/check-ci-config.py enforces all of it. # --------------------------------------------------------------------------- required_checks: name: ${{ github.event.action == 'labeled' && 'Required Checks (label run)' || 'Required Checks' }} diff --git a/.github/workflows/ci_label.yml b/.github/workflows/ci_label.yml new file mode 100644 index 0000000000..6af3e91928 --- /dev/null +++ b/.github/workflows/ci_label.yml @@ -0,0 +1,56 @@ +# 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. + +# Label runs: applying a `run-*` label to a pull request runs the suite that +# label gates at the current commit, without a new push. The pipeline is +# ci.yml itself; POLICY in dev/ci/compute-changes.py reads the `labeled` +# action from the event context, which a called workflow shares with its +# caller, and runs only what the new label gates. +# +# This is a separate workflow, and not a `labeled` type on ci.yml's own +# `pull_request` trigger, because of how GitHub treats two runs of one +# workflow at the same commit: it evaluates the pull request's required checks +# against only one of them. A pull request opened with a label already +# applied fires `opened` and `labeled` together, and when the label run was +# the one picked, `Required Checks` showed as "Expected" forever and the merge +# queue never accepted the pull request (issue #6159). Runs of this workflow +# get their own check suite, so they cannot hide the commit run's verdict. +# Nothing here publishes a required name either: every check is nested under +# the calling job's name, and the aggregator reports as +# `Required Checks (label run)`. +name: Comet CI (label run) + +# No `concurrency` here. ci.yml declares its own group, keyed on this +# workflow's name and the label, and the same group on the caller as well +# would deadlock the two against each other. +on: + pull_request: + types: [labeled] + +jobs: + ci: + name: Label run + # A called workflow's jobs can hold no more than the caller grants, and + # GitHub checks that for every job when the run starts, including the + # ones this event skips. This is the union of what ci.yml's jobs request: + # `changes` and `nightly_report` read the Actions API, and + # `nightly_report` files issues (it only runs on the schedule). + permissions: + actions: read + contents: read + issues: write + uses: ./.github/workflows/ci.yml diff --git a/dev/ci/check-ci-config.py b/dev/ci/check-ci-config.py index 55edd5f5e2..31b69675b7 100644 --- a/dev/ci/check-ci-config.py +++ b/dev/ci/check-ci-config.py @@ -34,9 +34,12 @@ # rename on either side of the ci.yml/.asf.yaml pair turns the required # context into one that never reports, which blocks *every* merge to main # until INFRA removes it by hand. The job's name must also route `labeled` -# runs, which skip the PR tier by design, to a name nothing requires: -# GitHub keeps the most recent check run per name per commit, so a label -# run publishing the required name would overwrite the real verdict. +# runs, which skip the PR tier by design, to a name nothing requires, so +# a label run cannot stand in for the real verdict. And `labeled` runs +# must come from a separate workflow (ci_label.yml) rather than ci.yml's +# own trigger: with two ci.yml runs at one commit, GitHub evaluates the +# required checks against only one of them, and when that is the label +# run the required context never reports (issue #6159). # # 4. Artifact-name uniqueness. Artifact names are scoped to the *run*, not # to the calling workflow, and ci.yml calls the Spark SQL and Iceberg @@ -943,6 +946,77 @@ def check_required_checks(): return not failures +def ci_triggers(): + """Return {event: its `types:` list, or None} from ci.yml's top-level `on:`.""" + lines = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8").splitlines() + event_key = re.compile(r"^ ([a-z_]+):\s*$") + types_key = re.compile(r"^ types:\s*\[(.*)\]\s*$") + triggers = {} + in_on = False + current = None + for line in lines: + if line.startswith("on:"): + in_on = True + continue + if not in_on: + continue + if line and not line.startswith((" ", "#")): + break + match = event_key.match(line) + if match: + current = match.group(1) + triggers[current] = None + continue + match = types_key.match(line) + if match and current: + triggers[current] = [t.strip() for t in match.group(1).split(",")] + return triggers + + +def check_label_runs_separate(): + """`labeled` runs come from ci_label.yml, never from ci.yml's own trigger. + + When one workflow runs twice at the same commit, GitHub evaluates the pull + request's required checks against only one of the two runs. A pull request + opened with a label already applied fires `opened` and `labeled` together; + if ci.yml took both, the label run could be the one GitHub picked, and it + publishes `Required Checks (label run)`, so `Required Checks` showed as + "Expected" forever and the merge queue never took the pull request (issue + #6159). A separate calling workflow gets its own check suite. + """ + failures = [] + triggers = ci_triggers() + pull_request_types = triggers.get("pull_request") or [] + if "labeled" in pull_request_types: + failures.append( + "ci.yml triggers on `pull_request: labeled`. Two ci.yml runs at one " + "commit let the label run hide `Required Checks` from the merge box; " + "leave `labeled` to ci_label.yml" + ) + if "workflow_call" not in triggers: + failures.append( + "ci.yml has no `workflow_call` trigger, so ci_label.yml cannot call it " + "and applying a `run-*` label no longer runs anything" + ) + + label_workflow = WORKFLOWS / "ci_label.yml" + if not label_workflow.exists(): + 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): + 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( + f"{label_workflow} must trigger on `pull_request: types: [labeled]` " + f"and nothing else; any other type would run ci.yml twice per push" + ) + + for failure in failures: + print(f"label runs: {failure}") + return not failures + + def check_cache_refresh_scope(): """Every job in pr_build_linux.yml is either a cache writer or guarded. @@ -1208,6 +1282,7 @@ if __name__ == "__main__": ok = check_artifact_names() and ok ok = check_local_actions_have_checkout() and ok ok = check_required_checks() and ok + ok = check_label_runs_separate() 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/docs/source/contributor-guide/ci.md b/docs/source/contributor-guide/ci.md index cea032370e..fa367de798 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -135,9 +135,10 @@ gh pr edit <number> --add-label run-spark-3.5-tests ``` Applying a label starts a new run immediately at the pull request's current commit. That run -executes only the suite the label gates; the PR tier already ran at that commit and is not -repeated. Its aggregate verdict is published as `Required Checks (label run)` rather than -`Required Checks`, so it can be read alongside the commit run without replacing it. +belongs to the separate `Comet CI (label run)` workflow and executes only the suite the label +gates; the PR tier already ran at that commit and is not repeated. Its aggregate verdict is +published as `Label run / Required Checks (label run)` rather than `Required Checks`, so it can be +read alongside the commit run without replacing it. For a queue-tier suite, that separate name costs nothing: the merge queue runs the suite again before the change lands, so a failure a label run surfaced still blocks the merge later. A --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
