Andrushika commented on code in PR #74229:
URL: https://github.com/apache/airflow/pull/74229#discussion_r4310493978
##########
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:
Applied, thanks. The warning is printed before the list in every mode, and
nothing is blocked anymore.
##########
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:
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.
##########
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:
Removed `--dry-run` from `breeze verify`, so it does not touch the global
dry-run state anymore.
##########
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:
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.
##########
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:
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. A test passes a test-types value with `[`,
`$HOME` and `;` through bash and checks it comes back unchanged. Ctrl+C now
stops the run, prints what failed and what did not finish, and exits 130.
##########
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:
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()`.
##########
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:
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.
--
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]
