SameerMesiah97 commented on code in PR #63878:
URL: https://github.com/apache/airflow/pull/63878#discussion_r2991248704
##########
airflow-core/src/airflow/logging_config.py:
##########
@@ -155,10 +155,19 @@ def configure_logging():
base_log_folder = conf.get("logging", "base_log_folder")
- return init_log_folder(
- base_log_folder,
- new_folder_permissions=new_folder_permissions,
- )
+ try:
+ return init_log_folder(
+ base_log_folder,
+ new_folder_permissions=new_folder_permissions,
+ )
+ except (PermissionError, OSError) as e:
+ log.warning(
Review Comment:
`PermissionError` is fine. But `OSError` is too broad. This will also
swallow things like invalid paths, disk issues, etc. I would remove `OSError`.
##########
shared/logging/src/airflow_shared/logging/structlog.py:
##########
@@ -689,9 +689,15 @@ def init_log_folder(directory: str | os.PathLike[str],
new_folder_permissions: i
user.
"""
directory = Path(directory)
- for parent in reversed(Path(directory).parents):
- parent.mkdir(mode=new_folder_permissions, exist_ok=True)
- directory.mkdir(mode=new_folder_permissions, exist_ok=True)
+ try:
+ directory.mkdir(mode=new_folder_permissions, parents=True,
exist_ok=True)
+ except (PermissionError, OSError) as e:
+ log.warning(
Review Comment:
Above comment applies here too. And I have noticed that `def
init_log_folder` is present in
`task-sdk/src/airflow/sdk/_shared/logging/structlog.py` and
`airflow-core/src/airflow/_shared/logging/structlog.py`. Why did you not make
the same changes in those files?
--
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]
