You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/17 10:24:07 UTC
[GitHub] [airflow] kazanzhy opened a new pull request #19639: Remove `allowed_conn_types` from SqlSensor
kazanzhy opened a new pull request #19639:
URL: https://github.com/apache/airflow/pull/19639
Removed hardcoded connection types. Check if the hook is an instance of DbApiHook.
--
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
[GitHub] [airflow] uranusjr commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971483609
Oh no the test suite doesn’t like this
```
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke: airflow.exceptions.AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_failure: airflow.exceptions.AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_failure_success: airflow.exceptions.AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_failure_success_same: airflow.exceptions.AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_invalid_failure: AssertionError: assert 'self.failure...llable -> [1]' == 'The connecti...Got MagicMock'
- The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
+ self.failure is present, but not callable -> [1]
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_invalid_success: AssertionError: assert 'self.success...llable -> [1]' == 'The connecti...Got MagicMock'
- The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
+ self.success is present, but not callable -> [1]
tests/sensors/test_sql_sensor.py::TestSqlSensor::test_sql_sensor_postgres_poke_success: airflow.exceptions.AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
```
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#discussion_r751117723
##########
File path: airflow/sensors/sql.py
##########
@@ -70,27 +71,13 @@ def __init__(
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
Review comment:
Note: apparently there was a mistake there - > we changed the type of connection for BigQueryHook to be 'gcpbigquery` (historically it used to be 'google_cloud_platform') so this would not work for BigQuery :( - but after that change it will work fine as BigQueryHook derives from DBApi
--
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
[GitHub] [airflow] uranusjr commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-981866151
@kazanzhy I think there are a few unrelated commits being added
--
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
[GitHub] [airflow] kazanzhy commented on pull request #19639: Draft: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-983620791
@potiuk @uranusjr @SamWheating
I did my best and seems the tests are passed.
But I realize that this solution is not elegant and might be not correct, therefore I want to ask for advice from more experienced colleagues.
In this PR there is a known problem of `mock` and `isinstance` conflict. I tried to patch different classes and got the following issues:
```
'airflow.sensors.sql.DbApiHook' -> TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types
'airflow.sensors.sql.BaseHook' -> The associated hook should be a subclass of `DbApiHook`. Got MagicMock
'airflow.hooks.base.BaseHook' -> Does not work as mock and trying to connect to database
```
Here is my toy script for testing this situation:
```
import unittest
from unittest import mock
class Connection:
def get_hook(self):
return BigQeryHook()
class BaseHook:
@classmethod
def get_connection(cls):
return Connection()
class DbApiHook(BaseHook): pass
class BigQeryHook(DbApiHook): pass
class SqlSensor:
def _get_hook(self):
hook = BaseHook.get_connection().get_hook()
print('Sensor:', type(hook), hook)
if not isinstance(hook, DbApiHook):
raise Exception(f'The associated hook should be a subclass of `DbApiHook`. Got {hook.__class__.__name__}')
def poke(self):
self._get_hook()
class TestSqlSensor1(unittest.TestCase):
@mock.patch('__main__.BaseHook')
def test_something(self, mock_hook):
#mock_hook.get_connection.return_value.get_hook.return_value = mock.MagicMock(spec=DbApiHook)
SqlSensor().poke()
class TestSqlSensor2(unittest.TestCase):
@mock.patch('__main__.DbApiHook')
def test_something(self, mock_hook):
SqlSensor().poke()
if __name__ == '__main__':
SqlSensor().poke()
unittest.main()
```
--
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
[GitHub] [airflow] potiuk commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971526642
You can add `spec` to MagicMock creation to pass "isinstance" test: see https://stackoverflow.com/questions/11146725/isinstance-and-mocking/26567750#26567750 (and the following comment shows simple example how to do 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
[GitHub] [airflow] potiuk commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-982046374
Tests failing :(
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971449783
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] potiuk merged pull request #19639: Draft: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19639:
URL: https://github.com/apache/airflow/pull/19639
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#discussion_r751125974
##########
File path: airflow/sensors/sql.py
##########
@@ -70,27 +71,13 @@ def __init__(
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
Review comment:
No no, that was just a note. It's good as you proposed, your change actually fixes the error that we had before.
--
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
[GitHub] [airflow] kazanzhy commented on a change in pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#discussion_r751122784
##########
File path: airflow/sensors/sql.py
##########
@@ -70,27 +71,13 @@ def __init__(
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
Review comment:
Is it necessary to change to:
```
if not isinstance(hook, DbApiHook) or conn.conn_type not in {'google_cloud_platform'}
```
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#discussion_r758930299
##########
File path: tests/sensors/test_sql_sensor.py
##########
@@ -86,8 +86,8 @@ def test_sql_sensor_postgres(self):
)
op2.run(start_date=DEFAULT_DATE, end_date=DEFAULT_DATE, ignore_ti_state=True)
- @mock.patch('airflow.sensors.sql.BaseHook')
- def test_sql_sensor_postgres_poke(self, mock_hook):
+ def test_sql_sensor_postgres_poke(self):
+ mock_hook = mock.MagicMock(spec=BaseHook)
Review comment:
I think you need to do this instead (same for all other tests)
```suggestion
@mock.patch('airflow.sensors.sql.BaseHook', new_callable=mock.MagicMock, spec=True)
def test_sql_sensor_postgres_poke(self, mock_hook):
```
`spec=True` is the same as `spec=BaseHook` without needing to import the class.
--
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
[GitHub] [airflow] kazanzhy commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-982057405
@potiuk give me, please few days to figure out how to modify tests and avoid this error:
```
AirflowException: The connection type is not supported by SqlSensor. The associated hook should be a subclass of `DbApiHook`. Got MagicMock
```
--
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
[GitHub] [airflow] kazanzhy commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-981987712
Thank you @uranusjr. Somehow I missed it.
Fixed and rebased
--
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
[GitHub] [airflow] potiuk commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971472896
> @potiuk I think a test for this case already exists
>
> https://github.com/apache/airflow/blob/6df69df421dfc6f1485f637aa74fc3dab0e2b9a4/tests/sensors/test_sql_sensor.py#L40
Good call!
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#discussion_r751117723
##########
File path: airflow/sensors/sql.py
##########
@@ -70,27 +71,13 @@ def __init__(
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
Review comment:
Note: apparently there was a mistake there - > we changed the type of connection for BigQueryHook to be 'gcpbigquery` (historically it used to be 'google_cloud_platform' so this would not work for BigQuery :( - but after that change it will work fine as BigQueryHook derives from DBApi
--
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
[GitHub] [airflow] kazanzhy commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971470093
@potiuk
I think a test for this case already exists
https://github.com/apache/airflow/blob/6df69df421dfc6f1485f637aa74fc3dab0e2b9a4/tests/sensors/test_sql_sensor.py#L40
--
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
[GitHub] [airflow] kazanzhy edited a comment on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy edited a comment on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971516564
> Oh no the test suite doesn’t like this
>
Yep. That's not good. In this situation, I see two ways.
1. Inherit SqlSensor from BaseSensorOperator and BaseSqlOperator. The last one already makes this check.
2. Somehow edit all tests in `tests/sensors/test_sql_sensor.py` to solve these errors. But I haven't suggestions
--
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
[GitHub] [airflow] kazanzhy commented on pull request #19639: Remove `allowed_conn_types` from SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #19639:
URL: https://github.com/apache/airflow/pull/19639#issuecomment-971516564
> Oh no the test suite doesn’t like this
Yep. That's not good. In this situation, I see two ways.
1. Inherit SqlSensor from BaseSensorOperator and BaseSqlOperator. The last one already makes this check.
2. Somehow edit all tests in `tests/sensors/test_sql_sensor.py` to solve these errors. But I haven't suggestions
--
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