Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test-case for callable values in path and query parameters of MSGraphAsyncOperator #43799

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Nov 7, 2024

This PR allow callable values to be passed into the path and query parameters so expensive evaluations (like Variable.get) don't get evaluated during DAG processing time. I'm aware you can get variables through jinja expressions, but in case of jinja None get's evaluated as an empty string which causes issues when calling the MSGraph end point as key/value parameters with value None aren't taken into account while empty string is causing issues further on.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

…cified so those don't get evaluated during the DAG processing
@raphaelauv
Copy link
Contributor

all templated fields can already be a callable since airflow 2.10.0

@dabla
Copy link
Contributor Author

dabla commented Nov 10, 2024

all templated fields can already be a callable since airflow 2.10.0

Ow damn well that makes it easy then we may close this one.

@dabla dabla closed this Nov 10, 2024
@dabla
Copy link
Contributor Author

dabla commented Nov 10, 2024

Maybe will reopen it and keep the tests but without additional code

@dabla dabla reopened this Nov 10, 2024
@dabla
Copy link
Contributor Author

dabla commented Nov 10, 2024

all templated fields can already be a callable since airflow 2.10.0

Indeed templated fields are callable, but here I also support callable values of a dict key/value pair which is passed to a templated field, which is not supported by default in templated fields. So this PR still makes sense.

@dabla
Copy link
Contributor Author

dabla commented Nov 12, 2024

I've removed the code regarding callable evaluation within dict values for templated fields and I've adapted the tests so it uses the already existing behaviour of the callable templated fields in Airflow.

@raphaelauv
Copy link
Contributor

I invite you to open a new PR , I don't understand what new feature you propose

@dabla
Copy link
Contributor Author

dabla commented Nov 12, 2024

I invite you to open a new PR , I don't understand what new feature you propose

I removed the feature, I just updated the tests using the callable as of 2.10. My proposition was to also allow callables within dict parameters, but I stepped away from it. I now just use the existing Airflow feature but updated the tests accordingly

@dabla dabla changed the title Allow callable values in path and query parameters of MSGraphAsyncOperator Also test callable values in path and query parameters of MSGraphAsyncOperator Nov 12, 2024
@dabla dabla changed the title Also test callable values in path and query parameters of MSGraphAsyncOperator Added test-case for callable values in path and query parameters of MSGraphAsyncOperator Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants