Skip to content

Add --run to breeze verify to run the listed checks - #74229

Open
Andrushika wants to merge 9 commits into
apache:mainfrom
Andrushika:verify-run-by-default
Open

Andrushika wants to merge 9 commits into
apache:mainfrom
Andrushika:verify-run-by-default

Conversation

@Andrushika

@Andrushika Andrushika commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Why

breeze verify applies the CI selective-checks rules to the changed files and lists the local commands CI would require. You still have to copy and run each command by hand. Follow-up of #73241.

What

  • breeze verify --run runs the listed commands one after another from the repository root, then the manual prek hooks. A failure does not stop the rest, and it exits 1 if any failed. Ctrl+C prints what failed and what did not finish.
  • Without --run, breeze verify still only lists. The stale-base warning now also covers a rebase onto any remote's main, for example a fork's origin/main.
  • The listed commands quote their values, since --run passes them to bash.

breeze verify --run

2026-10-19-pr74229-run

Local main 394 commits behind: warning, not a block

2026-10-19-pr74229-stale-base

related: #73241


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5.5)

Generated-by: Claude Code (Opus 5.5) following the guidelines

breeze verify only listed the commands CI would require, so a
contributor or an agent still had to run each one by hand. It now runs
them one after another from the repository root, the manual prek hooks
included, carries on after a failure, and exits non-zero if any failed.
--dry-run, the usual breeze option, and --json only list them.

A base that is older than the target-branch commits the branch already
has makes the list count commits that are not yours, which could mean
hours of unrelated tests. breeze verify now also detects this after a
rebase, not only after a merge, and then lists without running.
@Andrushika

Copy link
Copy Markdown
Contributor Author

Hi @potiuk would appreciate your eyes on this, thanks!

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, making breeze verify able to run what it lists is useful. Two things need changing before this goes in (inline): the stale-base check now stops runs it shouldn't, and running by default changes what breeze verify means for people and agents. The rest are smaller.


Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

