Copilot commented on code in PR #64572:
URL: https://github.com/apache/airflow/pull/64572#discussion_r3025327239
##########
dev/breeze/src/airflow_breeze/utils/environment_context.py:
##########
@@ -0,0 +1,29 @@
+from __future__ import annotations
+
+import os
+from dataclasses import dataclass
Review Comment:
This new module is missing the standard Apache ASF license header present in
other Breeze utils (e.g.,
dev/breeze/src/airflow_breeze/utils/host_info_utils.py). Please add the license
header at the top of this file to comply with repository conventions.
##########
dev/breeze/src/airflow_breeze/utils/environment_context.py:
##########
@@ -0,0 +1,29 @@
+from __future__ import annotations
+
+import os
+from dataclasses import dataclass
+
+
+@dataclass
+class EnvironmentContext:
+ is_wsl: bool
+ inside_container: bool
+ docker_available: bool
+ recommended_command: str
+
+
+def detect_environment_context() -> EnvironmentContext:
+ inside_container = os.path.exists("/.dockerenv")
+ is_wsl_env = "WSL_DISTRO_NAME" in os.environ
+
+ if inside_container:
+ recommended = "pytest"
+ else:
+ recommended = "breeze shell"
+
+ return EnvironmentContext(
+ is_wsl=is_wsl_env,
+ inside_container=inside_container,
+ docker_available=True,
+ recommended_command=recommended,
Review Comment:
`docker_available` is currently hard-coded to `True`, so the helper will
report Docker as available even when it is not installed/running (which also
makes `recommended_command` unreliable on host). Please implement a real
availability check (e.g., run `docker info`/`docker version` with `check=False`
and return a boolean) or rename/remove this field if it is intentionally not
detected here.
##########
dev/breeze/src/airflow_breeze/utils/test_environment_context.py:
##########
@@ -0,0 +1,6 @@
+from airflow_breeze.utils.environment_context import detect_environment_context
+
+
+def test_detect_host_environment():
+ context = detect_environment_context()
+ assert context.recommended_command in ["breeze shell", "pytest"]
Review Comment:
This test module is located under `src/airflow_breeze/...`, but Breeze's
pytest configuration only collects tests from `dev/breeze/tests`
(`dev/breeze/pyproject.toml` sets `testpaths = ["tests"]`). As a result, this
test will not run in the normal test suite. Please move it to
`dev/breeze/tests/` (and keep `src/` limited to library code).
##########
dev/breeze/src/airflow_breeze/utils/test_environment_context.py:
##########
@@ -0,0 +1,6 @@
+from airflow_breeze.utils.environment_context import detect_environment_context
+
Review Comment:
This file is missing the standard Apache ASF license header (present in
other Breeze source files). Please add the header to comply with repository
conventions.
##########
dev/breeze/src/airflow_breeze/utils/environment_context.py:
##########
@@ -0,0 +1,29 @@
+from __future__ import annotations
+
+import os
+from dataclasses import dataclass
+
+
+@dataclass
+class EnvironmentContext:
+ is_wsl: bool
+ inside_container: bool
+ docker_available: bool
+ recommended_command: str
+
+
+def detect_environment_context() -> EnvironmentContext:
+ inside_container = os.path.exists("/.dockerenv")
+ is_wsl_env = "WSL_DISTRO_NAME" in os.environ
Review Comment:
WSL detection here relies only on the `WSL_DISTRO_NAME` environment
variable. Breeze already has WSL detection logic based on `/proc/version` and
kernel/release checks (e.g.,
`airflow_breeze.commands.developer_commands.is_wsl` and
`airflow_breeze.utils.platforms.is_wsl2`). Please reuse/centralize that
existing detection (or match its approach) so detection works consistently
across entry points.
```suggestion
def _detect_wsl() -> bool:
"""
Detect whether we are running under Windows Subsystem for Linux (WSL).
Prefer feature-based detection (via /proc and kernel release) and only
use environment variables as an additional hint.
"""
if "WSL_DISTRO_NAME" in os.environ:
return True
try:
with open("/proc/version", encoding="utf-8") as version_file:
version = version_file.read()
if "Microsoft" in version or "WSL" in version:
return True
except OSError:
# /proc may not be available or readable; ignore and continue.
pass
try:
release = os.uname().release
except (AttributeError, OSError):
return False
return "microsoft" in release.lower()
def detect_environment_context() -> EnvironmentContext:
inside_container = os.path.exists("/.dockerenv")
is_wsl_env = _detect_wsl()
```
##########
dev/breeze/src/airflow_breeze/utils/test_environment_context.py:
##########
@@ -0,0 +1,6 @@
+from airflow_breeze.utils.environment_context import detect_environment_context
+
+
+def test_detect_host_environment():
+ context = detect_environment_context()
+ assert context.recommended_command in ["breeze shell", "pytest"]
Review Comment:
As written, this assertion will always pass because
`detect_environment_context()` always returns either "breeze shell" or "pytest"
unconditionally. The test doesn't validate the WSL/container branching
behavior. Please make the test deterministic by monkeypatching `os.path.exists`
and `os.environ` to cover at least the host and container cases, and assert on
the expected fields (`inside_container`, `is_wsl`, `recommended_command`,
`docker_available`).
--
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]
