diff --git a/.github/workflows/README.md b/.github/workflows/README.md index e0afce047bf..619e99f8b72 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 e4415959bd6..7f87d591995 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 00000000000..6af3e919284 --- /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 55edd5f5e27..31b69675b7a 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 @@ def check_cache_save_scope(): 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 cea032370e9..fa367de798a 100644 --- a/docs/source/contributor-guide/ci.md +++ b/docs/source/contributor-guide/ci.md @@ -135,9 +135,10 @@ gh pr edit --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