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

fix(providers/common/sql): add dummy connection setter for backward compatibility #42490

Merged

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Sep 26, 2024

Why

The introduction of connection property breaks apache-airflow-providers-mysql<5.7.1, apache-airflow-providers-elasticsearch<5.5.1 and apache-airflow-providers-postgres<5.13.0

What

Add a dummy connection setter

Closes: #42452


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

@Lee-W Lee-W requested a review from eladkal as a code owner September 26, 2024 04:12
@phanikumv phanikumv changed the title fix(providers/common/sql): add dummpy connection setter for backward compatibility fix(providers/common/sql): add dummy connection setter for backward compatibility Sep 26, 2024
@Lee-W Lee-W force-pushed the add-dummy-connection-setter-to-common-sql branch from e87f608 to 8b35d2b Compare September 26, 2024 07:23
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This stops the error, but why does it work? Were those providers never reading self.connection again?

@Lee-W
Copy link
Member Author

Lee-W commented Sep 26, 2024

This stops the error, but why does it work? Were those providers never reading self.connection again?

They're reading through the newly introduced property connection (since common-sql > 1.17.0)

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

Can you patch this change on old common/sql and test with the affected mysql?

My understanding is that the connection would be read from 'get_conn' which validates this change but yeah, let's test with a patch

…compatibility

the introduction of connection property breaks apache-airflow-providers-mysql<5.7.1,
apache-airflow-providers-elasticsearch<5.5.1
 and apache-airflow-providers-postgres<5.13.0
@Lee-W Lee-W force-pushed the add-dummy-connection-setter-to-common-sql branch from 8b35d2b to 8e3ea21 Compare September 26, 2024 08:16
@dabla
Copy link
Contributor

dabla commented Sep 26, 2024

Does the dummy setter solve this issue now? The problem is caused by my PR 40751.

@Lee-W
Copy link
Member Author

Lee-W commented Sep 26, 2024

Can you patch this change on old common/sql and test with the affected mysql?

My understanding is that the connection would be read from 'get_conn' which validates this change but yeah, let's test with a patch

Yep, works as expected

@dabla
Copy link
Contributor

dabla commented Sep 26, 2024

Can you patch this change on old common/sql and test with the affected mysql?
My understanding is that the connection would be read from 'get_conn' which validates this change but yeah, let's test with a patch

Yep, works as expected

Good to hear, sorry for the caused inconvenience.

@phanikumv phanikumv merged commit 7ad586e into apache:main Sep 26, 2024
55 checks passed
@phanikumv phanikumv deleted the add-dummy-connection-setter-to-common-sql branch September 26, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants