Repository navigation
Add breeze verify to list local checks for changed files - #73241
Conversation
328167e to
9938a3f
Compare
Andrushika
changed the title
|
Hi @potiuk when you have a chance, could you please take a look on this? |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — this is useful, and it's built the right way: it reuses SelectiveChecks rather than re-deriving anything, keeps list-only scope for this PR, and the two drift guards (every run_* flag must be classified; the core/providers flags must match run_unit_tests.sh) are exactly what stops a second CI mapping from rotting silently. I checked the hard-coded commands against the workflows (mypy-providers / migration-round-trip match ci-amd.yml, python-api-client-tests and the --sdk-docs-only flags exist), and the --no-renames changed-file logic matches how CI lists files. Approving.
I left a few inline comments — could you address them and resolve the threads before this is merged?
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
1a3bbcd to
535a961
Compare
7ea9dc5 to
9f59aad
Compare
9f59aad to
63b0aba
Compare
|
One limitation: the classification test guard treats The problem is that a new gate with another name would bypass our drift guard. I considered deriving this from the workflow YAML, but parsing yaml itself seems not a reliable way since there are too much edge cases. We can still merge this PR first. If we agree that a stronger naming contract is needed, I would prefer to discuss the design and handle it in a follow-up PR. |
|
Exception: I don't think |
|
Mostly agree. I was just thinking of creating a common command that lets contributors run all the local checks at once -- including all the commands our contribution docs ask them to run. |
|
I’ve also considered if we still need to include the manual stage prek hooks, because they won’t be triggered by prek's own file config. However, I don’t think we need include them either since they are quite "heavy" (one of them would spend over 10 minutes, most developers don't run it locally) The downside is that a failure there only shows up in CI, but I think that's totally okay. Just make sure this command really meets developers' conventions |
…files The self-review checklist in AGENTS.md (step 7) asks contributors to find the tests selective-checks would run and run them locally. The only tool for that is `breeze ci selective-check`, which looks at a single commit and prints GitHub Actions outputs, so a contributor still has to read the workflow files to turn those outputs into commands. This adds a developer-facing, dry-run command that feeds the working-tree diff to the same SelectiveChecks logic and prints the commands for the default CI matrix cell, with the rest explicitly left to CI.
…ays parseable In GitHub Actions breeze appends a "how to reproduce locally" banner after every command. It went to stdout, so any command whose stdout is a machine contract, like `breeze verify --json`, produced JSON followed by the banner. The banner is a log aid, so stderr is the right stream for it.
… moving it to stderr Moving the banner to stderr broke the build-info job: `breeze ci get-workflow-info` and `breeze ci selective-check` write their GitHub Actions outputs on stderr (`2>> $GITHUB_OUTPUT`), so the banner ended up in the outputs file. Neither stream is safe for every command. Keep the banner on stdout and let a command whose stdout is a machine contract, here `breeze verify --json`, switch it off through the click context.
…ed files CI feeds selective checks the output of `git diff-tree`, which has no rename detection and reports a rename as the old path deleted plus the new path added. `git diff --name-only` detects renames by default and lists only the new path, so a rule matching the old path would not fire locally although it fires in CI.
Breeze's own unit tests, the shared distribution tests and the translation check run on every PR regardless of what changed, so listing them on every run adds rows nobody acts on and pushes the relevant ones down. Now every row comes from a SelectiveChecks output. The `required` column goes with them: with the translation check gone it was always true.
…ion behind --full When a change touches CI tooling or dependency files, selective checks make CI run the whole suite. That is the right call for CI, but an agent that executes the list literally would run Go, Java and every provider for a breeze change. The default now applies the same selective-checks rules without that expansion, notes that CI will run more, and --full lists the whole default matrix cell.
A truncated command cannot be copied, and the prek row with its SKIP list is longer than most terminals.
A table cell is not something you can copy: long commands got truncated, and folding them only spread one command over several lines. Each command is now on its own unwrapped line under a comment saying what kind of check it is and where it runs, so a triple-click selects the whole command.
Reverts the one-command-per-line output. The table is easier to read, and anyone who wants to copy commands has --json.
--full means everything CI runs for the default matrix cell, so breeze's own unit tests and the shared distribution tests belong there even though no change triggers them. The default list still leaves them out. The translation check stays out of both: CI runs it with `|| true`.
…ents at breeze verify The manual-stage hook names in the mapping are now checked against ci-amd.yml, so a renamed hook fails a test instead of going stale. The default prek row uses --from-ref, the scope AGENTS.md recommends, and --full keeps CI's --all-files form. Step 7 of the publish-changes skill, the AGENTS.md commands block and the selective-checks doc now point at breeze verify. All patches in the CLI tests use autospec.
…un_ prefix The classification test used to enumerate SelectiveChecks properties named run_* and assume those were the job gates, so a gate with another name would slip through. It now scans every `if:` in .github/workflows/ for the selective-checks outputs CI actually gates on, which also drops the hand-kept list of run_* outputs no job reads.
prek already picks the hooks to run for the changed files, so breeze verify no longer lists a prek row, in either mode. The mypy-providers and migration-round-trip jobs are manual-stage prek hooks too; their selective-checks flags are now classified in PREK_JOBS and not printed. The output, the --full help, the breeze docs, AGENTS.md and the publish-changes skill say static checks are not listed. Also classify run_agent_framework_tests, added to SelectiveChecks on main, with the breeze shell commands its CI job runs per framework.
The default --base-ref was the local main branch. In a fork it is often behind the main the branch has merged (a GitHub "Update branch" or a conflict fix), and then every merged-in commit counted as a change: 3287 files instead of the 21 the PR changes. The default is now main on the remote whose URL points at apache/airflow, preferring upstream, which is the base GitHub compares the PR with. Without such a remote it falls back to the local main and says so. The local copy of that remote can be stale too, after "Update branch" without a fetch. breeze verify now warns when a merge on the branch brought in commits the base does not have. With --json the warnings go to stderr so stdout stays parseable.
Dropping the static-check rows also hid mypy-providers and migration-round-trip, which CI runs but a default prek install never triggers (pre-push and manual stage), so a migration change got no hint until CI failed. The CLI tests also ran real git against the developer's checkout and global config. Generated-by: Claude Opus 5
2f18558 to
0bcbb17
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Re-checked the commits since my approval: the new base-ref default (main on the apache/airflow remote, preferring upstream) and the stale-base warning look right and are well tested, and it still reuses SelectiveChecks rather than copying any file-group logic.
I rebased onto current main (only the generated images conflicted; they're regenerated) and pushed one commit on top with the follow-ups I'd have asked for:
- Hint for the prek hooks a default install doesn't run. Dropping the prek rows also hid
mypy-providers(pre-push/manual) andmigration-round-trip(manual only), anddefault_install_hook_typesis[pre-commit, commit-msg], so a migration change got no hint until CI failed.PREK_JOBSnow maps each job to its hook; when the change triggers one,breeze verifyprintsprek run --stage manual <hook> --from-ref <base>, and--jsonlists them undermanual_prek_hooks.test_prek_job_hooks_are_not_installed_by_defaultkeeps the mapping honest against the.pre-commit-config.yamlfiles, and the docs say the same. - Hermetic git test.
_gitintest_detects_merged_commits_the_base_does_not_havenow passescommit.gpgsign=falseandcore.hooksPath=/dev/null, so a global signing or hooks setup can't break it. - No real
gitin the CLI tests. An autouse fixture stubsfind_default_base_ref/has_merged_commits_missing_fromwith plain functions (so the tests that patch them withautospecstill work), making the tests independent of the local remotes. - Test for the empty shared-dists guard in
--full.
Thanks for all the iterations on this one — merging once CI is green.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker c688a6f v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
The breeze verify command (#73241) derives the base ref and the selective-checks default_branch from branch_defaults.AIRFLOW_BRANCH, but its tests hardcode "main". On a vX-Y-test release branch - where AIRFLOW_BRANCH is e.g. "v3-4-test" - the base-ref lookup, the no-apache-remote fallback, and the docs list all differ, so eight tests fail. v3-4-test is the first release branch to include the command and surface this. Pin AIRFLOW_BRANCH / DEFAULT_AIRFLOW_CONSTRAINTS_BRANCH to main / constraints-main in an autouse fixture so the suite asserts the main behaviour it was written for, on any branch.
4 participants
Result (TL;DR)
breeze verifyturns the working-tree diff into local verification commands. It lists directly relevant checks by default, with--fullfor the expanded default CI cell and--jsonfor machine-readable output.This PR only lists out the suggested commands; a follow-up can add
--runto actually execute all tests in one command.Why
Before opening a PR, AGENTS.md step 7 asks an agent to find the tests CI would run for the change and run the relevant ones locally. The tool for that is
breeze ci selective-check, which was built for the build-info job in CI: it reads one commit and prints outputs likerun-unit-tests=truefor the workflows to consume. It does not say which command each output maps to, so a contributor (agent) still has to readci-amd.yml,run-unit-tests.ymlandrun_unit_tests.shto get abreeze testingline.This PR makes the mapping from selective-check outputs to local commands deterministic. For a human it is a preview of what CI will do. For an agent following AGENTS.md step 7, and for the
--runflag that comes next, it is the list to execute. It could help save a lot of tokens and prevent the agent from selecting the wrong checks.What
Add
breeze verify. It reads committed and untracked changes against--base-ref, using the same rename behavior as CI. Nothing is executed.run_*flag onSelectiveChecksmust be classified, and the printed core/providers flags must match a line inscripts/ci/testing/run_unit_tests.sh.breeze ci selective-checkinstead, but that group is CI-facing and its inputs assume a CI context. The plan builder is a standalone function, so moving it there is a small change if you prefer.--run. This PR keeps the command list-only so the selection and mapping can be reviewed separately.--jsonmodeBetter for user copying or agent reading.
--fullmodeLists the expanded plan for CI's default Python and sqlite cell, including checks that run on every PR.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Fable 5.1) following the guidelines