Check GCSToAzureBlobStorageOperator match_glob support after template… - #70574
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Thanks — same fix as #70723 does for the Amazon GCSToS3Operator, and correct for the same reason: match_glob is in template_fields, so checking it in __init__ validated the un-rendered Jinja string and turned a bad value into a Dag parse failure rather than a task failure. Keeping the _is_match_glob_supported probe in __init__ is right, since that depends on the installed Google provider rather than anything templated.
Using ValueError here is the better choice — I've suggested on #70723 that it match yours rather than the other way round, since Airflow is trying to reduce direct AirflowException raises. Worth the two of you syncing so the sibling changes land consistent.
One test nit inline, not blocking.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
potiuk
left a comment
There was a problem hiding this comment.
Retracting my earlier approval — my mistake, and I'm sorry for the churn.
I approved this before reading #70296, which sets out the rule for this burn-down, and by that rule this change moves the wrong kind of check.
A check that only asks whether an argument was passed … belongs in
__init__and must not be moved. Fix these by rewriting in place, not by moving. Use theis not Nonepolarity.
The reasoning in the issue is sound and I'd missed both halves of it:
- With
render_template_as_native_obj=True, a field that was provided can render toNone, so the same check inexecute()reports a supplied argument as missing. - Raising in the constructor surfaces a static authoring mistake as a Dag import error, rather than once per task instance and per retry on a worker.
if not self._is_match_glob_supported and match_glob: asks whether match_glob was passed (combined with an environment capability that __init__ can already answer). It's a provision check, so it should be rewritten in place rather than relocated:
# in __init__ — keep it here
if not self._is_match_glob_supported and match_glob is not None:
raise ValueError(
"The 'match_glob' parameter requires 'apache-airflow-providers-google>=10.3.0'."
)Note the is not None polarity matters: if match_glob: is a truthiness test on the un-rendered Jinja string, which is a third question that matches neither intent. You already use ValueError here, which is right.
This is the twin of #70723 — worth the two of you landing the same shape.
Good news on mechanics: #70505 (which narrows the hook to allow provision checks written with is not None) merged on 28 July, so the rewrite below passes validate-operators-init and you can still remove the exemption-file entry in this PR — the burn-down goal is unaffected.
There's an active follow-up, #70503, cataloguing already-merged PRs that made exactly this move so they can be put back. Fixing it here saves this PR from joining that list.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
2fe93fa to
d00f1b8
Compare
|
@potiuk Thanks for catching this — restored the check to init using |
d00f1b8 to
1247839
Compare
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker cc279cd v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
apache#70574) * Check GCSToAzureBlobStorageOperator match_glob support after template rendering * Restore match_glob provision check in __init__ per apache#70296 rules
Fixes the microsoft/azure provider's GCSToAzureBlobStorageOperator entry from the #70296 exemption-list burn-down.
match_globis a template field, but__init__validated it against the installedapache-airflow-providers-googleversion and raised immediately if unsupported — so this check ran on the un-rendered Jinja expression instead of the actual rendered value.Moved the compatibility check from
__init__intoexecute(), right beforematch_globis used to build the GCS list call.__init__now only does a plain assignment._is_match_glob_supporteditself stays computed in__init__, since it depends on the installed provider version, not on any template field.__init__;self.match_glob = match_globis now a plain assignment.execute(), guarded by_is_match_glob_supported..execute()instead of construction.validate_operators_init_exemptions.txt.Verified locally that
scripts/ci/prek/validate_operators_init.pyreports zero findings for this class after the change, and that the existing unit tests intest_gcs_to_wasb.pypass with the updated test.Related to #70296
Gen-AI disclosure: I used a generative AI tool to help identify the root
cause, write tests, and draft the PR description. I reviewed, tested, and
verified all changes locally before submitting.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude following the guidelines