jscheffl commented on code in PR #58840:
URL: https://github.com/apache/airflow/pull/58840#discussion_r2572903055
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
Review Comment:
I assume AI assisted? I'd have used:
```suggestion
common_compat_changed = any(f.startswith("providers/common/compat/")
for f in self._files)
```
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
+
+ if not common_compat_changed:
+ return False
+
+ # Find only the providers that actually have file changes (not
including related providers)
+ changed_providers: set[str] = set()
+ for changed_file in self._files:
+ provider = find_provider_affected(changed_file, include_docs=False)
+ if provider and provider not in ["common.compat", "Providers"]:
+ changed_providers.add(provider)
+ elif provider == "Providers":
+ changed_providers.update(
+ provider for provider in DEPENDENCIES.keys() if provider
!= "common.compat"
+ )
+ break
+
+ if not changed_providers:
+ # Only common.compat changed, no issue
+ return False
+
+ other_providers = sorted(changed_providers)
+
+ get_console().print(
+ f"[warning]common.compat provider changed along with other
providers: {other_providers}[/]"
+ )
+
+ # Check pyproject.toml files of other affected providers
+ violations = []
+ for provider in other_providers:
+ # Convert provider name to path (e.g., "neo4j" ->
"providers/neo4j/pyproject.toml")
+ provider_path = provider.replace(".", "/")
+ pyproject_file = f"providers/{provider_path}/pyproject.toml"
+
+ # Check if this file exists and get its content
+ new_result = run_command(
+ ["git", "show", f"{self._commit_ref}:{pyproject_file}"],
+ capture_output=True,
+ text=True,
+ cwd=AIRFLOW_ROOT_PATH,
+ check=False,
+ )
+ if new_result.returncode != 0:
+ continue
+
+ # Check the raw file content for common-compat dependency with
comment
+ file_content = new_result.stdout
+ has_common_compat_dep = False
+ has_use_next_version = False
+
+ # Look through each line to find common-compat dependency
+ for line in file_content.splitlines():
+ if "apache-airflow-providers-common-compat" in line.lower():
+ has_common_compat_dep = True
+ # Check if the same line has the '# use next version'
comment
+ if "# use next version" in line.lower():
+ has_use_next_version = True
+ break
+
+ if has_common_compat_dep and not has_use_next_version:
+ violations.append(provider)
Review Comment:
This can be also a one-line (ruff might add line-breaks)
```suggestion
violations.append(provider) if
any("apache-airflow-providers-common-compat" in line and "# use next version"
not in line for line in file_content.splitlines())
```
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
+
+ if not common_compat_changed:
+ return False
Review Comment:
Oh and with this following even shorter:
```suggestion
if not any(f.startswith("providers/common/compat/") for f in
self._files):
return False
```
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
+
+ if not common_compat_changed:
+ return False
+
+ # Find only the providers that actually have file changes (not
including related providers)
+ changed_providers: set[str] = set()
+ for changed_file in self._files:
+ provider = find_provider_affected(changed_file, include_docs=False)
+ if provider and provider not in ["common.compat", "Providers"]:
+ changed_providers.add(provider)
+ elif provider == "Providers":
+ changed_providers.update(
+ provider for provider in DEPENDENCIES.keys() if provider
!= "common.compat"
+ )
+ break
Review Comment:
For readability I'd reverse the check:
(1) first check for providers
(2) then elif provider and provider != "common-compat"
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
+
+ if not common_compat_changed:
+ return False
+
+ # Find only the providers that actually have file changes (not
including related providers)
+ changed_providers: set[str] = set()
+ for changed_file in self._files:
+ provider = find_provider_affected(changed_file, include_docs=False)
+ if provider and provider not in ["common.compat", "Providers"]:
+ changed_providers.add(provider)
+ elif provider == "Providers":
Review Comment:
I am not sure, why do you check for "Providers" at all? I understand the
`func_provider_affected()` as this is only a result if devel-common is changed.
Why do we need to check then further? If devel common is changed this should
not affect a provider release version and dependecy in my view
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -1767,3 +1768,125 @@ def parse_dep(dep_str: str) -> tuple[str, str | None]:
)
return violations
+
+ @cached_property
+ def common_compat_changed_without_next_version(self) -> bool:
+ """
+ Check if common.compat provider changed and other providers changed
don't have '# use next version'
+ comment for their common-compat dependency.
+ """
+ if self._github_event != GithubEvents.PULL_REQUEST:
+ return False
+
+ # First, check if common.compat provider was changed
+ common_compat_changed = False
+ for changed_file in self._files:
+ if changed_file.startswith("providers/common/compat/"):
+ common_compat_changed = True
+ break
+
+ if not common_compat_changed:
+ return False
+
+ # Find only the providers that actually have file changes (not
including related providers)
+ changed_providers: set[str] = set()
+ for changed_file in self._files:
+ provider = find_provider_affected(changed_file, include_docs=False)
+ if provider and provider not in ["common.compat", "Providers"]:
+ changed_providers.add(provider)
+ elif provider == "Providers":
+ changed_providers.update(
+ provider for provider in DEPENDENCIES.keys() if provider
!= "common.compat"
+ )
+ break
+
+ if not changed_providers:
+ # Only common.compat changed, no issue
+ return False
+
+ other_providers = sorted(changed_providers)
+
+ get_console().print(
+ f"[warning]common.compat provider changed along with other
providers: {other_providers}[/]"
+ )
+
+ # Check pyproject.toml files of other affected providers
+ violations = []
+ for provider in other_providers:
+ # Convert provider name to path (e.g., "neo4j" ->
"providers/neo4j/pyproject.toml")
+ provider_path = provider.replace(".", "/")
+ pyproject_file = f"providers/{provider_path}/pyproject.toml"
+
+ # Check if this file exists and get its content
+ new_result = run_command(
+ ["git", "show", f"{self._commit_ref}:{pyproject_file}"],
+ capture_output=True,
+ text=True,
+ cwd=AIRFLOW_ROOT_PATH,
+ check=False,
+ )
+ if new_result.returncode != 0:
+ continue
Review Comment:
Why do you continue here if no positive return code?
--
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]
