shahar1 commented on code in PR #74820: URL: https://github.com/apache/airflow/pull/74820#discussion_r4367560926
########## dev/breeze/tests/test_selective_check_command.py: ########## @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import json +from unittest import mock + +from click.testing import CliRunner + +from airflow_breeze.commands.ci_commands import selective_check +from airflow_breeze.utils.fork_compare import ForkChanges + + +def _invoke(repository: str, event: str = "push"): + return CliRunner().invoke( + selective_check, + [ + "--commit-ref", + "abc123", + "--github-event-name", + event, + "--github-repository", + repository, + "--github-context", + json.dumps({"ref_name": "backport-abc-v3-1-test"}), + "--default-branch", + "v3-1-test", + ], + ) + + [email protected]("airflow_breeze.commands.ci_commands.get_fork_pull_request_labels", autospec=True) [email protected]("airflow_breeze.commands.ci_commands.get_changed_files", autospec=True) [email protected]("airflow_breeze.commands.ci_commands.get_fork_changes", autospec=True) +def test_fork_push_takes_changed_files_from_compare(mock_fork_changes, mock_changed_files, mock_labels): + mock_fork_changes.return_value = ForkChanges(files=("README.md",), truncated=True, merge_base="base-sha") + mock_labels.return_value = ("full tests needed",) + + with mock.patch("airflow_breeze.utils.selective_checks.SelectiveChecks", autospec=True) as mock_sc: Review Comment: nit: `dev/breeze/tests/test_ci_commands.py` already tests the `selective_check` command, with an `_invoke_selective_check` helper and `@patch("airflow_breeze.utils.selective_checks.SelectiveChecks", autospec=True)` as a decorator. These two tests could live there in the decorator form instead of a second CLI harness in a module that does not mirror the source. > - Prefer `@mock.patch` decorators over `with mock.patch(...)` context managers for patching. > - Test location mirrors source: `airflow/cli/cli_parser.py` → `tests/cli/test_cli_parser.py`. (AGENTS.md, Testing Standards) ########## dev/breeze/src/airflow_breeze/utils/fork_compare.py: ########## @@ -0,0 +1,147 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Changed files of a push in a contributor fork, computed against apache/airflow (AIP-120).""" + +from __future__ import annotations + +import json +from dataclasses import dataclass + +from airflow_breeze.global_constants import APACHE_AIRFLOW_GITHUB_REPOSITORY, GithubEvents +from airflow_breeze.utils.console import console_print +from airflow_breeze.utils.github import run_gh_command + +# The compare API lists at most this many files: a list that reaches it may be missing some, so +# the change set is treated as truncated. +COMPARE_FILE_CAP = 300 + + +@dataclass(frozen=True) +class ForkChanges: + files: tuple[str, ...] + truncated: bool + merge_base: str | None = None + + +def is_fork_push(github_event: GithubEvents, github_repository: str) -> bool: + return github_event == GithubEvents.PUSH and github_repository != APACHE_AIRFLOW_GITHUB_REPOSITORY + + +def get_fork_changes(*, fork_owner: str, sha: str, base: str) -> ForkChanges: + """ + Three-dot compare of the pushed commit against the upstream base branch. + + Three-dot semantics give the same file list a pull request against upstream would show, + so a fork whose ``main`` is behind upstream does not inflate the change set. ``base`` is the + branch line the pushed code belongs to (breeze's default branch at that commit), which a + branch name cannot be trusted to tell. A renamed file counts under both paths, as in + ``git diff``. When the compare fails the change set is reported as truncated, so the run + covers everything instead of failing. + + ``merge_base`` is the upstream commit the changes are measured from. Checks that compare a + file with its previous version use it instead of the pushed commit's parent, which after a + merge of upstream ``main`` into the branch is the contributor's own previous tip. + """ + result = run_gh_command( + [ + "gh", + "api", + f"repos/{APACHE_AIRFLOW_GITHUB_REPOSITORY}/compare/{base}...{fork_owner}:{sha}", + "--jq", + "{files: [.files[] | [.filename, (.previous_filename // empty)]], " + "merge_base: .merge_base_commit.sha}", + ], + capture_output=True, + text=True, + ) + if result.returncode != 0: + console_print( + f"[warning]Could not compare {fork_owner}:{sha} with {APACHE_AIRFLOW_GITHUB_REPOSITORY}:{base}, " + f"so everything runs: {result.stderr.strip()}[/]" + ) + return ForkChanges(files=(), truncated=True) + compare = json.loads(result.stdout or "{}") + entries = compare.get("files", []) + files = tuple(path for entry in entries for path in entry) + return ForkChanges( + files=files, truncated=len(entries) >= COMPARE_FILE_CAP, merge_base=compare.get("merge_base") + ) + + +def get_fork_pull_request_labels(*, fork_owner: str, branch: str) -> tuple[str, ...]: + """ + Labels of the open apache/airflow pull request from ``fork_owner:branch``. + + A fork push carries no pull request, so labels such as ``full tests needed`` or + ``allow provider dependency bump`` are read here. A label added later takes effect on the + next fork run. No pull request, or a failed call, means no labels. + """ + result = run_gh_command( + [ + "gh", + "api", + "--method", + "GET", + f"repos/{APACHE_AIRFLOW_GITHUB_REPOSITORY}/pulls", + "-f", + f"head={fork_owner}:{branch}", + "-f", + "state=open", + "--jq", + "[.[0].labels[]?.name]", Review Comment: nit: GitHub allows several open pull requests from the same head branch when they target different bases (say `main` and a `vX-Y-test`), and `.[0]` then takes the labels of whichever the API lists first. Passing `-f base=<default_branch>` as well would pick the pull request that matches the compare base. Rare enough to leave if you'd rather not add the parameter. -- 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]
