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

Session is not used in _do_render_template_fields #37856

Merged

Conversation

dstandish
Copy link
Contributor

It's not used so, we should be able to remove it. The context here is AIP-44 -- it's important to identify which functions require database access and which don't.

It's not used so, we should be able to remove it.  The context here is AIP-44 -- it's important to identify which functions require database access and which don't.
@dstandish dstandish requested a review from uranusjr as a code owner March 3, 2024 05:06
@dstandish dstandish requested review from potiuk and mhenc March 3, 2024 05:06
Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Don't we need to check for all operators?

@dstandish
Copy link
Contributor Author

i found one more place where session was being passed.

in case you are curious, we have not used session (at least not directly) in this method since this change:
5d1270c#diff-f373d874912ccfa03918e853ad15aa91d6bfaa1ee75f1676f78c8a756f332ed0L351

just want to double check with @uranusjr --- any reason not to merge this PR?

@uranusjr
Copy link
Member

uranusjr commented Mar 4, 2024

I don’t quite recall why we removed the session=session part in the change. It seems like a nice optimisation to commit eagerly when rendering nested fields. Maybe there are reasons. Since the current code works fine I guess it is worthwhile to at least try to remove the argument. _do_render_template_fields is supposed to be private-ish and shouldn’t be overriden very often.

@dstandish dstandish merged commit 1726b93 into apache:main Mar 4, 2024
59 checks passed
@dstandish dstandish deleted the _do_render_template_fields-no-need-session branch March 4, 2024 21:15
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
It's not used so, we should be able to remove it.  The context here is AIP-44 -- it's important to identify which functions require database access and which don't.  For reference, session has not been used in this function since apache@5d1270c#diff-f373d874912ccfa03918e853ad15aa91d6bfaa1ee75f1676f78c8a756f332ed0L351.
@utkarsharma2 utkarsharma2 added this to the Airflow 2.8.3 milestone Mar 6, 2024
@utkarsharma2 utkarsharma2 added the type:improvement Changelog: Improvements label Mar 6, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
It's not used so, we should be able to remove it.  The context here is AIP-44 -- it's important to identify which functions require database access and which don't.  For reference, session has not been used in this function since apache@5d1270c#diff-f373d874912ccfa03918e853ad15aa91d6bfaa1ee75f1676f78c8a756f332ed0L351.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement Changelog: Improvements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants