Keep S3DeleteObjectsOperator validation in __init__, narrow to ValueError - #70359
Conversation
boring-cyborg
Bot
added
area:dev-tools
area:providers
backport-to-v3-3-test
|
Sorry, the issue has been linked to the PR and should be unlinked to prevent the issue from closing. My apologies. |
shahar1
changed the title
No worries, it's a matter of changing the "resolves:" into "related:" :) |
|
Thanks for updating the title. Will commit the code to fix the failure in the static checks after they all finish running. The Tests (AMD) check was failing due to an incorrect number in the code. |
42f5c65 to
08f503f
Compare
|
@shahar1 please let me know if there's anything you want me to change in this PR. I fixed all the static bugs and checks from a few days ago. |
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Could you please focus the PR on a single provider? (I know that GCP compute operators are being handled in another PR, so maybe just focus on S3DeleteObjectsOperator)
|
Reverted changes! This PR is only focused on S3 now. @shahar1 |
|
Checks just passed. Please let me know what you think! |
AmoghAtreya
changed the title
|
The checks will pass once #70505 gets merged due to the condition being introduced there. |
Please merge from/rebase onto main, and push to your branch. It will retrigger the CI with latest changes (I'll approve if necessary). |
43f0541 to
7a1b525
Compare
|
Since all checks are passing, please let me know next steps @shahar1! |
shahar1
changed the title
shahar1
changed the title
|
@AmoghAtreya |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Thank you so much for working with me through this and reviewing my code. Really appreciate that you took the time to help me out and provide feedback! Looking forward to contributing more. |
match_glob is a template field, so its value is not available until Jinja rendering happens on the worker, well after __init__ has run. The guard that rejects match_glob on an unsupported google provider tested it for truthiness, which the validate-operators-init prek hook flags. The check only asks whether the argument was supplied; it never inspects the value. Per the false-positive guidance on apache#70296 that makes it a provision check, which is fixed in place rather than moved to execute(). Moving it would break the check under render_template_as_native_obj=True, where a supplied field can render to None and so becomes indistinguishable from an omitted one, and would defer a static authoring mistake from Dag parse time to every task instance and retry. Switching to an explicit is not None comparison changes one case: match_glob="" is now rejected on a google provider older than 10.3.0, where truthiness previously read the supplied empty string as absent. An empty glob is not a valid pattern and was never honoured on those versions. Narrow the raised exception from AirflowException to ValueError, which is what an invalid argument warrants, and drop the file from generated/known_airflow_exceptions.txt accordingly. This follows the merged apache#70359 precedent for S3DeleteObjectsOperator, which made the same one-for-one change to the same four kinds of file. Also remove the class from the prek exemption list, since the hook fails on stale entries, and add tests covering supplied, empty-string and omitted match_glob. Related: apache#70296
match_glob is a template field, so its value is not available until Jinja rendering happens on the worker, well after __init__ has run. The guard that rejects match_glob on an unsupported google provider tested it for truthiness, which the validate-operators-init prek hook flags. The check only asks whether the argument was supplied; it never inspects the value. Per the false-positive guidance on apache#70296 that makes it a provision check, which is fixed in place rather than moved to execute(). Moving it would break the check under render_template_as_native_obj=True, where a supplied field can render to None and so becomes indistinguishable from an omitted one, and would defer a static authoring mistake from Dag parse time to every task instance and retry. Switching to an explicit is not None comparison changes one case: match_glob="" is now rejected on a google provider older than 10.3.0, where truthiness previously read the supplied empty string as absent. An empty glob is not a valid pattern and was never honoured on those versions. Narrow the raised exception from AirflowException to ValueError, which is what an invalid argument warrants, and drop the file from generated/known_airflow_exceptions.txt accordingly. This follows the merged apache#70359 precedent for S3DeleteObjectsOperator, which made the same one-for-one change to the same four kinds of file. Also remove the class from the prek exemption list, since the hook fails on stale entries, and add tests covering supplied, empty-string and omitted match_glob. Related: apache#70296
2 participants
Description
related: #70296 — burn-down of
__init__-time validation exemptions.Changes Made (
S3DeleteObjectsOperator,amazon):__init__argument-combination guard with explicitis not Nonechecks so the
validate-operators-inithook recognizes it (the oldcomprehension-based check read as a false positive). The logic is unchanged —
equivalent on every input combination.
AirflowExceptiontoValueError(parse-time only; the
execute()-time guard is deliberately retainedunchanged as a safety net for templated
keysrendering toNone).S3DeleteObjectsOperatorentry fromscripts/ci/prek/validate_operators_init_exemptions.txtand updated thegenerated/known_airflow_exceptions.txtcount (5 → 4).ValueError.works on: #70296
Was generative AI tooling used to co-author this PR?
Generated-by: Claude following the guidelines