potiuk commented on a change in pull request #18964:
URL: https://github.com/apache/airflow/pull/18964#discussion_r729789012
##########
File path: airflow/providers/microsoft/azure/sensors/wasb.py
##########
@@ -57,7 +57,7 @@ def __init__(
self.check_options = check_options
def poke(self, context: dict):
- self.log.info('Poking for blob: %s\nin wasb://%s', self.blob_name,
self.container_name)
+ self.log.info('Poking for blob: %s\n in wasb://%s', self.blob_name,
self.container_name)
Review comment:
You can split the strings into two strings in two log entries @haloko4
- that will actually even better reflect thte idea of this. I personally
believe using /n on logs is a bad idea because it does not play well with the
way how prefix logs works. If you have separate log entries without the /n they
will be much nicer laid out (both lines will 'start' in the same place:
```
Date time log level: Line 1
Date time log level: LIne 2
```
Instead of (as we have now)
```
Date time log level: Line 1
LIne 2
```
The current approach is also potentially (low level) opening for so called
'log injection' vulnerability and it is considered as bad practice to have \n
in the log lines because of that.
I think this is a great opportunity for you to improve those logs in general.
Can we.count on your help here ?
##########
File path: airflow/providers/microsoft/azure/sensors/wasb.py
##########
@@ -57,7 +57,7 @@ def __init__(
self.check_options = check_options
def poke(self, context: dict):
- self.log.info('Poking for blob: %s\nin wasb://%s', self.blob_name,
self.container_name)
+ self.log.info('Poking for blob: %s\n in wasb://%s', self.blob_name,
self.container_name)
Review comment:
Feel free to add it
##########
File path: airflow/providers/microsoft/azure/sensors/wasb.py
##########
@@ -57,7 +57,7 @@ def __init__(
self.check_options = check_options
def poke(self, context: dict):
- self.log.info('Poking for blob: %s\nin wasb://%s', self.blob_name,
self.container_name)
+ self.log.info('Poking for blob: %s\n in wasb://%s', self.blob_name,
self.container_name)
Review comment:
Btw yeah - this is exactly how other people do it. Usually when someone
fixes thing like that, also pre-commit to prevent it in the future - so yeah we
are thinking in the same direction. It would be great to fix it in all places
and prevent it in the future (probably 20 or 30 pre-commits of ours came to
being in this way)
##########
File path: airflow/providers/microsoft/azure/sensors/wasb.py
##########
@@ -57,7 +57,7 @@ def __init__(
self.check_options = check_options
def poke(self, context: dict):
- self.log.info('Poking for blob: %s\nin wasb://%s', self.blob_name,
self.container_name)
+ self.log.info('Poking for blob: %s\n in wasb://%s', self.blob_name,
self.container_name)
Review comment:
Btw yeah - this is exactly how other people do it. Usually when someone
fixes thing like that, also adds pre-commit to prevent it in the future - so
yeah we are thinking in the same direction. It would be great to fix it in all
places and prevent it in the future (probably 20 or 30 pre-commits of ours came
to being in this way)
--
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]
