You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vatsrahul1001 (via GitHub)" <gi...@apache.org> on 2025/03/05 16:47:42 UTC
[PR] Add support for get_hook in connection using task sdk [airflow]
vatsrahul1001 opened a new pull request, #47401:
URL: https://github.com/apache/airflow/pull/47401
closes: https://github.com/apache/airflow/issues/47353
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47401:
URL: https://github.com/apache/airflow/pull/47401#discussion_r1983568654
##########
task_sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -56,7 +58,28 @@ class Connection:
def get_uri(self): ...
- def get_hook(self): ...
+ def get_hook(self, *, hook_params=None):
+ """Return hook based on conn_type."""
+ from airflow.providers_manager import ProvidersManager
+ from airflow.utils.module_loading import import_string
Review Comment:
Because this is in the Task SDK, and long term we don't want to have mixed imports, but short term delay the import of code outside of Task SDK to the specific place it is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 commented on code in PR #47401:
URL: https://github.com/apache/airflow/pull/47401#discussion_r1983575187
##########
task_sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -56,7 +58,28 @@ class Connection:
def get_uri(self): ...
- def get_hook(self): ...
+ def get_hook(self, *, hook_params=None):
+ """Return hook based on conn_type."""
+ from airflow.providers_manager import ProvidersManager
+ from airflow.utils.module_loading import import_string
Review Comment:
Should we do the moving of `ProvidersManager ` in this PR only, or that can be a separate one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #47401:
URL: https://github.com/apache/airflow/pull/47401#discussion_r1982058619
##########
task_sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -56,7 +58,28 @@ class Connection:
def get_uri(self): ...
- def get_hook(self): ...
+ def get_hook(self, *, hook_params=None):
+ """Return hook based on conn_type."""
+ from airflow.providers_manager import ProvidersManager
+ from airflow.utils.module_loading import import_string
Review Comment:
why not top-level import?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #47401:
URL: https://github.com/apache/airflow/pull/47401
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47401:
URL: https://github.com/apache/airflow/pull/47401#discussion_r1983571118
##########
task_sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -56,7 +58,28 @@ class Connection:
def get_uri(self): ...
- def get_hook(self): ...
+ def get_hook(self, *, hook_params=None):
+ """Return hook based on conn_type."""
+ from airflow.providers_manager import ProvidersManager
+ from airflow.utils.module_loading import import_string
Review Comment:
This'll work for now, but oh boy do we need to find a solution for ProvidersManager inside task sdk.
We might have to move _that_ class in to the SDK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for get_hook in connection using task sdk [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47401:
URL: https://github.com/apache/airflow/pull/47401#discussion_r1983609761
##########
task_sdk/src/airflow/sdk/definitions/connection.py:
##########
@@ -56,7 +58,28 @@ class Connection:
def get_uri(self): ...
- def get_hook(self): ...
+ def get_hook(self, *, hook_params=None):
+ """Return hook based on conn_type."""
+ from airflow.providers_manager import ProvidersManager
+ from airflow.utils.module_loading import import_string
Review Comment:
Separate one. I'll create a new ticket for it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org