SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714229811
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None,
fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None,
fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
I found the PR which introduced this change:
https://github.com/apache/airflow/pull/4723/files
But this was because at the time, the `get_hook` function looked very
different:
https://github.com/apache/airflow/blob/b1acc5508a4b9aaa7c98104ba31ab0623c95f4aa/airflow/models/connection.py#L184-L190
And the `google_cloud_platform` conn type returned a BigQueryHook.
So I think you're right that this is incorrect, since this would allow
someone to create a SqlSensor with a `ComputeEngineSSHHook`, which certainly
wouldn't work.
cc @XD-DENG I think a refactor might be required here - thoughts on either:
- updating the list of conn_types to include some new connections
- changing the check here to assert that a connection is a subclass of a
dbApiHook (and thus has a get_records method).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
