Copilot commented on code in PR #73808:
URL: https://github.com/apache/airflow/pull/73808#discussion_r4152210496
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -23,6 +23,7 @@
import subprocess
import sys
import tempfile
+from urllib.parse import urlparse
from unittest.mock import MagicMock, patch
Review Comment:
Keep these standard-library imports in the repository's sorted order:
`unittest` precedes `urllib` (for example,
`task-sdk/tests/task_sdk/definitions/test_connection.py:21-22`). This ordering
should otherwise fail the import-sorting check.
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -609,3 +610,59 @@ def test_zero_wait_is_allowed(self):
assert result.returncode == 0, result.stderr
assert result.stdout.strip() == "0"
assert result.stderr == ""
+
+
+class TestGetClientEnvironment:
+ """Regression coverage for GH#70519.
+
+ ``get_client`` (and therefore every ``@provide_api_client``-decorated
+ command) must resolve the config file for the requested environment
+ instead of silently using production credentials for every command except
+ ``auth login``.
+ """
+
+ @staticmethod
+ def _write_environment_config(airflow_home, environment: str, api_url:
str) -> None:
+ (airflow_home /
f"{environment}.json").write_text(json.dumps({"api_url": api_url}),
encoding="utf-8")
+
+ @pytest.fixture(autouse=True)
+ def environments(self, tmp_path):
+ """The module fixture already clears the environment; pin AIRFLOW_HOME
to tmp_path."""
+ os.environ["AIRFLOW_HOME"] = str(tmp_path)
+ self._write_environment_config(tmp_path, "production",
"https://prod.example.com";)
+ self._write_environment_config(tmp_path, "staging",
"https://staging.example.com";)
+ yield tmp_path
+ del os.environ["AIRFLOW_HOME"]
+
+ def test_defaults_to_production(self):
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"prod.example.com"
+
+ def test_uses_the_requested_environment(self):
Review Comment:
This test still passes when the PR's source changes are reverted because
`get_client` already defaulted to production. Repository testing standards
require each added test to exercise changed behavior and fail without the fix,
so remove this padding test.
##########
airflow-ctl/tests/airflow_ctl/ctl/commands/test_version_command.py:
##########
@@ -52,13 +52,20 @@ def test_ctl_version_remote(self, mock_client):
assert "version" in stdout.getvalue()
assert "git_version" in stdout.getvalue()
assert "airflowctl_version" in stdout.getvalue()
- mock_get_client.assert_called_once_with(kind=ClientKind.NO_AUTH)
+ mock_get_client.assert_called_once_with(kind=ClientKind.NO_AUTH,
api_environment="production")
def test_ctl_version_remote_with_api_token(self, mock_client):
with mock.patch("airflowctl.ctl.commands.version_command.get_client")
as mock_get_client:
mock_get_client.return_value.__enter__.return_value = mock_client
version_info(self.parser.parse_args(["version", "--remote",
"--api-token", "TOKEN"]))
- mock_get_client.assert_called_once_with(kind=ClientKind.NO_AUTH)
+ mock_get_client.assert_called_once_with(kind=ClientKind.NO_AUTH,
api_environment="production")
+
+ def test_ctl_version_remote_with_env(self, mock_client):
+ """``--env`` selects which environment's credentials the remote call
uses."""
+ with mock.patch("airflowctl.ctl.commands.version_command.get_client")
as mock_get_client:
Review Comment:
This docstring only restates the test name and invocation, so it adds no
context that the code does not already convey. Remove it per the repository's
non-narrating comment guidance.
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -609,3 +610,59 @@ def test_zero_wait_is_allowed(self):
assert result.returncode == 0, result.stderr
assert result.stdout.strip() == "0"
assert result.stderr == ""
+
+
+class TestGetClientEnvironment:
+ """Regression coverage for GH#70519.
+
+ ``get_client`` (and therefore every ``@provide_api_client``-decorated
+ command) must resolve the config file for the requested environment
+ instead of silently using production credentials for every command except
+ ``auth login``.
+ """
+
+ @staticmethod
+ def _write_environment_config(airflow_home, environment: str, api_url:
str) -> None:
+ (airflow_home /
f"{environment}.json").write_text(json.dumps({"api_url": api_url}),
encoding="utf-8")
+
+ @pytest.fixture(autouse=True)
+ def environments(self, tmp_path):
+ """The module fixture already clears the environment; pin AIRFLOW_HOME
to tmp_path."""
+ os.environ["AIRFLOW_HOME"] = str(tmp_path)
+ self._write_environment_config(tmp_path, "production",
"https://prod.example.com";)
+ self._write_environment_config(tmp_path, "staging",
"https://staging.example.com";)
+ yield tmp_path
+ del os.environ["AIRFLOW_HOME"]
+
+ def test_defaults_to_production(self):
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"prod.example.com"
+
+ def test_uses_the_requested_environment(self):
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN",
api_environment="staging") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_no_auth_uses_the_requested_environment(self):
+ with get_client(kind=ClientKind.NO_AUTH, api_environment="staging") as
client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_environment_variable_beats_the_explicit_argument(self,
monkeypatch):
+ """AIRFLOW_CLI_ENVIRONMENT keeps precedence, matching Credentials' own
contract."""
+ monkeypatch.setenv("AIRFLOW_CLI_ENVIRONMENT", "staging")
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN",
api_environment="production") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_decorator_forwards_the_env_argument(self):
+ """``@provide_api_client`` reads ``--env`` off the parsed args
namespace."""
+ from types import SimpleNamespace
Review Comment:
This docstring merely repeats the test name and the assertion below. Remove
it to follow the repository's rule against narrating comments.
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -609,3 +610,59 @@ def test_zero_wait_is_allowed(self):
assert result.returncode == 0, result.stderr
assert result.stdout.strip() == "0"
assert result.stderr == ""
+
+
+class TestGetClientEnvironment:
+ """Regression coverage for GH#70519.
+
+ ``get_client`` (and therefore every ``@provide_api_client``-decorated
+ command) must resolve the config file for the requested environment
+ instead of silently using production credentials for every command except
+ ``auth login``.
+ """
Review Comment:
Test docstrings must describe behavior rather than track issue numbers.
Remove the `GH#70519` reference; the remaining prose only repeats the class and
test names, so the whole class docstring can be removed.
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -609,3 +610,59 @@ def test_zero_wait_is_allowed(self):
assert result.returncode == 0, result.stderr
assert result.stdout.strip() == "0"
assert result.stderr == ""
+
+
+class TestGetClientEnvironment:
+ """Regression coverage for GH#70519.
+
+ ``get_client`` (and therefore every ``@provide_api_client``-decorated
+ command) must resolve the config file for the requested environment
+ instead of silently using production credentials for every command except
+ ``auth login``.
+ """
+
+ @staticmethod
+ def _write_environment_config(airflow_home, environment: str, api_url:
str) -> None:
+ (airflow_home /
f"{environment}.json").write_text(json.dumps({"api_url": api_url}),
encoding="utf-8")
+
+ @pytest.fixture(autouse=True)
+ def environments(self, tmp_path):
+ """The module fixture already clears the environment; pin AIRFLOW_HOME
to tmp_path."""
+ os.environ["AIRFLOW_HOME"] = str(tmp_path)
+ self._write_environment_config(tmp_path, "production",
"https://prod.example.com";)
+ self._write_environment_config(tmp_path, "staging",
"https://staging.example.com";)
+ yield tmp_path
+ del os.environ["AIRFLOW_HOME"]
+
+ def test_defaults_to_production(self):
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"prod.example.com"
+
+ def test_uses_the_requested_environment(self):
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN",
api_environment="staging") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_no_auth_uses_the_requested_environment(self):
+ with get_client(kind=ClientKind.NO_AUTH, api_environment="staging") as
client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_environment_variable_beats_the_explicit_argument(self,
monkeypatch):
+ """AIRFLOW_CLI_ENVIRONMENT keeps precedence, matching Credentials' own
contract."""
+ monkeypatch.setenv("AIRFLOW_CLI_ENVIRONMENT", "staging")
+ with get_client(kind=ClientKind.CLI, api_token="TOKEN",
api_environment="production") as client:
+ assert urlparse(str(client.base_url)).hostname ==
"staging.example.com"
+
+ def test_decorator_forwards_the_env_argument(self):
+ """``@provide_api_client`` reads ``--env`` off the parsed args
namespace."""
+ from types import SimpleNamespace
+
+ from airflowctl.api.client import provide_api_client
Review Comment:
These imports are unconditional and have no circular-import or lazy-loading
requirement, so placing them inside the test violates the repository's
top-level-import rule. Move `SimpleNamespace` and `provide_api_client` into the
module import block.
--
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]
