potiuk commented on code in PR #72484:
URL: https://github.com/apache/airflow/pull/72484#discussion_r3952120326
##########
airflow-core/tests/unit/cli/commands/test_connection_command.py:
##########
@@ -147,6 +147,32 @@ def
test_cli_connections_list_hide_sensitive_without_show_values_fails(self):
with pytest.raises(SystemExit, match="--hide-sensitive can only be
used with --show-values"):
connection_command.connections_list(args)
+ def test_cli_connections_list_warns_about_env_var_connections(self,
monkeypatch):
+ """An `AIRFLOW_CONN_*` environment variable should trigger a stderr
warning."""
+ # `setup_method`'s `add_default_connections_back=True` also seeds
`AIRFLOW_CONN_*`
+ # env vars for every default connection, so isolate this test from
that ambient state.
Review Comment:
This rationale isn't accurate.
`clear_db_connections(add_default_connections_back=True)` calls
`create_default_connections(session=session)`
(`devel-common/src/tests_common/test_utils/db.py:382-386`), which only writes
metadata-DB rows — it never touches the environment. The function that seeds
`AIRFLOW_CONN_*` env vars is `create_default_connections_for_tests()`
(`db.py:567`, env writes at `db.py:1017`), and it is reached through
`clear_test_connections()` / `clear_all()`, not through this class's
`setup_method`.
The defensive clearing itself may well still be needed — a session-scoped
`clear_all()` can leave those vars behind — so this is about the comment, not
the code. `AGENTS.md` asks comments to carry the *why*, and a wrong *why* is
worse than none: the next person reading it will trust it. Either correct it to
name the real source, or drop it and let the loop stand as plain isolation.
##########
airflow-core/tests/unit/cli/commands/test_connection_command.py:
##########
@@ -147,6 +147,32 @@ def
test_cli_connections_list_hide_sensitive_without_show_values_fails(self):
with pytest.raises(SystemExit, match="--hide-sensitive can only be
used with --show-values"):
connection_command.connections_list(args)
+ def test_cli_connections_list_warns_about_env_var_connections(self,
monkeypatch):
+ """An `AIRFLOW_CONN_*` environment variable should trigger a stderr
warning."""
+ # `setup_method`'s `add_default_connections_back=True` also seeds
`AIRFLOW_CONN_*`
+ # env vars for every default connection, so isolate this test from
that ambient state.
+ for key in list(os.environ):
+ if key.startswith("AIRFLOW_CONN_"):
+ monkeypatch.delenv(key, raising=False)
+ monkeypatch.setenv("AIRFLOW_CONN_MY_HIDDEN_CONN",
"postgresql://u:p@host/db")
+ args = self.parser.parse_args(["connections", "list", "--output",
"json"])
+ with redirect_stderr(StringIO()) as stderr_io:
+ connection_command.connections_list(args)
+ stderr = stderr_io.getvalue()
+ assert "AIRFLOW_CONN_" in stderr
+ assert "metadata database" in stderr
+
+ def test_cli_connections_list_does_not_warn_by_default(self, monkeypatch):
+ """With no env-var connections or secrets backend configured, no
warning is printed."""
+ for key in list(os.environ):
+ if key.startswith("AIRFLOW_CONN_"):
+ monkeypatch.delenv(key, raising=False)
+ args = self.parser.parse_args(["connections", "list", "--output",
"json"])
+ with redirect_stderr(StringIO()) as stderr_io:
+ connection_command.connections_list(args)
+ stderr = stderr_io.getvalue()
+ assert stderr == ""
Review Comment:
This asserts `stderr == ""`, but it only neutralises the env-var half of the
condition — `[secrets] backend` and `[workers] secrets_backend` are left at
whatever the ambient config says.
`TestGetHiddenEntriesWarning` in `airflow-core/tests/unit/cli/test_utils.py`
gets this right with `conf_vars({("secrets", "backend"): ""})`; this test and
its `variables` twin don't. It passes in CI only because `unit_tests.cfg` has
no `[secrets]` section — a contributor with `AIRFLOW__SECRETS__BACKEND`
exported in their shell will see it fail with a message that gives no hint why.
Wrapping the body in `conf_vars({("secrets", "backend"): "", ("workers",
"secrets_backend"): ""})` makes the test state what it actually depends on.
Same shape one level down, incidentally:
`test_utils.py::TestGetHiddenEntriesWarning::test_warns_about_env_var_defined_entries`
pins `[secrets] backend` but not `[workers] secrets_backend` before asserting
`"secrets backend" not in warning`.
##########
airflow-core/tests/unit/cli/commands/test_variable_command.py:
##########
@@ -324,6 +324,27 @@ def test_variables_list_edge_cases(self):
if item["key"] in ["empty_var", "none_var", "normal_var"]:
assert item["val"] == "***"
+ def test_variables_list_warns_about_env_var_variables(self, monkeypatch):
+ """An `AIRFLOW_VAR_*` environment variable should trigger a stderr
warning."""
+ monkeypatch.setenv("AIRFLOW_VAR_MY_HIDDEN_VAR", "hidden_value")
+ args = self.parser.parse_args(["variables", "list", "--output",
"json"])
+ with redirect_stderr(StringIO()) as stderr_io:
+ variable_command.variables_list(args)
+ stderr = stderr_io.getvalue()
+ assert "AIRFLOW_VAR_" in stderr
+ assert "metadata database" in stderr
+
+ def test_variables_list_does_not_warn_by_default(self, monkeypatch):
+ """With no env-var variables or secrets backend configured, no warning
is printed."""
+ for key in list(os.environ):
+ if key.startswith("AIRFLOW_VAR_"):
+ monkeypatch.delenv(key, raising=False)
+ args = self.parser.parse_args(["variables", "list", "--output",
"json"])
+ with redirect_stderr(StringIO()) as stderr_io:
+ variable_command.variables_list(args)
+ stderr = stderr_io.getvalue()
+ assert stderr == ""
Review Comment:
Same point as the `connections` twin: `stderr == ""` only holds when no
secrets backend is configured, but the test clears only `AIRFLOW_VAR_*`. Worth
pinning `[secrets] backend` and `[workers] secrets_backend` to `""` with
`conf_vars` here too, the way `TestGetHiddenEntriesWarning` does.
##########
airflow-core/src/airflow/cli/utils.py:
##########
@@ -100,6 +103,42 @@ def print_export_output(command_type: str, exported_items:
Collection, file: Tex
print(f"{len(exported_items)} {command_type} successfully exported to
{file.name}.")
+def get_hidden_entries_warning(entity_name: str, env_prefix: str) -> str |
None:
+ """
+ Return a warning when the database listing may be incomplete.
+
+ :param entity_name: Human-readable plural noun to use in the message, e.g.
``"connections"``.
+ :param env_prefix: Environment variable prefix used for this entity, e.g.
``AIRFLOW_CONN_``.
+ :return: A warning message, or ``None`` if neither hiding source appears
to be in use.
+ """
+ # Connections and variables may also come from environment variables or a
+ # custom secrets backend. These sources can override database entries but
+ # are not included by commands that enumerate database rows.
+ has_env_vars = any(key.startswith(env_prefix) for key in os.environ)
+ # Only check whether custom backends are *configured*, without
instantiating them (which could
+ # have side effects, e.g. opening a network connection to a Vault/AWS/GCP
secrets service).
+ # Workers may override the general backend with their own [workers]
secrets_backend.
+ has_secrets_backend = any(
+ conf.get(section, key, fallback=None)
+ for section, key in (("secrets", "backend"), ("workers",
"secrets_backend"))
Review Comment:
This tuple duplicates knowledge that
`AirflowConfigParser._get_custom_secret_backend()` already owns —
`shared/configuration/src/airflow_shared/configuration/parser.py:726-728`
carries the same `("secrets", "backend")` / `("workers", "secrets_backend")`
mapping, plus the worker-to-general fallback.
Adding `[workers] secrets_backend` addressed the specific case raised
earlier in this PR, but not the underlying drift risk that comment was pointing
at: when a third source appears, this list goes quietly stale and the warning
silently under-reports again — which is the exact failure mode this PR exists
to fix.
Not instantiating the backend here is the right call, and the comment
explaining that is good, so calling `_get_custom_secret_backend()` directly
isn't the answer. Minimum fix: a comment here pointing at
`_get_custom_secret_backend` so whoever adds the next source knows there are
two places to update. Better: lift the section/key pairs into a constant in the
shared parser that both this function and `_get_custom_secret_backend` read.
--
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]
