flolas commented on a change in pull request #16002:
URL: https://github.com/apache/airflow/pull/16002#discussion_r640248752
##########
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:
Valid states are:
(append, overwrite)
(True, True)
(False, True)
(False, False)
But i dont know if we have a pattern for state (false, false). In the
context of logs if we have the combination (false, false), when the log exists
the method will fail
@ephraimbuddy what do u think? do we really need the separation or use only
one argument? maybe always using overwrite =True into wasb_write method? If you
look at s3 log handler, it has the same behaviour (replace=True)
--
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:
[email protected]
