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

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

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



##########
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:
       I could not find an easy way to raise an exception any more after removing Iterable. I believe we cannot get the Exception if we do what we do now - i.e. walking the built-in structures in the way we do. But maybe there is some way an exception can be raised here? 

##########
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):

Review comment:
       I think the only reason IOBase was added here specifically was that it was Iterable. This else is not needed now as this one will fall through the last `return item` anyway.

##########
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:
       Could not find a way to trigger Exception :) 




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