[GitHub] [airflow] jhtimmins commented on a change in pull request #16002: fix remote logging when blob already exists

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

jhtimmins commented on a change in pull request #16002:
URL: https://github.com/apache/airflow/pull/16002#discussion_r640233019



##########
File path: airflow/providers/microsoft/azure/log/wasb_task_handler.py
##########
@@ -176,19 +178,18 @@ def wasb_write(self, log: str, remote_log_location: str, append: bool = True) ->
         :type log: str
         :param remote_log_location: the log's location in remote storage
         :type remote_log_location: str (path)
-        :param append: if False, any existing log file is overwritten. If True,
-            the new log is appended to any existing logs.
+        :param append: if False and overwrite = True, any existing log file is overwritten with the new log.
+                       If True and overwrite = True, the new log is appended to any existing logs.
+                       If True and overwrite = False the method will fail
         :type append: bool
+        :param overwrite: if False, does not overwrite log file. If the file already exists the method will fail
+                          if True, any existing log file is overwritten.
+        :type overwrite: bool

Review comment:
       This behavior doesn't really make sense to me. Specifically, it seems like `append + overwrite == append`.
   
   More generally, I think that deriving behavior as the cross product of two different arguments is an antipattern. Is there a way that this can be simplified? Is `append=False && overwrite=False` a valid state? If not, and there are only two valid. states, then this can be simplified into one argument.




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