Added static check for conn-fields defined in provider.yaml#69473
Conversation
dcc3524 to
1f8b257
Compare
92524b0 to
0057094
Compare
… to make sure those correspond with the ones defined in the get_connection_form_widgets method of the corresponding hook
7345dc0 to
ece3e32
Compare
potiuk
left a comment
There was a problem hiding this comment.
Thanks @dabla — nice, well-factored check: the pure helpers in check_provider_conn_fields.py are cleanly separated from the import glue and thoroughly unit-tested, and the subset semantics (React conn-fields ⊆ Flask form widgets) are the right call.
One thing worth flagging on process: the check-provider-yaml-valid prek hook only triggers on provider.yaml changes, so this PR's own CI never actually runs the new check against real providers — only the mocked unit tests ran here. I reproduced the real check locally against the full set of providers that declare conn-fields (installed all 33, ran _get_widget_keys + the subset comparison): 50 connection-type entries validated, 0 stale keys, so it passes cleanly against current main and won't start failing unrelated PRs. Approving on that basis. It might still be worth running prek run check-provider-yaml-valid --all-files once before merge as a belt-and-suspenders check in the full Breeze environment (a handful of hooks — AzureBatchHook etc. — I could only skip locally on optional-dep import errors).
Two non-blocking nits inline (docstring correctness + a coverage note). Neither blocks merge.
This review was drafted with AI assistance (Claude Code, Opus 4.8) and may contain mistakes; an Apache Airflow maintainer (@potiuk) has read and confirmed it before posting. For what the project considers a maintainer review, see the contributing guide.
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Backport successfully created: v3-3-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…ml (#69473) (#69653) * refactor: Added static check for conn-fields defined in provider.yaml to make sure those correspond with the ones defined in the get_connection_form_widgets method of the corresponding hook * Update scripts/in_container/run_provider_yaml_files_check.py * refactor: Updated comment in _get_widget_keys method --------- (cherry picked from commit 53de1c1) Co-authored-by: David Blain <info@dabla.be> Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
Added static check for
conn-fieldsdefined inprovider.yamlto make sure those correspond with the ones defined in theget_connection_form_widgetsmethod of the corresponding hook.Add static check: validate
conn-fieldsinprovider.yamlagainstget_connection_form_widgets()Background
Airflow's connection UI has two layers:
get_connection_form_widgets()on each Hook class.conn-fieldsmapping inprovider.yaml(and the generatedget_provider_info.py).An existing static check already works in one direction: if a field is declared in
provider.yamlconn-fieldsbut absent fromget_connection_form_widgets(), it is automatically added to the hook.What this PR adds
This PR adds the reverse check: fields declared in
provider.yamlconn-fieldsmust exist in the corresponding hook'sget_connection_form_widgets().In other words —
conn-fieldsmust be a valid subset of the hook's form widgets. It is intentionally not required to be an identical set: some Flask form fields may be deliberately omitted from the new React UI.The check:
connection-typesentry.get_connection_form_widgets(), or has noconn-fieldsdeclared (the new UI intentionally exposes no custom fields for that connection type).conn-fieldsis not found inget_connection_form_widgets()— those are stale or mistyped field names that the React UI would try to render but the hook doesn't know about.Error example
Files changed
scripts/ci/prek/check_provider_conn_fields.pyscripts/in_container/run_provider_yaml_files_check.pyscripts/tests/ci/prek/test_check_conn_fields_match_form_widgets.pyWas generative AI tooling used to co-author this PR?
Claude Sonnet 4.6 (GitHub Copilot CLI)
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.