potiuk commented on code in PR #63878:
URL: https://github.com/apache/airflow/pull/63878#discussion_r3215299268
##########
shared/logging/tests/logging/test_structlog.py:
##########
@@ -512,6 +514,17 @@ def spy(et, ev, tb):
sys.excepthook = original
+def test_init_log_folder_permission_error_logs_warning_and_continues(caplog):
+ """Uses caplog to assert stdlib logging output from init_log_folder
(exception path)."""
+ 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)
+
+ assert "Could not create log folder" in caplog.text
Review Comment:
[`CLAUDE.md`](https://github.com/apache/airflow/blob/main/CLAUDE.md) is
explicit on this: *"Do not use `caplog` in tests, prefer checking logic and not
log output."*
The load-bearing contract for callers of `init_log_folder` is *"doesn't
raise on filesystem errors so CLI startup continues"*. The exact warning text
is implementation detail — if someone reworded the message tomorrow, this test
would fail without anything actually breaking.
Suggested rewrite:
```python
def test_init_log_folder_does_not_raise_on_permission_error():
from airflow_shared.logging.structlog import init_log_folder
with mock.patch.object(Path, "mkdir", side_effect=PermissionError("not
allowed")):
# Must not raise — CLI commands like `airflow db migrate` rely on
this.
init_log_folder("/tmp/blocked", 0o775)
```
If the test stops using `caplog`, the comment-block update at lines 39–41 of
this file is no longer needed either — please revert it back to the original
two-line comment.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
shared/logging/src/airflow_shared/logging/structlog.py:
##########
@@ -756,9 +756,15 @@ def init_log_folder(directory: str | os.PathLike[str],
new_folder_permissions: i
user.
"""
directory = _PatchedPath(directory)
- for parent in reversed(_PatchedPath(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:
Review Comment:
Catching only `PermissionError` is too narrow for "best-effort log dir
creation". Other `OSError` subclasses can hit on the same code path:
- `OSError(EROFS)` — read-only filesystem (e.g. recovery mode, broken bind
mount)
- `OSError(ENOSPC)` — disk full
- `OSError(EIO)` — I/O error on a flaky NFS mount or hardware fault
- `OSError(ESTALE)` — stale NFS file handle
None of these are `PermissionError`, so they'd still crash CLI commands. The
peer function `init_log_file` already catches the broader `OSError` (see line
783) for exactly the same reason — worth being consistent:
```python
except OSError as e:
```
Side note: the PR description says the catch is `(PermissionError, OSError)`
— the broader form may have been lost in a rebase.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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]
