Skip to content

Conversation

@amoghrajesh
Copy link
Contributor

closes: #52520

During the migration of BaseHook to task sdk, I introduced a temporary workaround in the API server context:
https://github.com/apache/airflow/pull/51873/files#diff-acfa34467241916c1189174e98f87a4094e8cbf58d391fa084e6afb2594decbcR61-R71 to help solve some compat issues which arrived along the way.

When in the API server context, the system uses the Connection model (via custom backend > env var > DB fallback) directly.

This issue was on hold because it was dependent on coupling between Git bundle and API server. For view_url use case, a slim git bundle is instantiated to form the view_url leading to it using BaseHook to get a connection and BaseHook uses Connection from SDK, it goes back to API server causing a self loop scenario.

That has been fixed via: #52876 so attempting the removal again


^ 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 airflow-core/newsfragments.

@amoghrajesh amoghrajesh requested review from ashb and kaxil as code owners August 4, 2025 08:54
@amoghrajesh amoghrajesh changed the title Replace API server’s direct Connection access workaround Replace API server’s direct Connection access workaround in BaseHook Aug 4, 2025
@amoghrajesh amoghrajesh self-assigned this Aug 4, 2025
@amoghrajesh amoghrajesh added this to the Airflow 3.1.0 milestone Aug 4, 2025
@amoghrajesh
Copy link
Contributor Author

Created the PR, let's see what tests start to fail.

@amoghrajesh
Copy link
Contributor Author

Core failures are handled by #54084

@amoghrajesh amoghrajesh requested a review from o-nikolas as a code owner August 5, 2025 07:33
@amoghrajesh
Copy link
Contributor Author

Alright, fighting with the CI for compat + provider tests!

@amoghrajesh amoghrajesh requested a review from Fokko as a code owner August 6, 2025 13:46
@amoghrajesh
Copy link
Contributor Author

Alright, kube tests should be fixed by: #54261

Last set of compat failing!

@amoghrajesh
Copy link
Contributor Author

@ashb I made updates on this one, could you take another look when possible?

@amoghrajesh
Copy link
Contributor Author

Failing K8s tests to be fixed by #54385

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.

Nice one!

@amoghrajesh amoghrajesh merged commit 845f775 into apache:main Aug 19, 2025
105 checks passed
@amoghrajesh amoghrajesh deleted the remove-compat-shim-basehook branch August 19, 2025 04:22
@amoghrajesh
Copy link
Contributor Author

@seanghaeli the eventual goal is to move towards using the task sdk for these sort of things, so I'd suggest that the task sdk is installed

@potiuk
Copy link
Member

potiuk commented Aug 20, 2025

@seanghaeli the eventual goal is to move towards using the task sdk for these sort of things, so I'd suggest that the task sdk is installed

Ah you are right :)

@potiuk
Copy link
Member

potiuk commented Aug 20, 2025

But also @amoghrajesh -> we should handle the case where system tests are run on Airflow 2 -> I guess we cannot install task.sdk on 2 and we will never aim to do it ?

@amoghrajesh
Copy link
Contributor Author

@potiuk @o-nikolas I do not think that should be an issue at all.

For system tests running on Airflow 2, we use the BaseHook class + deps from Airflow 2 itself. In other words, the BaseHook doesnt come from task sdk in those cases.

Most hook imports for amazon systests are from base_aws, and see the import for BaseHook there: https://github.com/apache/airflow/blob/main/providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py#L63 (its from a compat layer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Replace API server’s direct Connection access workaround before 3.1

3 participants