af: catch OSError in _read_yaml so non-file ASTRO_HOME degrades gracefully#205
af: catch OSError in _read_yaml so non-file ASTRO_HOME degrades gracefully#205
Conversation
…fully _read_yaml only caught FileNotFoundError, so reading /dev/null/config.yaml (NotADirectoryError) or any other OSError-shape failure (permission denied, broken symlink) leaked out of the resolver as an uncaught exception that detect_version then masked as "Failed to detect Airflow version". Catch the broader OSError in both the initial read and the partial-write retry, so the resolver consistently surfaces "No astro context. Run astro login first." for any reason the file isn't readable. Found via smoke testing 0.7.0a1 with ASTRO_HOME=/dev/null.
| try: | ||
| text = path.read_text() | ||
| except FileNotFoundError: | ||
| except OSError: |
There was a problem hiding this comment.
Catching all of OSError is broader than the /dev/null repro needs. PermissionError on a real config (e.g. user's ~/.astro/config.yaml chmod'd 000) will now silently fall through to "Run astro login first", which is misleading — the file is there, it's just unreadable. Was the broad catch intentional, or would (FileNotFoundError, NotADirectoryError, IsADirectoryError) be enough to fix the reported bug while still surfacing genuine read failures?
There was a problem hiding this comment.
good catch, narrowed in c860abf. now only catches the three OSError shapes that actually mean 'no readable config file' (FileNotFoundError, NotADirectoryError, IsADirectoryError). PermissionError and other OSErrors propagate so chmod 000 surfaces clearly instead of dead-ending at 'run astro login'
| # global astro state. Reading /dev/null/config.yaml raises | ||
| # NotADirectoryError (an OSError, NOT FileNotFoundError); the | ||
| # resolver should still surface "no session" cleanly. | ||
| monkeypatch.setenv("ASTRO_HOME", "/dev/null") |
There was a problem hiding this comment.
/dev/null is Unix-only. test_config.py::test_load_with_dev_null_via_env (line 549) already has the right pattern for this: os.devnull plus a pytest.skip if it's a regular file on the platform. Worth mirroring that here so the test doesn't fail on Windows.
There was a problem hiding this comment.
fixed in c860abf, mirrors the test_load_with_dev_null_via_env pattern (os.devnull + skip when it's a regular file). thanks for the pointer
akshayarora
left a comment
akshayarora
left a comment
There was a problem hiding this comment.
Yeah, this is a clean and minimal fix with a good explanatory docstring. Thanks!
Address PR review feedback: - _read_yaml now catches the specific OSError shapes that mean "config doesn't exist as a file" (FileNotFoundError, NotADirectoryError, IsADirectoryError) instead of swallowing all OSError. PermissionError on a real config (chmod 000) propagates so the user gets a useful error rather than a misleading "run astro login". - Test uses os.devnull + skip-if-regular-file, mirroring the pattern in test_config.py::test_load_with_dev_null_via_env so it doesn't fail on Windows (where os.devnull is "nul" and behaves differently).
3 participants
summary
found while smoke-testing the 0.7.0a1 alpha (#204):
_read_yamlonly caughtFileNotFoundError, so any other read failure leaked through as an uncaught exception thatdetect_versionthen masked as "Failed to detect Airflow version"simplest repro:
ASTRO_HOME=/dev/null af api version(/dev/null/config.yamlraisesNotADirectoryError, which is anOSErrorbut notFileNotFoundError). same for permission-denied or broken symlinkswhat changed
_read_yamlnow catchesOSErrorinstead ofFileNotFoundError(both the initial read and the partial-write retry)ASTRO_HOME=/dev/nullnow surfacesAstroNotLoggedInErrorcleanlytest plan
ASTRO_HOME=/dev/null af api versionnow reports "No astro context. Run astro login first." instead of "Failed to detect Airflow version"