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: RedshiftDataHook and RdsHook not use cached connection #24387

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

Taragolis
Copy link
Contributor

After finding in this PR: #24057 (comment) I've checked other AWS Hooks which not use cached conn property and found two hooks RedshiftDataHook and RdsHook

Add to AwsBaseHook generic connection, so it won't required anymore overwrite conn property

Type hinting still work in IDE (PyCharm) after changes

out

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jun 11, 2022
@Taragolis Taragolis force-pushed the fix-amazon-hook-generic-conn branch from 331eb02 to 8458542 Compare June 13, 2022 07:26
@potiuk
Copy link
Member

potiuk commented Jun 13, 2022

LGTM. @ferruzzi @o-nikolas WDYT?

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 13, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@Taragolis
Copy link
Contributor Author

I've just wondering that in amazon-provider boto3-stubs packages defined in main package requirements is it better also move to mypy_dependencies section? I think it won't required in Airflow cluster, only for developing.

airflow/setup.py

Lines 201 to 202 in dfdf8eb

'mypy-boto3-rds>=1.21.0',
'mypy-boto3-redshift-data>=1.21.0',

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

I've just wondering that in amazon-provider boto3-stubs packages defined in main package requirements is it better also move to mypy_dependencies section? I think it won't required in Airflow cluster, only for developing.

airflow/setup.py

Lines 201 to 202 in dfdf8eb

'mypy-boto3-rds>=1.21.0',
'mypy-boto3-redshift-data>=1.21.0',

Not really. Mypy Section is for "generic" mypuy dependencies that are required for Arflow "core" (in development but still). We are likely going to reshuffle deps soon, preparing to splitting providers from Airlflow core and the "amazon" specific dependencies will land in setup.py of amazon provider eventually - they will be completely removed from the "airflow" setup.py.

So it's better to keep them where they are for now. We are going to likely make also each provider to have their own [devel] extra when we split out (or similar), so they should likely go there.

@potiuk
Copy link
Member

potiuk commented Jun 14, 2022

soon = few months from now. I am gearing up to write detailed description/AIP of what's needed there (I will ask for help on reviewing and finalizing when I get there).

@potiuk potiuk merged commit 796e0a0 into apache:main Jun 19, 2022
@Taragolis Taragolis deleted the fix-amazon-hook-generic-conn branch August 6, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants