Copilot commented on code in PR #63878:
URL: https://github.com/apache/airflow/pull/63878#discussion_r3025338539
##########
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 as e:
+ log.warning(
+ "Could not create log folder %s: %s. "
+ "Airflow will continue but logging to this directory may fail.",
+ base_log_folder,
+ e,
+ )
+ return None
Review Comment:
The `try/except PermissionError` wrapper here is likely redundant:
`init_log_folder()` already catches `PermissionError` and logs a warning (see
`airflow/_shared/logging/structlog.py` /
`airflow_shared/logging/structlog.py`). This block will never be hit, and it
duplicates the warning message. Consider removing the wrapper, or broaden it to
catch errors that `init_log_folder()` intentionally does not handle.
```suggestion
return init_log_folder(
base_log_folder,
new_folder_permissions=new_folder_permissions,
)
```
##########
shared/logging/tests/logging/test_structlog.py:
##########
@@ -512,6 +513,18 @@ def spy(et, ev, tb):
sys.excepthook = original
+def test_init_log_folder_permission_error_logs_warning_and_continues(caplog):
+ from airflow_shared.logging.structlog import init_log_folder
+
+ with mock.patch.object(Path, "mkdir", side_effect=PermissionError("not
allowed")):
+ with caplog.at_level(logging.WARNING):
+ init_log_folder("/tmp/blocked", 0o775)
Review Comment:
This file-level comment says these tests avoid `caplog`, but this new test
uses it. Either adjust the comment to note the exception, or capture the
warning via the existing structlog testing helpers to keep the test style
consistent within this file.
##########
shared/logging/tests/logging/test_structlog.py:
##########
@@ -512,6 +513,18 @@ def spy(et, ev, tb):
sys.excepthook = original
+def test_init_log_folder_permission_error_logs_warning_and_continues(caplog):
+ from airflow_shared.logging.structlog import init_log_folder
+
+ with mock.patch.object(Path, "mkdir", side_effect=PermissionError("not
allowed")):
+ with caplog.at_level(logging.WARNING):
+ init_log_folder("/tmp/blocked", 0o775)
+ continued_after_call = True
+
+ assert continued_after_call
Review Comment:
`continued_after_call` is only used to assert the code path continued; since
the assignment itself would never run if an exception was raised, this
assertion is redundant. You can simplify the test by removing the flag and just
asserting on the warning in `caplog` (or using a no-exception assertion
pattern).
```suggestion
```
##########
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 as e:
+ log.warning(
+ "Could not create log folder %s: %s. "
+ "Airflow will continue but logging to this directory may fail.",
+ directory,
+ e,
+ )
Review Comment:
`init_log_folder()` only catches `PermissionError`, but the PR description
says it should also handle `OSError`. `Path.mkdir(..., parents=True)` can raise
other `OSError` subclasses (e.g. read-only filesystem), which would still crash
callers. Consider catching `(PermissionError, OSError)` here (or `OSError` if
you want all filesystem failures handled uniformly) and logging the warning.
--
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]
