Skip to content

More explicit mapped argument validation#21933

Merged
ashb merged 2 commits intoapache:mainfrom
astronomer:operator-mapping-validation
Mar 3, 2022
Merged

More explicit mapped argument validation#21933
ashb merged 2 commits intoapache:mainfrom
astronomer:operator-mapping-validation

Conversation

@uranusjr
Copy link
Copy Markdown
Member

@uranusjr uranusjr commented Mar 2, 2022

Instead of always using MagicMock to validate mapped arguments, this implements a more sophisticated protocol that allows an operator to implement a validate_mapped_arguments to provide custom validation logic. If an operator just wants to use __init__ for validation, however, they can set a flag mapped_arguments_validated_by_init to get the behavior easily. (This does not use MagicMock, however, since any custom validation logic should be able to handle those on its own).

The validate_mapped_arguments flag is currently only set on PythonOperator. It can likely be used on a lot more operators down the road.

uranusjr added 2 commits March 2, 2022 10:09
Instead of always using MagicMock to validate mapped arguments, this
implements a more sophisticated protocol that allows an operator to
implement a 'validate_mapped_arguments' to provide custom validation
logic. If an operator just wants to use __init__ for validation,
however, they can set a flag 'mapped_arguments_validated_by_init' to get
the behavior easily. (This does *not* use MagicMock, however, since any
custom validation logic should be able to handle those on its own).

The 'validate_mapped_arguments' flag is currently only set on
PythonOperator. It can likely be used on a lot more operators down the
road.
There's just too much magic during a task's initialization that tries to
add it into the dependency graph. This flag is needed to work around all
that, I think.
@uranusjr uranusjr force-pushed the operator-mapping-validation branch 3 times, most recently from 39636eb to 3cccb20 Compare March 3, 2022 09:51
@uranusjr
Copy link
Copy Markdown
Member Author

uranusjr commented Mar 3, 2022

Should be ready!

@uranusjr uranusjr marked this pull request as ready for review March 3, 2022 09:51
@uranusjr uranusjr requested review from XD-DENG and kaxil as code owners March 3, 2022 09:51
@uranusjr uranusjr requested a review from ashb March 3, 2022 09:52
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 3, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 3, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ashb ashb force-pushed the operator-mapping-validation branch from 6a23699 to 3cccb20 Compare March 3, 2022 14:58
@ashb ashb merged commit b65e522 into apache:main Mar 3, 2022
@ashb ashb deleted the operator-mapping-validation branch March 3, 2022 17:10
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dynamic-task-mapping AIP-42 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants