Resolve AzureFileShareToGCSOperator directory_name alias after rendering - #70740
Resolve AzureFileShareToGCSOperator directory_name alias after rendering#70740Andrushika wants to merge 3 commits into
Conversation
directory_name and directory_path are both template fields, rendered after the constructor runs. Resolving the deprecated directory_name -> directory_path alias in __init__ meant the provision decision was made on the un-rendered values; under render_template_as_native_obj a supplied directory_path can render to None, so the decision has to be taken before rendering while a supplied-but-None field is still distinguishable from a missing one, and the copy has to happen after, on the real values. Keeping it in the constructor also tripped the validate-operators-init guard tracked by apache#70296.
boring-cyborg
Bot
added
area:dev-tools
area:providers
backport-to-v3-3-test
|
cc @shahar1 |
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Thanks for taking this on — moving the alias out of __init__ is the right direction, and the pre-render/post-render split is a genuinely subtle point that the second test documents well.
One blocking issue: the render_template_fields override never runs for mapped tasks, so this turns a currently-working case into a runtime failure. Details inline.
Smaller observations:
test_no_directory_name_deprecation_without_directory_namepasses unchanged onmain— it exercises pre-existing__init__behaviour rather than anything this PR changes. Per AGENTS.md Testing Standards: "every test must fail without the PR's change... Do not add tests for pre-existing logic that was already present before the PR." (test_native_directory_path_rendering_to_none_is_not_aliasedis fine — it's an explicit behaviour-preservation guard, which is the documented exception.)- Nit: the class docstring still says "Note that
share_name,directory_path,prefix, anddest_gcsare templated" — worth addingdirectory_name, since making its templating actually work is what this PR is about.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. After you've
addressed the points above and pushed an update, an Apache Airflow
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
MappedOperator.render_template_fields calls _do_render_template_fields on the unmapped task directly, so render_template_fields overrides never run for mapped tasks and the alias was lost on that path. execute() runs on both the mapped and unmapped paths, after rendering; the was-it-passed decision stays in __init__ where the un-rendered values are visible.
|
Thanks for reviewing, Shahar! I've addressed the requested changes. Could you take another look when you have time? |
2 participants
Resolve AzureFileShareToGCSOperator directory_name alias after rendering
Why
directory_nameanddirectory_pathare both template fields, rendered after__init__runs. The old constructor resolved the deprecateddirectory_name->directory_pathalias there, on the un-rendered values, which #70296 flags.What
Move the alias copy into
render_template_fields, notexecute. The alias needs to know ifdirectory_pathwas passed, and withrender_template_as_native_obj=Truea supplieddirectory_pathcan render toNone. So the check runs before rendering, where the raw values are still visible, and the copy runs after. The deprecation warning stays in__init__.related: #70296
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines
{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.