This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-gate-narrow-ci-defining in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 1c9bd02562e804011907f36147815cbe2cc7566c Author: Jarek Potiuk <[email protected]> AuthorDate: Mon Nov 2 07:37:32 2026 +0100 Hold back only build-info changes in fork CI and stop drafting them 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 --- ...0-run-ci-for-external-contributions-in-forks.md | 5 +- .../doc/ci/08_fork_ci_for_external_contributors.md | 41 +++++++---- .../src/airflow_breeze/utils/fork_ci_messages.py | 28 ++++++-- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 34 ++++++--- .../airflow_breeze/utils/provider_dependencies.py | 6 +- .../airflow_breeze/utils/workflow_run_approval.py | 69 ++++++++++++++---- dev/breeze/tests/test_fork_ci_messages.py | 17 +++-- 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 | 82 +++++++++++++++++++--- 10 files changed, 262 insertions(+), 65 deletions(-) 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..ac3873ffc58 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 and the breeze installation); 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/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index e4efe527cd3..be92ce3626a 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, the pull request is labelled `need workflow approval`, @@ -392,6 +392,11 @@ maintainer. One rule applies instead: `<!-- workflow-approval: ci-change -->` and is posted once per pull request; only comments by `github-actions[bot]` with the marker count. A change to `.github/` can remove the gate or add a workflow that does run, so such a run must not start without a review. +- 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. 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 @@ -531,7 +536,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 | @@ -581,17 +586,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 @@ -1237,7 +1251,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..63b95e0b215 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,9 @@ _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 or the breeze installation), 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 +275,40 @@ 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 -->" + + +def ci_modification_note_comment(*, ci_files: list[str]) -> 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." + 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 80dd7242a3e..347476551d3 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,25 @@ 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 @@ -457,7 +465,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) ) @@ -749,12 +757,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. 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 d32d0455f1b..95e9d5efced 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 @@ -168,6 +173,8 @@ class ApprovalDecision: estimate: CiMinutesEstimate | None = None # Posted on the PR once, when the runs are left for a maintainer. comment: str | None = None + # 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. @@ -234,6 +241,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)] @@ -288,8 +316,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( @@ -789,18 +817,22 @@ 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, + else: + 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) + return decision def build_failure_reason(error: Exception) -> str: @@ -868,6 +900,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))}[/]") @@ -880,10 +913,18 @@ def apply_decision( description=build_status_description("Waits for a maintainer", decision.reasons), ) if (not has_label 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..069c5c65b55 100644 --- a/dev/breeze/tests/test_fork_ci_messages.py +++ b/dev/breeze/tests/test_fork_ci_messages.py @@ -26,6 +26,7 @@ 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, @@ -34,6 +35,7 @@ from airflow_breeze.utils.fork_ci_messages import ( MAX_LISTED_CI_FILES, Marker, ci_change_approval_comment, + ci_modification_note_comment, gate_modified_close_comment, inactivity_close_comment, job_status_context, @@ -159,13 +161,18 @@ 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): 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 diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 417a08540af..b74bd33f222 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -275,7 +275,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 == ( @@ -288,6 +288,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") @@ -472,11 +487,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 6aa5d85d5ee..9ef4aad86f5 100644 --- a/dev/breeze/tests/test_workflow_run_approval.py +++ b/dev/breeze/tests/test_workflow_run_approval.py @@ -27,7 +27,7 @@ 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 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 +146,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( @@ -866,9 +864,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", ), ], @@ -934,6 +932,74 @@ 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) + 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)
