ColtenOuO commented on PR #72006: URL: https://github.com/apache/airflow/pull/72006#issuecomment-5386826865
Two follow-up items I'd like reviewer input on — neither blocks this PR. ### 1. CI doesn't catch changes to `generate_erd.py` in isolation `selective-checks`' `DOC_FILES` group (in [`selective_checks.py`](https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/utils/selective_checks.py)) is what decides whether the `docs-build` job runs — and that job is the only thing that actually exercises `generate_erd` end-to-end (importing the models, building the `MetaData`, emitting Mermaid markup, and feeding it through the `.. mermaid::` directive). Its patterns include `^devel-common/src/docs` but not `^devel-common/src/sphinx_exts`, where `generate_erd.py` actually lives. This PR's own `docs-build` run is triggered fine, because it also touches `airflow-core/docs/` and `providers/*/docs/`. But a future PR that touches *only* `generate_erd.py` (or anything else under `devel-common/src/sphinx_exts/`) wouldn't trigger a docs build at all — it could break the extension (import error, bad Mermaid syntax, whatever) and CI would stay green. **Proposal:** add `^devel-common/src/sphinx_exts` to the `DOC_FILES` pattern list in `selective_checks.py`, plus the matching doc/test updates (`04_selective_checks.md` and `test_selective_checks.py`). Happy to open that as a separate PR if the direction sounds right. ### 2. The ERD is dense and doesn't have real zoom/pan On the core schema (57 tables) the diagram is dense enough that even full-screen isn't very readable. `sphinxcontrib-mermaid` supports real client-side pan/zoom via `mermaid_d3_zoom = True`, which loads `d3.js` and lets users scroll-to-zoom / drag-to-pan the rendered SVG (as opposed to whole-page browser zoom, which just scales the already-cramped layout uniformly and doesn't help). Two reasons I left it out of this PR: - It's a Sphinx-wide `conf.py` setting (`app.config`), not something scoped per-diagram — turning it on would affect every `.. mermaid::` diagram across the docs (e.g. the flowcharts in `security_model.rst` / `jwt_token_authentication.rst`), not just the three ERD pages. - By default it pulls `d3.min.js` from `cdn.jsdelivr.net`. These docs otherwise seem to lean toward mirroring external JS in (see `swagger_mirror_external_resources = True` in `airflow-core/docs/conf.py`), so doing this properly would mean also setting `d3_use_local` and vendoring/hosting `d3.min.js` ourselves — more surface area than I wanted to add unreviewed in the same PR as the SVG→Mermaid switch. **Proposal:** if there's appetite for this, I'd want to confirm the CDN-vs-local-hosting call with a maintainer before implementing, given the existing convention around external resources. Let me know if it's worth a follow-up PR, and if so which way to go on that. -- 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]
