Check GCSToAzureBlobStorageOperator match_glob support after template… - #70574
Check GCSToAzureBlobStorageOperator match_glob support after template…#70574bramhanandlingala wants to merge 3 commits into
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
| container_name=CONTAINER, | ||
| match_glob="**/*.csv", | ||
| ) | ||
| op.execute(context=None) |
There was a problem hiding this comment.
context=None works only because the raise happens before anything touches the context. It's a bit fragile as documentation of intent — if the guard later moves below something that reads context, this fails with an unrelated AttributeError rather than the assertion you meant.
mock.MagicMock() (as #70529 uses for the same situation) or {} would be more robust. Also worth adding mock_gcs_hook.return_value.list.assert_not_called() — #70723's equivalent test does that, and it's what actually proves the operator bailed out before doing any work rather than just raising somewhere.
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
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