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 backwards compatibility for RedshiftSQLOperator #27602

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

syedahsn
Copy link
Contributor

With the introduction of SQLExecuteQueryOperator, the RedshiftSQLOperator was broken. This was due to the parameter redshift_conn_id not being used. This PR fixes the issue by introducing the redshift_conn_id as a variable to pass the connection id through to the base operator.

related: #25717


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

@syedahsn syedahsn requested a review from eladkal as a code owner November 10, 2022 19:54
@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Nov 10, 2022
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nope. Why? I do not understand it. the redshift_conn_id was used. What is the problem you wanted to solve?

@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

How broken was it ? the parameter was used actually, so what you wrote does not seem correctl, maybe you expected redshift_conn_id to be set as field, but this is super-internal-detail and you should not rely on it.

@syedahsn - could you please explain?

@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2022

@potiuk the problem is templating
#25717 (comment)

@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

@potiuk the problem is templating #25717 (comment)

Then the right fix for it is removing "redshift_conn_id" from templated fields, not doing what the PR is doing (and adding conn_id).

@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2022

Then the right fix for it is removing "redshift_conn_id" from templated fields, not doing what the PR is doing.

This is a deprecared operator.
Users had redshift_con as templated field and when we added generic sql operator it broke this behavior unexpectedly. I think the goal of this PR is just to get back the functionality. Ideally we should have notice it before releasing the new operator.

@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

@syedahsn - can you please do it ? rather than adding some init_code, just removig "redshift_conn_id"/adding "conn_id" to templated_fields should fix the problem :)

@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2022

@syedahsn - can you please do it ? rather than adding some init_code, just removig "redshift_conn_id"/adding "conn_id" to templated_fields should fix the problem :)

Oh yeah. Right 👍

@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

Then the right fix for it is removing "redshift_conn_id" from templated fields, not doing what the PR is doing.

This is a deprecared operator. Users had redshift_con as templated field and when we added generic sql operator it broke this behavior unexpectedly. I think the goal of this PR is just to get back the functionality. Ideally we should have notice it before releasing the new operator.

The problem is not with init code, but with leaving redhshift_conn_id in templated_fields. everything will work when it is replaced with "conn_id".

@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2022

Yep. You are right. Its late hour here. I should not review code when I'm this tired 😴

@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

Been there, done that @eladkal :D

@syedahsn
Copy link
Contributor Author

If I remove redshift_conn_id from the signature of the operator, workflows that have been passing a redshift_conn_id would fail right? That was how the original break was caught.

@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2022

You dont remove from the signature. You only remove from the templated_fields

@syedahsn syedahsn requested review from eladkal and potiuk and removed request for eladkal and potiuk November 10, 2022 22:33
@potiuk potiuk merged commit f919abc into apache:main Nov 10, 2022
@potiuk
Copy link
Member

potiuk commented Nov 10, 2022

Way better :)

Adityamalik123 pushed a commit to Adityamalik123/airflow that referenced this pull request Nov 12, 2022
* Fix backwards compatibility for RedshiftSQLOperator

* Remove redshift_conn_id from template fields
@vandonr-amz vandonr-amz deleted the syedahsn/redshiftsql-redshift-conn-id branch May 24, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants