Skip to content

Remove AIP-44 from models/renderedtifields - #44500

Closed
jason810496 wants to merge 1 commit into
apache:mainfrom
jason810496:fix/AIP-44/remove-renderedtifields
Closed

Remove AIP-44 from models/renderedtifields#44500
jason810496 wants to merge 1 commit into
apache:mainfrom
jason810496:fix/AIP-44/remove-renderedtifields

Conversation

@jason810496

Copy link
Copy Markdown
Member

related: #44436

  • ./airflow/models/renderedtifields.py: _update_runtime_evaluated_template_fields

@jason810496

Copy link
Copy Markdown
Member Author

Note

I thinks this method is not the case that:

re-join back methods that were separated out from the main code - when methods start with _

Only Edge-Worker is depends on this _update_runtime_evaluated_template_fields method, and there isn't update_runtime_evaluated_template_fields public method.

@potiuk

potiuk commented Nov 30, 2024

Copy link
Copy Markdown
Member

Note

I thinks this method is not the case that:

re-join back methods that were separated out from the main code - when methods start with _

Only Edge-Worker is depends on this _update_runtime_evaluated_template_fields method, and there isn't update_runtime_evaluated_template_fields public method.

This one needs a bit more effort I think. Actually this method seems to be nice idea to be renamed and added as "legit" method to use from this module. It's already used in renderedtifields.py and bash operator in standards provider (which is a bit problematic because bash operator should be backwards compatible - so it needs some compatibility code to make sure it also works for past Airflow versions when we rename it.

And I think it's entirely worth it.

@potiuk

potiuk commented Dec 1, 2024

Copy link
Copy Markdown
Member

Fixed in #44546

@potiuk potiuk closed this Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants