developer-rpai commented on PR #74051:
URL: https://github.com/apache/airflow/pull/74051#issuecomment-5998370828
Re-reviewed the current diff against my three points from Oct 2:
**1. Silent skip on non-zip parse failures — no change needed.** Verified
the exception tuple `(SyntaxError, ValueError, TypeError, FileNotFoundError)`
is identical to the pre-PR code, so this behavior predates the change and
quiet-skip remains the established intent for an advisory checker. Fine as is.
**2. Zip resource bounds — acceptable as is, with one optional follow-up.**
Confirmed `check_dag_file_stability` runs inside `_parse_file` in the DAG
processor, so a large bundle is re-opened per parse cycle, and each candidate
member is fully read twice (once inside `might_contain_dag`, once by
`zip_file.read`) before `ast.parse`. That said: DAG bundles are
operator-deployed rather than untrusted input, `dag_file_processor_timeout`
bounds any single runaway parse, and this matches how the existing DAG
processor already handles zips. No additional safeguard required for this fix.
If you want belt-and-braces later, a per-member size cap before the full read
would be the natural hardening — happy to leave that as a follow-up, not a
blocker.
**3. Non-.py members — clean.** The `is_dir()` / `.endswith(".py")` filter
runs before `might_contain_dag` and before any read/parse, so non-Python
members are skipped without cost. Good.
Also spot-checked the merge logic: `warnings` is `dict[int, ...]`, so
`result.warnings[len(result.warnings)] = warning` appends sequentially across
members, and `setdefault` on `runtime_varying_values` keeps first-wins
semantics — both correct. The `might_contain_dag(..., zip_file=zip_file,
conf=conf)` call matches the helper's signature.
**Verdict: approve.** The fix does what the issue asks — zipped DAGs no
longer bypass stability analysis — and all three review points are resolved.
--
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]
