This is an automated email from the ASF dual-hosted git repository. vatsrahul1001 pushed a commit to branch v3-4-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit b017aa02bbb096421cb44c19fb2d7d25b3498a86 Author: Shahar Epstein <[email protected]> AuthorDate: Fri Nov 6 12:18:22 2026 +0200 [v3-4-test] Hold back only build-info changes in fork CI and stop drafting them (#76282) (#76394) A docs-only change under dev/breeze/ drafted the pull request and waited for a maintainer as if it changed CI, although only the files that decide which jobs run and on which runners (workflows, breeze sources, the breeze installation, and scripts/ci/prek/ whose provider dependencies script selective checks runs) can let a pull request steer its own CI. Such pull requests still wait for a maintainer to run project CI, but are no longer drafted, so they stay visible for review. Changes [...] Generated-by: Claude Opus 5 (cherry picked from commit 3edd0521b6c950312fb390ccd4633e725236d980) Co-authored-by: Jarek Potiuk <[email protected]> --- contributing-docs/05_pull_requests.rst | 20 ++-- contributing-docs/18_contribution_workflow.rst | 4 +- contributing-docs/25_maintainer_pr_triage.md | 16 ++-- ...0-run-ci-for-external-contributions-in-forks.md | 5 +- dev/breeze/doc/ci/05_workflows.md | 10 +- .../doc/ci/08_fork_ci_for_external_contributors.md | 42 ++++++--- .../src/airflow_breeze/utils/fork_ci_messages.py | 35 ++++++- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 37 +++++--- .../airflow_breeze/utils/provider_dependencies.py | 6 +- .../airflow_breeze/utils/workflow_run_approval.py | 72 +++++++++++--- dev/breeze/tests/test_fork_ci_messages.py | 29 +++++- dev/breeze/tests/test_fork_ci_reconcile.py | 34 +++++-- dev/breeze/tests/test_provider_dependencies.py | 11 +++ dev/breeze/tests/test_workflow_run_approval.py | 105 +++++++++++++++++++-- 14 files changed, 340 insertions(+), 86 deletions(-) diff --git a/contributing-docs/05_pull_requests.rst b/contributing-docs/05_pull_requests.rst index 2a4aaf1a04a..7320241b18c 100644 --- a/contributing-docs/05_pull_requests.rst +++ b/contributing-docs/05_pull_requests.rst @@ -252,10 +252,12 @@ and approves them automatically when all of these hold: * The PR targets ``main``. PRs to release branches, such as ``v3-4-test``, always wait for a maintainer. -* The PR does not change files that define CI: anything under ``.github/``, ``dev/breeze/``, - ``scripts/ci/``, ``scripts/docker/`` or ``scripts/in_container/``, a top-level ``Dockerfile*``, - or a ``.pre-commit-config.yaml`` in any directory. Those files define the CI workflows, what - their jobs run and the runners they use. +* The PR does not change files that decide which CI jobs run and on which runners: anything under + ``.github/``, ``dev/breeze/src/`` or ``scripts/ci/prek/``, or ``dev/breeze/pyproject.toml``, + ``dev/breeze/uv.lock`` or ``scripts/ci/install_breeze.sh``. Changes to the other files CI uses + (the rest of ``dev/breeze/`` and ``scripts/ci/``, ``scripts/docker/``, ``scripts/in_container/``, + ``Dockerfile*``, ``.pre-commit-config.yaml``) are not held, but a one-time comment on the PR + points them out to the reviewer. * The change does not need the full set of tests, does not change dependencies (``pyproject.toml``, lock files, ``package.json``, ``go.mod``, Gradle build files) and does not add a new provider. * The CI jobs your change selects are estimated to take at most 310 job-minutes, about a quarter of @@ -271,10 +273,12 @@ and approves them automatically when all of these hold: If your PR's CI runs in your fork (it carries the ``fork ci required`` label), the CI run in ``apache/airflow`` only checks that, so none of the rules above apply. The ``CI run started`` check waits until the fork CI monitor sees your fork's run, usually within 15 minutes, and then links it. Its runs are approved -automatically unless the PR changes files that define CI: anything under ``.github/``, -``dev/breeze/``, ``scripts/ci/``, ``scripts/docker/`` or ``scripts/in_container/``, a top-level -``Dockerfile*``, or a ``.pre-commit-config.yaml`` in any directory. Then the runs wait for a maintainer and a comment on the PR explains -why. CI in your fork runs either way. +automatically unless the PR changes files that decide which CI jobs run and on which runners: +anything under ``.github/``, ``dev/breeze/src/`` or ``scripts/ci/prek/``, or +``dev/breeze/pyproject.toml``, ``dev/breeze/uv.lock`` or ``scripts/ci/install_breeze.sh``. Then +the runs wait for a maintainer and a comment on the PR explains why. Changes to the other files +CI uses are not held, but a one-time comment points them out to the reviewer. CI in your fork +runs either way. The runs of PRs opened from the repositories of trusted stakeholders (``astronomer/airflow``, ``aws-mwaa/upstream-to-airflow`` and ``VladaZakharova/airflow``) are always approved, so none of diff --git a/contributing-docs/18_contribution_workflow.rst b/contributing-docs/18_contribution_workflow.rst index d49d732c5d2..4578e3cabe2 100644 --- a/contributing-docs/18_contribution_workflow.rst +++ b/contributing-docs/18_contribution_workflow.rst @@ -173,8 +173,8 @@ request explains the current state. Once the run is green, a pull request the mo marked ready for review and labelled ``ready for maintainer review``; a pull request you drafted yourself stays a draft, and gets the label, once you mark it ready. A pull request whose fork CI does not run for 7 days is closed with an explanation; you can reopen it after setting up fork CI. If your pull request changes -files that define what CI runs, such as ``.github/``, ``dev/breeze/`` or ``scripts/ci/``, a green -fork run is not enough: a maintainer reviews the change and runs project CI for it. +files that decide which CI jobs run and where, such as ``.github/``, the breeze sources or +``scripts/ci/prek/``, a green fork run is not enough: a maintainer reviews the change and runs project CI for it. **Joining the pilot.** Open a pull request that adds your GitHub login to the ``[pilot]`` table in ``.github/fork-ci-gate.toml``, with a join instant after the expected merge, for example the next day diff --git a/contributing-docs/25_maintainer_pr_triage.md b/contributing-docs/25_maintainer_pr_triage.md index ddbc647895d..333faefdd1f 100644 --- a/contributing-docs/25_maintainer_pr_triage.md +++ b/contributing-docs/25_maintainer_pr_triage.md @@ -260,7 +260,9 @@ for them, and the triage skill must treat those pull requests as out of scope fo removes it when a new push starts a new run. - **`needs project ci`.** The reconciler adds it when the fork run is green but the pull request changes CI-defining files, and removes it once the state is anything else. These pull requests - stay in draft and wait for a maintainer to review the CI change and add `use project ci`. + are not drafted and get no `ready for maintainer review` label. Their `Fork CI / Tests (AMD)` + status stays pending without `CI passed`, and they keep `needs project ci` until a maintainer + reviews the CI change and adds `use project ci`. - **Closing for changing the gate.** The reconciler closes a pull request that changes the fork CI gate (its code, configuration other than the author's own `[pilot]` entry, the `gate` job of `ci-amd.yml` or the monitor), and reports it on Slack. Add `use project ci` to accept such @@ -275,11 +277,13 @@ releases it on its next run; removing `fork ci required` by hand is not enough, labels a gated pull request again. Adding the label does not start project CI by itself, and the reconciler deletes the gated `Tests (AMD)` runs, so there is usually nothing to re-run: close and reopen the pull request (`gh pr close <n> -R apache/airflow && gh pr reopen <n> -R apache/airflow`), -or ask the author to push. Pull requests that change CI-defining files stay in draft even -when the fork run is green and are labelled `needs project ci`, for a maintainer to review and run -project CI with `use project ci`. When the fork run is green and trusted, the reconciler also posts -the `CI passed` status the merge ruleset requires, so these pull requests can be merged or -auto-merged without project CI. See +or ask the author to push. Pull requests that change CI-defining files are not drafted +and get no `ready for maintainer review` label even when the fork run is green. Their +`Fork CI / Tests (AMD)` status stays pending without `CI passed`, and they are labelled +`needs project ci` until a maintainer reviews the CI change and adds `use project ci`. For +every other pull request, when the fork run is green and trusted, the reconciler also posts the +`CI passed` status the merge ruleset requires, so it can be merged or auto-merged without project +CI. See [Fork CI for external contributors](../dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md). ## For contributors diff --git a/dev/breeze/doc/adr/0020-run-ci-for-external-contributions-in-forks.md b/dev/breeze/doc/adr/0020-run-ci-for-external-contributions-in-forks.md index 7c2b7a998a6..2b7ee1a2738 100644 --- a/dev/breeze/doc/adr/0020-run-ci-for-external-contributions-in-forks.md +++ b/dev/breeze/doc/adr/0020-run-ci-for-external-contributions-in-forks.md @@ -102,8 +102,9 @@ record captures the decisions that shape it and the alternatives rejected. ``.github/workflows/``, and legitimate workflow edits that keep the gate intact, are never closed. A pull request that is gated again after legitimate project CI runs and also edits workflows can still be closed while those runs are in the tampering window. A green fork run is trusted only when the pull request does not - change what fork CI runs (``.github/``, ``dev/breeze/``, ``scripts/ci/`` and the like); - otherwise it stays a draft until a maintainer runs project CI. A trusted green fork run also + change the ``build-info`` outputs that decide which jobs run and where (``.github/``, the + breeze sources, the breeze installation and ``scripts/ci/prek/``); otherwise it is labelled ``needs project ci`` + and cannot merge until a maintainer runs project CI. A trusted green fork run also gets a ``CI passed`` commit status, the check the merge ruleset requires, because a gated pull request never runs project CI to report it; it is only ever posted as success, and overwritten when the head commit is no longer green. Commit statuses were chosen over check diff --git a/dev/breeze/doc/ci/05_workflows.md b/dev/breeze/doc/ci/05_workflows.md index ebc42af64fc..8ea6810bb20 100644 --- a/dev/breeze/doc/ci/05_workflows.md +++ b/dev/breeze/doc/ci/05_workflows.md @@ -107,10 +107,12 @@ them, for the contributors the repository's approval setting covers. The - the PR targets `main`; PRs to release branches (`v3-4-test`, `airflow-ctl/v0-1-test`, ...) run those branches' own workflows and always wait for a maintainer -- the PR changes no file the fork CI reconciler counts as CI-defining (`.github/`, `dev/breeze/`, - `scripts/ci/`, `scripts/docker/`, `scripts/in_container/`, a top-level `Dockerfile*`, a - `.pre-commit-config.yaml` in any directory), - which define the workflows, what their jobs run and the runners they use +- the PR changes no file the fork CI reconciler counts as CI-defining (`.github/`, `dev/breeze/src/`, + `dev/breeze/pyproject.toml`, `dev/breeze/uv.lock`, `scripts/ci/install_breeze.sh` and + `scripts/ci/prek/`), which decide which jobs run and on which runners. Changes to the other files + CI uses (the rest of `dev/breeze/` and `scripts/ci/`, `scripts/docker/`, `scripts/in_container/`, + `Dockerfile*`, `.pre-commit-config.yaml`) are not held, but get a one-time + `<!-- workflow-approval: ci-modification -->` note comment on the PR for the reviewer - the PR changes no dependency manifest or lock file (`pyproject.toml`, `uv.lock`, `package.json`, `pnpm-lock.yaml`, `go.mod`, Gradle build files, ...) and adds no new provider (selective checks run from `main`'s tree and do not know it) diff --git a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index 781da587905..4627372f5fa 100644 --- a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +++ b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md @@ -380,9 +380,9 @@ created after it are approved this way, while earlier ones, whose gate ran proje maintainer. One rule applies instead: - The waiting runs of the head commit are approved when the pull request's merge commit changes no - file that the reconciler counts as CI-defining: `.github/`, `dev/breeze/`, `scripts/ci/`, - `scripts/docker/`, `scripts/in_container/`, `Dockerfile*` and `.pre-commit-config.yaml` (see - "Trusting a green fork run"). The approved `ci-amd.yml` run executes only the gate, and the other + file that the reconciler counts as CI-defining: `.github/`, `dev/breeze/src/`, + `dev/breeze/pyproject.toml`, `dev/breeze/uv.lock`, `scripts/ci/install_breeze.sh` and + `scripts/ci/prek/` (see "Trusting a green fork run"). The approved `ci-amd.yml` run executes only the gate, and the other `pull_request` workflows (CodeQL for the changed languages, the newsfragment check, dependency review) run on GitHub-hosted runners. - Otherwise the runs wait for a maintainer, its head commit gets a pending `Workflow approval` @@ -393,6 +393,12 @@ maintainer. One rule applies instead: workflow that does run, so such a run must not start without a review. The pull request does not get the `need workflow approval` label, and loses it if it has it. Approving its runs does not start project CI. A maintainer adds `use project ci` for that. +- A pull request that changes other files CI uses (the rest of `dev/breeze/` and `scripts/ci/`, + `scripts/docker/`, `scripts/in_container/`, `Dockerfile*` or `.pre-commit-config.yaml`) is not + held for them, but gets one comment listing them, so the reviewer checks the CI change with the + rest of the diff. It starts with the marker `<!-- workflow-approval: ci-modification -->` and is + posted once per pull request, for pull requests that run project CI as well. Like the rest of + this workflow, it only reaches pull requests whose runs waited for approval. A run started before `use fork ci` or `use project ci` was last added or removed is approved too, so a committer can add `use fork ci` after the PR's first runs were created and before they are @@ -532,7 +538,7 @@ pull requests without a fork run for their head commit, not by the total. | **running**: run in progress | unchanged (a rebase of an approved pull request does not draft it; a failed run does) | remove | `pending` (yellow), link to the run | update state and run link | | **failed**: conclusion is failure, cancelled or timed out | draft | remove | `failure` (red), link to the run | update with the run link | | **green**: conclusion success and the pull request does not change CI-defining files | undraft\* | add | `success` (green), link to the run | update: ready for review, run link | -| **green_unverified**: conclusion success, but the pull request changes CI-defining files (see "Trusting a green fork run") | draft | remove | `pending` (yellow), link to the run | update: a maintainer must run project CI; labelled `needs project ci` | +| **green_unverified**: conclusion success, but the pull request changes CI-defining files (see "Trusting a green fork run") | undraft\* | remove | `pending` (yellow), link to the run | update: a maintainer must run project CI; labelled `needs project ci` | | setup_required or awaiting_push for `close_after_days` | unchanged | unchanged | unchanged | close with a new comment; reopening restarts the clock | | **ungated**: the base branch has no gate, for example after retargeting to an older release branch | undraft\* | unchanged | `success` (green), link to the pull request's checks | update: project CI is in use; `fork ci required` is removed | | **gate_modified**: the pull request changes the fork CI gate (see "Gate protection") | draft | remove | `error` (red), link to the pull request's changed files | post a **new** comment explaining why, then close; checked after gate tampering, before the fork state | @@ -582,17 +588,26 @@ Rules that sit under the table: ### Trusting a green fork run -The fork run executes the workflow files, the breeze sources and the CI scripts of the pushed -commit. A pull request that changes them decides what its own fork CI checks, and could make it -pass trivially. When a new head commit turns green, the reconciler lists the pull request's +The fork run executes the workflow files and the breeze sources of the pushed commit, and the +`build-info` job decides from them which jobs run and on which runners. A pull request that +changes them decides what its own fork CI checks, and could make it pass trivially. CI-defining +files are only those that change the `build-info` outputs: the workflows and actions, the breeze +sources whose selective checks compute the outputs, the files that install breeze, and +`scripts/ci/prek/`. Selective checks runs `scripts/ci/prek/update_providers_dependencies.py` to +generate the gitignored `generated/provider_dependencies.json`, and the script runs with its +directory first on `sys.path`, so any module added there would run as well. Other CI +scripts, Dockerfiles and prek configuration change only what a selected job does, so they are +reviewed with the rest of the diff and do not hold the pull request back. When a new head commit turns green, the reconciler lists the pull request's changed files once (`GET /repos/apache/airflow/pulls/<n>/files`) and then reads the head commit again: the files endpoint lists the current head, so when the head moved meanwhile nothing is decided or recorded for the old commit, and the next run decides for the new one. If any file is under `.github/`, -`dev/breeze/`, `scripts/ci/`, `scripts/docker/` or `scripts/in_container/`, is a -`Dockerfile*` or a `.pre-commit-config.yaml`, or the list reaches GitHub's 3000-file cap so the -rest cannot be seen, the state is `green_unverified` instead of `green`. The pull request stays -a draft, the status is pending, and it is labelled `needs project ci` so maintainers can list -these pull requests, review the change and add `use project ci`. It is not posted to Slack: +`dev/breeze/src/` or `scripts/ci/prek/`, is `dev/breeze/pyproject.toml`, `dev/breeze/uv.lock` or +`scripts/ci/install_breeze.sh`, or the list reaches GitHub's 3000-file cap so the rest cannot be +seen, the state is `green_unverified` instead of `green`. The pull request is not drafted (a +draft the reconciler made earlier is undone, as for `green`), but it gets no review label, the +status is pending, and it is labelled `needs project ci` so maintainers can list these pull +requests, review the change and add `use project ci`. Without `CI passed` it cannot be merged +until then. It is not posted to Slack: the label already lists these pull requests, and a post for every new green commit would bury the CI breakages that channel is for. The reconciler removes `needs project ci` as soon as the state is anything else. A renamed file @@ -1243,7 +1258,8 @@ as a failed check rather than a crash. the conflicting-labels alert, the close clock reset (including a reopen after an inactivity close), a fork with a name other than `airflow`, marker round-trips, status desired-vs-current comparison, the unknown-failure collection, green trust (a change to - CI-defining files keeps the pull request a draft, decided once per commit) and the gate + CI-defining files keeps the status pending without drafting the pull request, decided once + per commit) and the gate tampering check: a tampered run on an earlier commit is caught after a clean push, each signal alone does not close, short runs and gate-only jobs do not count, an in-progress non-gate job, a removed, duplicated or long-running gate job and an injected workflow with a diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py index 6fb8d04aac5..10cca162083 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py @@ -155,9 +155,10 @@ _STATE_TEXTS = { "green": "**Fork CI: green.** CI for the latest commit passed in your fork. The pull request is " "ready for maintainer review.", "green_unverified": "**Fork CI: green, needs a maintainer.** CI for the latest commit passed in your " - "fork, but this pull request changes files that define what CI runs (such as `.github/`, " - "`dev/breeze/` or `scripts/ci/`), and the fork run used those changed files. A maintainer will " - "review the change and run project CI for it; the pull request stays a draft until then.", + "fork, but this pull request changes files that decide which CI jobs run and where (`.github/`, " + "the breeze sources, the breeze installation or `scripts/ci/prek/`), and the fork run used those " + "changed files. A maintainer will review the change and run project CI for it before it can be " + "merged.", "closed": "**Fork CI: closed after inactivity.** CI did not run in your fork in time; see the " "comment below for how to continue.", "gate_bypassed": "**Fork CI: closed.** Project CI ran for this pull request although its CI runs in " @@ -275,22 +276,46 @@ def _sanitize_path(path: str) -> str: return "".join(char if char.isprintable() and char != "`" else "?" for char in path) -def ci_change_approval_comment(*, ci_files: list[str]) -> str: +def build_ci_file_list(ci_files: list[str]) -> str: listed = [f"- `{_sanitize_path(path)}`" for path in ci_files[:MAX_LISTED_CI_FILES]] if len(ci_files) > MAX_LISTED_CI_FILES: listed.append(f"- and {len(ci_files) - MAX_LISTED_CI_FILES} more") + return "\n".join(listed) + + +def ci_change_approval_comment(*, ci_files: list[str]) -> str: return ( f"{CI_CHANGE_MARKER}\n\n" "**Your CI change needs a maintainer.** CI for this pull request runs in your fork, and the " "workflow runs it starts in apache/airflow are approved automatically while they only run the " "fork CI gate. This pull request changes files that define what CI runs:\n\n" - + "\n".join(listed) + + build_ci_file_list(ci_files) + "\n\nSuch a change can make project CI run on the project's runners, so its workflow runs in " "apache/airflow wait for a maintainer to review the change and approve them. CI in your fork " "is not affected, and its result stays visible on this pull request. " + WHY_FORK_CI + FOOTER ) +CI_MODIFICATION_MARKER = "<!-- workflow-approval: ci-modification -->" + + +GATED_NOTE_SENTENCE = ( + " CI for this pull request runs in the author's fork, so any green result and `CI passed` status will " + "come from a run that executed these changed files." +) + + +def ci_modification_note_comment(*, ci_files: list[str], gated: bool) -> str: + return ( + f"{CI_MODIFICATION_MARKER}\n\n" + "**This pull request changes CI files.** It changes files that CI uses:\n\n" + + build_ci_file_list(ci_files) + + "\n\nThey do not decide which CI jobs run or on which runners, so its workflow runs are not " + "held for this. A maintainer reviewing the pull request will check the CI change together with " + "the rest of it." + (GATED_NOTE_SENTENCE if gated else "") + FOOTER + ) + + _STATUS_DESCRIPTIONS = { "setup_required": "Fork CI not set up - see the pull request comment", "awaiting_push": "Fork CI enabled, waiting for a push - see the pull request comment", diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py index 845638b3ca2..fc316385adf 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -134,17 +134,26 @@ TAMPER_WINDOW = timedelta(hours=6) # can have done project CI work and are worth listing jobs for. SUSPICIOUS_RUN_DURATION = timedelta(minutes=5) GATE_MAX_DURATION = timedelta(minutes=5) -# Paths whose content defines what fork CI runs. A pull request changing them controls its own -# fork CI result, so a green fork run cannot be trusted for it. +# Paths that decide the build-info outputs (which jobs run and on which runners): the workflows +# and actions, the breeze sources whose selective checks compute those outputs, the files that +# install breeze, and scripts/ci/prek/, which holds the script selective checks runs to generate +# the gitignored generated/provider_dependencies.json. That script runs with its directory first +# on sys.path, so any module added there, such as a `yaml.py`, would run too. A pull request +# changing them controls its own fork CI result and could change where project CI runs. Other CI +# scripts, Dockerfiles and prek configuration only change what a selected job does, which review +# of the diff covers like any other change. CI_DEFINING_PREFIXES = ( ".github/", - "dev/breeze/", - "scripts/ci/", - "scripts/docker/", - "scripts/in_container/", - "Dockerfile", + "dev/breeze/src/", + "scripts/ci/prek/", +) +CI_DEFINING_FILES = frozenset( + { + "dev/breeze/pyproject.toml", + "dev/breeze/uv.lock", + "scripts/ci/install_breeze.sh", + } ) -CI_DEFINING_SUFFIXES = (".pre-commit-config.yaml",) PULL_REQUEST_FILES_CAP = 3000 COMPARE_FILES_CAP = 300 SLACK_SECTION_LIMIT = 3000 @@ -467,7 +476,7 @@ def changes_ci_definition(files: Sequence[str], *, file_count: int | None = None return ( (len(files) if file_count is None else file_count) >= PULL_REQUEST_FILES_CAP or touches(files, CI_DEFINING_PREFIXES) - or any(path.endswith(CI_DEFINING_SUFFIXES) for path in files) + or any(path in CI_DEFINING_FILES for path in files) ) @@ -759,12 +768,16 @@ def classify( plan.close = True return plan - if state == ForkState.GREEN: + if state in (ForkState.GREEN, ForkState.GREEN_UNVERIFIED): + # A green run that changes CI files still needs a maintainer's review, which a draft would + # hide; the `needs project ci` label marks it instead of the review label. if pr.is_draft and _is_bot(pr.last_draft_actor): plan.draft = False ready = not pr.is_draft or plan.draft is False - if ready and LABEL_READY_FOR_REVIEW not in labels: + if state == ForkState.GREEN and ready and LABEL_READY_FOR_REVIEW not in labels: plan.add_labels.append(LABEL_READY_FOR_REVIEW) + elif state == ForkState.GREEN_UNVERIFIED and LABEL_READY_FOR_REVIEW in labels: + plan.remove_labels.append(LABEL_READY_FOR_REVIEW) else: # A run in progress needs nothing from the author, and drafting on it would draft every # approved pull request the moment it is rebased; it is drafted only if the run then fails. @@ -773,7 +786,7 @@ def classify( if LABEL_READY_FOR_REVIEW in labels: plan.remove_labels.append(LABEL_READY_FOR_REVIEW) # Lets maintainers list the pull requests that only wait for them to run project CI, which - # otherwise look like any other draft with a pending fork CI status. + # otherwise look like any other pull request with a pending fork CI status. _set_label(plan, labels, LABEL_NEEDS_PROJECT_CI, present=state == ForkState.GREEN_UNVERIFIED) target_url = fork.run_url or fork_actions_url(pr.head_repo) diff --git a/dev/breeze/src/airflow_breeze/utils/provider_dependencies.py b/dev/breeze/src/airflow_breeze/utils/provider_dependencies.py index 201f43c97b7..2a878587d10 100644 --- a/dev/breeze/src/airflow_breeze/utils/provider_dependencies.py +++ b/dev/breeze/src/airflow_breeze/utils/provider_dependencies.py @@ -89,8 +89,12 @@ def regenerate_provider_dependencies_once() -> None: with _regenerate_provider_deps_lock: # Run the regeneration command from the repository root to ensure correct # relative paths if the script expects to be run from AIRFLOW_ROOT_PATH. + # CI runs this while deciding which jobs run, from the pull request's checkout: --no-config + # keeps a `[tool.uv]` index the pull request adds to pyproject.toml or uv.toml from choosing + # where the script's dependencies come from. subprocess.check_call( - ["uv", "run", UPDATE_PROVIDER_DEPENDENCIES_SCRIPT.as_posix()], cwd=AIRFLOW_ROOT_PATH + ["uv", "run", "--no-config", UPDATE_PROVIDER_DEPENDENCIES_SCRIPT.as_posix()], + cwd=AIRFLOW_ROOT_PATH, ) diff --git a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py index 2dba4b3842a..40a72eba97e 100644 --- a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py +++ b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py @@ -60,7 +60,12 @@ from airflow_breeze.utils.fork_ci_gate import ( load_config, parse_github_datetime, ) -from airflow_breeze.utils.fork_ci_messages import CI_CHANGE_MARKER, ci_change_approval_comment +from airflow_breeze.utils.fork_ci_messages import ( + CI_CHANGE_MARKER, + CI_MODIFICATION_MARKER, + ci_change_approval_comment, + ci_modification_note_comment, +) from airflow_breeze.utils.fork_ci_reconcile import changes_ci_definition from airflow_breeze.utils.path_utils import AIRFLOW_ROOT_PATH from airflow_breeze.utils.run_utils import run_command @@ -170,6 +175,8 @@ class ApprovalDecision: comment: str | None = None # Whether a PR whose runs are left for a maintainer gets the ``need workflow approval`` label. add_approval_label: bool = True + # Posted on the PR once whatever the decision, unless ``comment`` is posted instead. + note: str | None = None # The labels the decision depends on: runs started before one of them changed are not approved. decision_labels: frozenset[str] = TEST_SELECTION_LABELS # Runs started before this are not approved either. @@ -236,6 +243,27 @@ def decide_trusted_approval(*, stakeholder: str) -> ApprovalDecision: ) +# Files CI uses that do not decide the build-info outputs: a change to them is reviewed with the rest +# of the pull request, and only noted on it. +CI_MODIFYING_PREFIXES = ( + ".github/", + "dev/breeze/", + "scripts/ci/", + "scripts/docker/", + "scripts/in_container/", + "Dockerfile", +) +CI_MODIFYING_SUFFIXES = (".pre-commit-config.yaml",) + + +def find_ci_modifying_files(files: list[str]) -> list[str]: + return [ + file + for file in files + if file.startswith(CI_MODIFYING_PREFIXES) or file.endswith(CI_MODIFYING_SUFFIXES) + ] + + def find_fork_ci_definition_files(files: list[str]) -> list[str]: """The files the fork CI reconciler counts as defining CI, so both decide alike.""" return [file for file in files if changes_ci_definition([file], file_count=1)] @@ -292,8 +320,8 @@ def decide_approval( ) paths = [file.path for file in changed_files] # CI definition files can change which runner a job uses (``runs-on:``, ``runner-type`` - # computed by selective checks, the scripts build-info runs) or what the jobs run, so they - # always need a maintainer's look. + # computed by selective checks, the breeze installation build-info runs) or which jobs run, + # so they always need a maintainer's look. ci_files = find_fork_ci_definition_files(paths) if ci_files: return ApprovalDecision( @@ -690,6 +718,7 @@ def process_waiting_runs(api: GitHubApi, *, created_since: datetime) -> list[Pul "or changed a label its decision depends on" ], estimate=decision.estimate, + note=decision.note, ) pr_run_ids = run_ids if decision.approve: @@ -793,18 +822,24 @@ def decide_for_pull_request( ): decision = decide_gated_approval(changed_files=changed_files) decision.not_before = joined_at - return decision - pr_labels = tuple(label["name"] for label in pr.get("labels", [])) - return decide_approval( - base_branch=pr["base"]["ref"], - changed_files=changed_files, - selective_checks=build_selective_checks( - merge_sha=merge_sha, - changed_files=changed_files, - pr_labels=pr_labels, + gated = True + else: + gated = False + pr_labels = tuple(label["name"] for label in pr.get("labels", [])) + decision = decide_approval( base_branch=pr["base"]["ref"], - ), - ) + changed_files=changed_files, + selective_checks=build_selective_checks( + merge_sha=merge_sha, + changed_files=changed_files, + pr_labels=pr_labels, + base_branch=pr["base"]["ref"], + ), + ) + paths = [file.path for file in changed_files] + if not find_fork_ci_definition_files(paths) and (ci_files := find_ci_modifying_files(paths)): + decision.note = ci_modification_note_comment(ci_files=ci_files, gated=gated) + return decision def build_failure_reason(error: Exception) -> str: @@ -872,6 +907,7 @@ def apply_decision( api.approve_run(run_id) if has_label: api.remove_label(pr_number, NEED_WORKFLOW_APPROVAL_LABEL) + post_note(api, pr_number=pr_number, decision=decision) return # Reasons can contain contributor-chosen file names, which must not be read as Rich markup. console_print(f"[warning]PR #{pr_number} needs a maintainer: {escape('; '.join(decision.reasons))}[/]") @@ -887,10 +923,18 @@ def apply_decision( description=build_status_description("Waits for a maintainer", decision.reasons), ) if (label_added or status_changed) and clear_if_runs_were_approved(api, pr=pr, run_ids=run_ids): + post_note(api, pr_number=pr_number, decision=decision) return # Once per PR: every later push would otherwise repeat it while the change stays. if decision.comment is not None and not api.has_bot_comment(pr_number, CI_CHANGE_MARKER): api.create_comment(pr_number, decision.comment) + post_note(api, pr_number=pr_number, decision=decision) + + +def post_note(api: GitHubApi, *, pr_number: int, decision: ApprovalDecision) -> None: + """Post the decision's note once per PR; every later push would otherwise repeat it.""" + if decision.note is not None and not api.has_bot_comment(pr_number, CI_MODIFICATION_MARKER): + api.create_comment(pr_number, decision.note) MARKDOWN_SPECIAL_CHARACTERS = re.compile(r"([\\`*_{}\[\]()<>#+!|~])") diff --git a/dev/breeze/tests/test_fork_ci_messages.py b/dev/breeze/tests/test_fork_ci_messages.py index f7c205c0973..6bf940c69df 100644 --- a/dev/breeze/tests/test_fork_ci_messages.py +++ b/dev/breeze/tests/test_fork_ci_messages.py @@ -16,6 +16,7 @@ # under the License. from __future__ import annotations +import functools from dataclasses import replace from datetime import UTC, datetime @@ -26,14 +27,17 @@ from airflow_breeze.utils.fork_ci_messages import ( _STATUS_DESCRIPTIONS, BEHIND_WARNING_COMMITS, CI_CHANGE_MARKER, + CI_MODIFICATION_MARKER, CI_PASSED_DESCRIPTION, CI_PASSED_REVOKED_DESCRIPTION, FOOTER, + GATED_NOTE_SENTENCE, JOB_NAME_MAX_LENGTH, JOB_STATUS_CONTEXT_PREFIX, MAX_LISTED_CI_FILES, Marker, ci_change_approval_comment, + ci_modification_note_comment, gate_modified_close_comment, inactivity_close_comment, job_status_context, @@ -159,16 +163,31 @@ def test_job_status_context(name, expected): assert job_status_context(name) == f"{JOB_STATUS_CONTEXT_PREFIX}{expected}" -def test_ci_change_approval_comment_lists_the_files(): [email protected]( + ("build_comment", "marker"), + [ + pytest.param(ci_change_approval_comment, CI_CHANGE_MARKER, id="approval"), + pytest.param(ci_modification_note_comment, CI_MODIFICATION_MARKER, id="note"), + ], +) +def test_ci_file_comments_list_the_files(build_comment, marker): + if build_comment is ci_modification_note_comment: + build_comment = functools.partial(build_comment, gated=False) paths = [f".github/workflows/w{i}.yml" for i in range(MAX_LISTED_CI_FILES + 2)] - comment = ci_change_approval_comment( - ci_files=["dev/breeze/`evil`.py", ".github/a\n\[email protected]", *paths] - ) + comment = build_comment(ci_files=["dev/breeze/`evil`.py", ".github/a\n\[email protected]", *paths]) - assert comment.startswith(CI_CHANGE_MARKER) + assert comment.startswith(marker) assert comment.endswith(FOOTER) assert "- `dev/breeze/?evil?.py`" in comment assert "- `.github/[email protected]`" in comment assert "\n@someone" not in comment assert "- and 4 more" in comment assert f"`{paths[-1]}`" not in comment + + [email protected]("gated", [True, False]) +def test_ci_modification_note_mentions_fork_execution_only_when_gated(gated): + comment = ci_modification_note_comment(ci_files=["Dockerfile.ci"], gated=gated) + + assert (GATED_NOTE_SENTENCE in comment) is gated + assert comment.endswith(FOOTER) diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 9a624b1005a..a26a3cd21fc 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -277,7 +277,7 @@ def test_green_is_not_trusted_when_ci_files_change(): plan, _, changes = _classify(pr, GREEN, changes_ci_files=True) assert plan.state == ForkState.GREEN_UNVERIFIED - assert plan.draft is True + assert plan.draft is None assert plan.add_labels == ["needs project ci"] assert plan.remove_labels == ["ready for maintainer review"] assert plan.status == ( @@ -290,6 +290,21 @@ def test_green_is_not_trusted_when_ci_files_change(): changes.assert_called_once_with() [email protected]( + ("last_draft_actor", "expected_draft"), + [ + pytest.param("github-actions[bot]", False, id="bot-draft-undrafted"), + pytest.param("someone", None, id="author-draft-kept"), + ], +) +def test_green_unverified_does_not_keep_a_bot_draft(last_draft_actor, expected_draft): + plan, _, _ = _classify(_pr(last_draft_actor=last_draft_actor), GREEN, changes_ci_files=True) + + assert plan.state == ForkState.GREEN_UNVERIFIED + assert plan.draft is expected_draft + assert plan.add_labels == ["needs project ci"] + + LABELLED = ("fork ci required", "needs project ci") @@ -474,11 +489,18 @@ def test_find_heads_shared_across_bases(): pytest.param([".github/workflows/ci-amd.yml"], True, id="workflow"), pytest.param([".github/actions/breeze/action.yml"], True, id="action"), pytest.param(["dev/breeze/src/airflow_breeze/utils/selective_checks.py"], True, id="breeze"), - pytest.param(["scripts/ci/prek/check_x.py"], True, id="ci-scripts"), - pytest.param(["Dockerfile.ci"], True, id="dockerfile"), - pytest.param(["scripts/in_container/run_ci_tests.sh"], True, id="in-container-scripts"), - pytest.param([".pre-commit-config.yaml"], True, id="root-prek-config"), - pytest.param(["dev/.pre-commit-config.yaml"], True, id="nested-prek-config"), + pytest.param(["dev/breeze/pyproject.toml"], True, id="breeze-project"), + pytest.param(["dev/breeze/uv.lock"], True, id="breeze-lock"), + pytest.param(["scripts/ci/install_breeze.sh"], True, id="breeze-install"), + pytest.param(["scripts/ci/prek/update_providers_dependencies.py"], True, id="provider-deps-script"), + pytest.param(["scripts/ci/prek/yaml.py"], True, id="module-next-to-provider-deps-script"), + pytest.param(["dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md"], False, id="breeze-doc"), + pytest.param(["dev/breeze/tests/test_selective_checks.py"], False, id="breeze-tests"), + pytest.param(["scripts/ci/testing/run_unit_tests.sh"], False, id="ci-scripts"), + pytest.param(["Dockerfile.ci"], False, id="dockerfile"), + pytest.param(["scripts/docker/entrypoint_ci.sh"], False, id="docker-scripts"), + pytest.param(["scripts/in_container/run_ci_tests.sh"], False, id="in-container-scripts"), + pytest.param([".pre-commit-config.yaml"], False, id="prek-config"), pytest.param([f"airflow-core/f{i}.py" for i in range(3000)], True, id="too-many-files-to-list"), ], ) diff --git a/dev/breeze/tests/test_provider_dependencies.py b/dev/breeze/tests/test_provider_dependencies.py index bf77f6dd29d..a1aaa8650d8 100644 --- a/dev/breeze/tests/test_provider_dependencies.py +++ b/dev/breeze/tests/test_provider_dependencies.py @@ -128,3 +128,14 @@ def test_hash_not_written_when_regeneration_fails(mock_hash, mock_regenerate, pr generate_provider_dependencies_if_needed() assert not hash_path.exists() + + [email protected](provider_dependencies_module.subprocess, "check_call", autospec=True) +def test_regenerate_provider_dependencies_ignores_uv_config_from_the_checkout(mock_check_call): + provider_dependencies_module.regenerate_provider_dependencies_once.cache_clear() + try: + provider_dependencies_module.regenerate_provider_dependencies_once() + finally: + provider_dependencies_module.regenerate_provider_dependencies_once.cache_clear() + + assert mock_check_call.call_args.args[0][:3] == ["uv", "run", "--no-config"] diff --git a/dev/breeze/tests/test_workflow_run_approval.py b/dev/breeze/tests/test_workflow_run_approval.py index 86c9511cc74..f1c9c4b47a2 100644 --- a/dev/breeze/tests/test_workflow_run_approval.py +++ b/dev/breeze/tests/test_workflow_run_approval.py @@ -27,7 +27,11 @@ from click.testing import CliRunner from airflow_breeze.commands.ci_commands import approve_workflow_runs from airflow_breeze.utils.ci_minutes_estimate import CiMinutesEstimate, EstimatedJobGroup from airflow_breeze.utils.fork_ci_gate import GateConfig -from airflow_breeze.utils.fork_ci_messages import CI_CHANGE_MARKER +from airflow_breeze.utils.fork_ci_messages import ( + CI_CHANGE_MARKER, + CI_MODIFICATION_MARKER, + GATED_NOTE_SENTENCE, +) from airflow_breeze.utils.selective_checks import SelectiveChecks from airflow_breeze.utils.shared_options import set_dry_run from airflow_breeze.utils.workflow_run_approval import ( @@ -146,11 +150,9 @@ def mock_estimate(): id=f"fork-ci-defining-{path}", ) for path in ( - ".pre-commit-config.yaml", - "airflow-core/.pre-commit-config.yaml", - "Dockerfile.ci", - "scripts/docker/entrypoint_ci.sh", - "scripts/in_container/run_ci_tests.sh", + "dev/breeze/pyproject.toml", + "dev/breeze/uv.lock", + "scripts/ci/install_breeze.sh", ) ), pytest.param( @@ -887,9 +889,9 @@ def _gated_pr(*, base_ref="main", author_association="CONTRIBUTOR", labels=()) - id="workflow change", ), pytest.param( - [ChangedFile("scripts/docker/entrypoint_ci.sh", 1)], + [ChangedFile("scripts/ci/install_breeze.sh", 1)], False, - ["scripts/docker/entrypoint_ci.sh"], + ["scripts/ci/install_breeze.sh"], id="file the fork CI reconciler counts as CI", ), ], @@ -956,6 +958,75 @@ def test_decide_for_pull_request_running_project_ci_keeps_the_size_rules( assert decision is mock_decide_approval.return_value [email protected]( + ("changed_files", "noted_files"), + [ + pytest.param(CORE_CHANGE, [], id="no CI file"), + pytest.param( + [ChangedFile("scripts/docker/entrypoint_ci.sh", 1), ChangedFile("Dockerfile.ci", 1)], + ["scripts/docker/entrypoint_ci.sh", "Dockerfile.ci"], + id="CI files outside build-info", + ), + pytest.param( + [ChangedFile("dev/breeze/doc/ci/04_selective_checks.md", 1)], + ["dev/breeze/doc/ci/04_selective_checks.md"], + id="breeze doc", + ), + pytest.param( + [ChangedFile(".github/workflows/ci-amd.yml", 1), ChangedFile("Dockerfile.ci", 1)], + [], + id="build-info change gets the approval comment instead", + ), + ], +) [email protected]("gated", [True, False]) [email protected](f"{MODULE}.decide_approval", autospec=True) [email protected](f"{MODULE}.build_selective_checks", autospec=True) [email protected](f"{MODULE}.load_config", autospec=True) [email protected](f"{MODULE}.get_changed_files", autospec=True) [email protected](f"{MODULE}.fetch_merge_commit", autospec=True, return_value="merge") +def test_decide_for_pull_request_notes_ci_files_that_do_not_hold_the_runs( + mock_fetch, + mock_get_changed_files, + mock_load_config, + mock_selective_checks, + mock_decide_approval, + gated, + changed_files, + noted_files, +): + mock_get_changed_files.return_value = changed_files + mock_load_config.return_value = GATE_ENABLED if gated else GateConfig() + mock_decide_approval.return_value = ApprovalDecision(approve=True) + api = mock.MagicMock(spec=GitHubApi) + api.github_repository = "apache/airflow" + + decision = decide_for_pull_request(api=api, pr=_gated_pr()) + + if noted_files: + assert decision.note is not None + assert decision.note.startswith(CI_MODIFICATION_MARKER) + assert all(f"`{path}`" in decision.note for path in noted_files) + assert (GATED_NOTE_SENTENCE in decision.note) is gated + else: + assert decision.note is None + + [email protected]("approve", [True, False]) [email protected](("already_noted", "expect_note"), [(False, True), (True, False)]) +def test_apply_decision_posts_the_note_once(approve, already_noted, expect_note): + api = mock.MagicMock(spec=GitHubApi) + api.has_bot_comment.return_value = already_noted + api.get_approval_status.return_value = None + api.is_run_waiting_for_approval.return_value = True + decision = ApprovalDecision(approve=approve, reasons=["reason"], note="note") + + apply_decision(api=api, pr=_pr(10, "aaa"), run_ids=[1], decision=decision) + + api.has_bot_comment.assert_called_once_with(10, CI_MODIFICATION_MARKER) + assert api.create_comment.call_args_list == ([mock.call(10, "note")] if expect_note else []) + + @pytest.mark.parametrize(("already_commented", "expect_comment"), [(False, True), (True, False)]) def test_apply_decision_comments_once_on_a_ci_change(already_commented, expect_comment): api = mock.MagicMock(spec=GitHubApi) @@ -1019,6 +1090,24 @@ def test_process_waiting_runs_skips_runs_started_before_a_test_selection_label_c assert api.approve_run.call_args_list == [mock.call(run_id) for run_id in approved_runs] [email protected](f"{MODULE}.decide_for_pull_request", autospec=True) +def test_process_waiting_runs_keeps_the_note_when_every_run_started_before_a_label_changed(mock_decide): + api = mock.MagicMock(spec=GitHubApi) + api.github_repository = "apache/airflow" + api.get_runs_waiting_for_approval.return_value = [_run(2, "aaa", created_at="2026-10-22T10:00:00Z")] + api.get_open_pull_requests.return_value = [_pr(10, "aaa")] + api.get_last_base_change.return_value = None + api.get_last_label_change.return_value = datetime(2026, 10, 22, 12, 0, tzinfo=UTC) + api.has_bot_comment.return_value = False + api.get_approval_status.return_value = None + mock_decide.return_value = ApprovalDecision(approve=True, reasons=["cheap"], note="note") + + process_waiting_runs(api, created_since=CREATED_SINCE) + + api.approve_run.assert_not_called() + assert api.create_comment.call_args_list == [mock.call(10, "note")] + + def test_get_last_label_change_ignores_other_labels_and_reads_the_timeline_once(): api = GitHubApi(github_repository="apache/airflow", github_token="token", report_only=True) events = [
