-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Move BaseHook
class to task SDK
#51873
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
base: main
Are you sure you want to change the base?
Conversation
4106b95
to
f166ee4
Compare
# TODO: Revisit this | ||
status, message = conn.test_connection() # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work or fail currently -- with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically haven't made any change here, just to think a bit loud, this one can use Connection
from task SDK right?
Right now its not
try: | ||
from airflow.sdk import BaseHook | ||
except ImportError: | ||
from airflow.hooks.base import BaseHook # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be stricter:
from airflow.hooks.base import BaseHook # type: ignore | |
from airflow.hooks.base import BaseHook # type: ignore[no-redef] |
try: | ||
from airflow.sdk import BaseHook | ||
except ImportError: | ||
from airflow.hooks.base import BaseHook # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from airflow.hooks.base import BaseHook # type: ignore | |
from airflow.hooks.base import BaseHook # type: ignore[no-redef] |
try: | ||
from airflow.sdk import BaseHook | ||
except ImportError: | ||
from airflow.hooks.base import BaseHook # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from airflow.hooks.base import BaseHook # type: ignore | |
from airflow.hooks.base import BaseHook # type: ignore[no-redef] |
@@ -93,7 +97,7 @@ def __init__(self, region: str | None = None, oss_conn_id="oss_default", *args, | |||
|
|||
def get_conn(self) -> Connection: | |||
"""Return connection for the hook.""" | |||
return self.oss_conn | |||
return self.oss_conn # type: ignore[return-value] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can/should be fixed -- if you change L37 to be Connection from SDK
@@ -127,7 +127,10 @@ def conn_config(self) -> AwsConnectionWrapper: | |||
) | |||
|
|||
return AwsConnectionWrapper( | |||
conn=connection, region_name=self._region_name, botocore_config=self._config, verify=self._verify | |||
conn=connection, # type: ignore[arg-type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
try: | ||
from airflow.sdk import BaseHook | ||
except ImportError: | ||
from airflow.hooks.base import BaseHook # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth moving to version_compat.py -- same as #52528
@@ -69,7 +69,7 @@ def _get_webhook_endpoint(self, conn_id: str) -> str: | |||
token = conn.password | |||
if token is None: | |||
raise AirflowException("Webhook token field is missing and is required.") | |||
url = conn.schema + "://" + conn.host | |||
url = cast("str", conn.schema) + "://" + cast("str", conn.host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add an error above, since you expect conn.schema
and conn.host
to be non-None -- similar to token
field above
try: | ||
from airflow.sdk import BaseHook | ||
except ImportError: | ||
from airflow.hooks.base import BaseHook # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictier typer ignore please for all
@kaxil thanks for the review, pretty positive ill be getting a green run now. Will look at your comments tomorrow! |
closes: #51672
Moving the BaseHook class into task SDK, exactly where it should live similar to other base classes in sdk like notifiers, operators etc.
Testing: Running by all the methods defined on BaseHook -- old vs new
Running with the new path
DAG:
Running with the older path to check backcompat
^ 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.