ashb commented on code in PR #43040:
URL: https://github.com/apache/airflow/pull/43040#discussion_r1802917515


##########
airflow/configuration.py:
##########
@@ -775,6 +775,13 @@ def _create_future_warning(name: str, section: str, 
current_value: Any, new_valu
     def _env_var_name(self, section: str, key: str) -> str:
         return f"{ENV_VAR_PREFIX}{section.replace('.', 
'_').upper()}__{key.upper()}"
 
+    def get_section_and_key_for_env(self, env: str):

Review Comment:
   I don't get why this is needed. If we need a list (again, we already have a 
concept of sensitive config values I think) then we should specify the list 
items as `(section,key)` etc



##########
tests/core/test_settings.py:
##########
@@ -385,3 +390,21 @@ def test_usage_data_collection_disabled(env_var, 
conf_setting, is_enabled, clear
     else:
         with conf_patch:
             assert is_usage_data_collection_enabled() == is_enabled
+
+
+@patch("airflow.utils.log.secret_masker.mask_secret")
+@patch("airflow.configuration.conf.get")
+@patch("airflow.configuration.get_section_and_key_for_env")
+@patch("airflow.settings.ENV_VARIABLES_TO_MASK", ["AIRFLOW__CORE__FERNET_KEY", 
"AIRFLOW__NONMASK_ENV"])
+def test_mask_conf_values(self, mock_get_section_and_key, mock_get, 
mock_mask_secret):
+    mock_get_section_and_key.side_effect = [

Review Comment:
   I don't understand why this needs to be mocked - by mocking it you never 
test the function you added 



-- 
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]

Reply via email to