potiuk commented on code in PR #74229:
URL: https://github.com/apache/airflow/pull/74229#discussion_r4307908018
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -59,13 +67,22 @@ def find_default_base_ref() -> str | None:
return None
-def has_merged_commits_missing_from(base_ref: str) -> bool:
- """Whether a merge on the branch brought in commits that ``base_ref`` does
not have, as after
- GitHub's "Update branch" when the local copy of the target branch was not
fetched since."""
+def has_target_branch_commits_missing_from(base_ref: str) -> bool:
+ """Whether the branch holds target-branch commits that ``base_ref`` lacks,
so they would count as changes.
+
+ A merge brings them in, as after GitHub's "Update branch" when the local
copy was not fetched
+ 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():
Review Comment:
**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.
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -178,3 +217,26 @@ def verify(ctx: click.Context, base_ref: str | None, full:
bool, as_json: bool):
f"\n[warning]Covers Python {result['default_python_version']} on
sqlite only; other Python versions, "
"Postgres, MySQL, the provider compatibility matrix and CI-only jobs
run in CI.[/]"
)
+ if get_dry_run():
+ return
+ 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)
Review Comment:
Until the check can tell those cases apart, a warning keeps it visible
without blocking:
```suggestion
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.[/]"
)
```
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -102,7 +130,13 @@ def _git(*args: str) -> list[str]:
help="List everything CI runs for the default matrix cell except static
checks, including the full "
"suite CI adds when a change touches CI tooling or dependency files.",
)
[email protected]("--json", "as_json", is_flag=True, help="Print machine-readable
JSON instead of a table.")
[email protected](
+ "--json",
+ "as_json",
+ is_flag=True,
+ help="Print the list as machine-readable JSON instead of a table. Nothing
is run.",
+)
+@option_dry_run
Review Comment:
`--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.
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -86,9 +103,20 @@ def _git(*args: str) -> list[str]:
return tuple(sorted(set(changed)))
+def run_verification_commands(commands: list[str]) -> list[int]:
+ """Run each command in a shell from the repository root, carrying on after
a failure."""
+ return_codes = []
+ 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
Review Comment:
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.
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -59,13 +67,22 @@ def find_default_base_ref() -> str | None:
return None
-def has_merged_commits_missing_from(base_ref: str) -> bool:
- """Whether a merge on the branch brought in commits that ``base_ref`` does
not have, as after
- GitHub's "Update branch" when the local copy of the target branch was not
fetched since."""
+def has_target_branch_commits_missing_from(base_ref: str) -> bool:
+ """Whether the branch holds target-branch commits that ``base_ref`` lacks,
so they would count as changes.
+
+ A merge brings them in, as after GitHub's "Update branch" when the local
copy was not fetched
+ 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():
for merged_parent in line.split()[2:]:
if _run_git("merge-base", "--is-ancestor", merged_parent,
base_ref).returncode != 0:
return True
+ 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:
Review Comment:
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.
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -86,9 +103,20 @@ def _git(*args: str) -> list[str]:
return tuple(sorted(set(changed)))
+def run_verification_commands(commands: list[str]) -> list[int]:
+ """Run each command in a shell from the repository root, carrying on after
a failure."""
+ return_codes = []
+ 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
+ )
+ return return_codes
+
+
@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.",
Review Comment:
**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`.
##########
dev/breeze/src/airflow_breeze/commands/verify_commands.py:
##########
@@ -136,10 +170,11 @@ def verify(ctx: click.Context, base_ref: str | None,
full: bool, as_json: bool):
f"No git remote points at apache/airflow, so changes are compared
with the local "
f"{AIRFLOW_BRANCH} branch."
)
- if has_merged_commits_missing_from(base_ref):
+ base_is_stale = has_target_branch_commits_missing_from(base_ref)
+ if base_is_stale:
warnings.append(
- f"Your branch has merged commits that are not in {base_ref}, so
they are listed as your "
- f"changes. Update {base_ref} (for example with git fetch) and run
again."
+ 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:
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
