potiuk commented on PR #70878: URL: https://github.com/apache/airflow/pull/70878#issuecomment-5151244123
Thanks @shahar1 — all four addressed, and the `get_config` one was a real regression I introduced. Moving the check out of `_get_secret` and up into the two team-aware getters left the third reaching `_get_secret` bare, so the helper's "refused for *every* lookup" claim stopped being true the moment I wrote it. - **`get_config` guarded** in both `secrets_manager.py` and `systems_manager.py`, and the docstring now names the three getters instead of claiming universality. Confirmed the test fails without the guard (`assert 2 == 3`), so it pins the behaviour rather than just passing. - **`@staticmethod`** on `_names_a_team_namespace` in both files. - **The log assertion** — I tried your structured suggestion first and it does not work here: Airflow logs through structlog, which renders the format args into `msg` before the stdlib record exists, so `record.args` is `()` and there is no structured payload to assert on. Settled on level + logger + the refused id, which is the load-bearing data, and left a comment saying why so the next person does not repeat the attempt. Extended to cover `get_config`. On the duplicated docstring and `_log_refusal` between the two files — agreed it is not ideal, but the two backends share no base class today and I would rather not introduce one inside a security fix. Left as is. One thing you should know: **the same `get_config` gap is in #70876 (Azure), which is already merged.** Same shape, and you approved that one before finding this in the sibling. Not a cross-team read there either — `get_config` never receives a `team_name` — but the merged docstring makes the same false claim. Opening a follow-up. -- 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]
