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
And it looks like at the time at the time, the `get_hook` function looked
very different and returned a BigQueryHook for the `google_cloud_platform` conn
type.:
https://github.com/apache/airflow/blob/b1acc5508a4b9aaa7c98104ba31ab0623c95f4aa/airflow/models/connection.py#L184-L190
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 - nice catch!
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]