since. A rebase or fast-forward onto the apache/airflow remote does too, when ``base_ref`` is an
older copy such as a local branch.
"""
for line in _run_git("rev-list", "--merges", "--parents", f"{base_ref}..HEAD").stdout.splitlines():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False "stale base". Any merged parent missing from base_ref counts as a stale base, not only merges of target-branch commits. A stacked PR that merged a colleague's unmerged branch, or a merge of v3-x-test, makes this return True, and fetching does not change it. Combined with the new sys.exit(1) below, breeze verify then refuses to run at all for those branches.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It is only a warning now, and the docstring and docs say a merge of another branch can be a false alarm. Without a fetch I could not find a way to tell an unfetched main merge apart from a colleague's branch.

Comment on lines +222 to +227
if base_is_stale:
console.print(
f"\n[error]Not running the list, because {base_ref} is out of date and the list includes commits "
f"that are not yours. Update {base_ref} and run again, or pass --dry-run to only list it.[/]"
)
sys.exit(1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until the check can tell those cases apart, a warning keeps it visible without blocking:

Suggested change
if base_is_stale:
console.print(
f"\n[error]Not running the list, because {base_ref} is out of date and the list includes commits "
f"that are not yours. Update {base_ref} and run again, or pass --dry-run to only list it.[/]"
)
sys.exit(1)
if base_is_stale:
console.print(
f"\n[warning]{base_ref} may be out of date, so the list can include commits that are not yours. "
f"If it looks too long, update {base_ref} and run again.[/]"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied, thanks. The warning is printed before the list in every mode, and nothing is blocked anymore.

@main.command(
name="verify",
help="List the local verification CI would require for the current changes. Nothing is executed.",
help="Run the local verification CI would require for the current changes. Pass --dry-run to only list it.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running by default changes what breeze verify means. Until now it printed a list instantly. Now a bare breeze verify (or --full) starts the whole list in sequence, which for a change touching providers, core and docs can take hours. That surprises people who ran it to see the list, and agents: the airflow-publish-changes skill tells them to run it before every push. I'd keep listing as the default and add an explicit --run (or at least ask for confirmation when interactive), rather than making listing the opt-in --dry-run.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. breeze verify lists by default again, and --run runs the list. I also dropped the AGENTS.md and publish-changes skill changes, so they stay as on main.

is_flag=True,
help="Print the list as machine-readable JSON instead of a table. Nothing is run.",
)
@option_dry_run

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--dry-run sets breeze's process-wide dry-run mode, so selective checks' own git show calls (pyproject.toml checks, the common.compat "next version" check) return empty output. The list printed with --dry-run can differ from what a plain run executes. A local "list only" flag would avoid touching the global state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed --dry-run from breeze verify, so it does not touch the global dry-run state anymore.

Comment on lines +81 to +84
apache_ref = find_default_base_ref()
if apache_ref and apache_ref != base_ref:
fork_point = _run_git("merge-base", apache_ref, "HEAD").stdout.strip()
if fork_point and _run_git("merge-base", "--is-ancestor", fork_point, base_ref).returncode != 0:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works when an apache/airflow remote exists. A contributor whose only remote is their fork, rebased onto origin/main with an old local main, gets every rebased-over upstream commit counted as their own change, with no warning. Also, find_default_base_ref() is called again here although verify() already resolved it; passing it in would save the extra git calls and make this easier to test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check now looks at every remote's main (refs/remotes/*/main), not only the apache/airflow one, so a fork-only setup rebased onto origin/main gets the warning too. There is a test for that case. It also no longer calls find_default_base_ref().

for index, command in enumerate(commands, start=1):
get_console().print(f"\n[info]({index}/{len(commands)}) {escape(command)}[/]\n", soft_wrap=True)
return_codes.append(
run_command(["bash", "-c", command], check=False, cwd=AIRFLOW_ROOT_PATH).returncode

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These strings were built for display, and now run through bash -c. Providers[amazon] can glob-expand, and a ; or $ in a value would run as shell. Building the commands as argument lists (or quoting with shlex.join) would be safer. Ctrl+C during a long command also loses the summary of what already ran; catching KeyboardInterrupt here and printing the partial results would help.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commands need shell syntax (cd ... &&, the loop over shared dists), so I quote the values with shlex.quote / shlex.join where verification_plan.py builds them. Tests pass test-types, docs and shared-dist values with [, $HOME, ; or a space through bash and check they come back unchanged. Ctrl+C now stops the run, prints what failed and what did not finish, and exits 130.

f"{base_ref} is older than the target-branch commits your branch already has, so they are "
f"listed as your changes. Update {base_ref} (for example with git fetch) and run again."
)
if as_json:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same stale base, the exit code is 0 here (and with --dry-run) but 1 when running, so scripts can't rely on it to tell whether the list can be trusted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the stale check is only a warning now, the exit code is the same in every mode: 0 unless --run has a failing command.

breeze verify lists again by default. --run runs the listed commands
and the manual prek hooks, and --json cannot be combined with it.
Ctrl+C stops the run and prints what failed and what did not finish.

The stale-base check is only a warning now, since a merge of another
branch looks the same as a merge of an unfetched main. It also checks
every remote's main, so a fork-only setup rebased onto origin/main is
warned about.

The listed commands quote the values they interpolate, since --run
passes them to bash. --dry-run is gone: it set breeze's global
dry-run mode, which also changed what selective checks read from git.
@Andrushika

Copy link
Copy Markdown
Contributor Author

All comments above are drafted by Claude Code (Opus 5.5); reviewed by @Andrushika before posting

CI forces rich colours, so the option names in the error were wrapped
in ANSI codes and the plain-text match failed.
The docs and shared-distribution commands are now checked through bash
like the test types, so dropping their quoting fails a test.

Without --run, nothing guarded the tests from running the listed
commands for real if the list-only return regressed: that started a CI
image build. An autouse fixture replaces the runner again.
@Andrushika Andrushika changed the title Make breeze verify run the checks it lists instead of only listing them Add --run to breeze verify to run the listed checks Oct 19, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants