Skip to content

Conversation

@Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Jan 25, 2024

related: #36948 (comment)

We need to properly sanitise logger_name in providers Hooks kwargs in case of if Airflow < 2.8 uses.
The faster way is just revert this changes and implements in backward compatible way on next wave of provider release

This reverts commit 6bd450d.


^ 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.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

My bad, I just tested with 2.8.1

However, since it impacts only the classes where I provided the logger name explicitly, why do we need to revert the whole change?

@hussein-awala
Copy link
Member

I know it's faster, but with a simple reset soft for the revert commit, we can easily exclude the classes where it's not provided explicitly, wdyt?

@potiuk
Copy link
Member

potiuk commented Jan 25, 2024

Yeah. I thought a bit about it and I think reverting is the fastest and most secure way. This way we can release RC2 now, and properly get it implemented and tested. There are a bit unexpected scenarios, where passing **kwargs is a bad idea.

I think ince BaseHook does not take **kwargs, we cannot just pass the kwargs if we want to support all the scenarios explaied in the #36948 (comment)

I will revert and re-release the next wave with accelerated voring, and we should implement the log_name as specific, new parameter for all the hooks and get common sanitization pattern. Having **kwargs supported in "blanket" way and passing it down to the BaseHook that does not support **kwargs opens up to too many potential issues.

BTW. @eladkal is flying tonight and he asked me to take over - so I will take it from here.

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