[GitHub] [airflow] ashb commented on a change in pull request #16424: Switch to built-in data structures in SecretsMasker

[GitBox <gi...@apache.org>]

ashb commented on a change in pull request #16424:
URL: https://github.com/apache/airflow/pull/16424#discussion_r652508465



##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -196,12 +195,11 @@ def redact(self, item: "RedactableItem", name: str = None) -> "RedactableItem":
             elif isinstance(item, (tuple, set)):
                 # Turn set in to tuple!
                 return tuple(self.redact(subval) for subval in item)
-            elif isinstance(item, io.IOBase):
-                return item
-            elif isinstance(item, Iterable):
+            elif isinstance(item, list):
                 return list(self.redact(subval) for subval in item)
             else:
                 return item
+        # I think this should never happen, but it does not hurt to leave it just in case

Review comment:
       Agreet

##########
File path: airflow/utils/log/secrets_masker.py
##########
@@ -165,7 +164,7 @@ def _redact_all(self, item: "RedactableItem") -> "RedactableItem":
         elif isinstance(item, (tuple, set)):
             # Turn set in to tuple!
             return tuple(self._redact_all(subval) for subval in item)
-        elif isinstance(item, Iterable):
+        elif isinstance(item, list):

Review comment:
       Tuple too please

##########
File path: tests/utils/log/test_secrets_masker.py
##########
@@ -72,22 +72,6 @@ def test_args(self, logger, caplog):
 
         assert caplog.text == "INFO Cannot connect to user:***\n"
 
-    def test_non_redactable(self, logger, caplog):

Review comment:
       Inherit from list or dict should trigger it, but don't mind deleting this test




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org