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 2022/02/13 09:41:51 UTC
[GitHub] [airflow] david30907d opened a new pull request #21542: (bigquery-hook): forgot to pass engine_kwargs in
david30907d opened a new pull request #21542:
URL: https://github.com/apache/airflow/pull/21542
related: https://github.com/apache/airflow/issues/21443
my bad, forgot to pass the `engine_kwargs` to instantiate a sqlalchemy engine
this is how I test it on my local
1. open localhost:8080 and then create a bigquery connection
2. test this snippet in shell
```python
from airflow.providers.google.cloud.hooks.bigquery import BigQueryHook
connection = BigQueryHook('bigquery')
isinstance(connection.get_sqlalchemy_engine(), sqlalchemy.engine.base.Engine) is True
```
---
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21542:
URL: https://github.com/apache/airflow/pull/21542
--
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 #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#issuecomment-1038016260
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main 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] david30907d commented on a change in pull request #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
david30907d commented on a change in pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#discussion_r805308216
##########
File path: airflow/providers/google/cloud/hooks/bigquery.py
##########
@@ -163,6 +163,8 @@ def get_sqlalchemy_engine(self, engine_kwargs=None):
:param engine_kwargs: Kwargs used in :func:`~sqlalchemy.create_engine`.
:return: the created engine.
"""
+ if engine_kwargs is None:
+ engine_kwargs = {}
Review comment:
copied this block from [DbAPI.py](https://github.com/apache/airflow/blob/b40dffa08547b610162f8cacfa75847f3c4ca364/airflow/hooks/dbapi.py#L91-L99)
##########
File path: tests/providers/google/cloud/hooks/test_bigquery.py
##########
@@ -946,6 +947,16 @@ def test_dbapi_get_sqlalchemy_engine(self):
):
self.hook.get_sqlalchemy_engine()
+ @mock.patch(
+ 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook._get_credentials_and_project_id',
+ return_value=(CREDENTIALS, PROJECT_ID),
+ )
+ def test_dbapi_get_sqlalchemy_engine_success(
+ self, mock_insert, mock_get_service, mock_get_creds_and_proj_id
+ ):
+ bq_hook = BigQueryHook(use_legacy_sql=False)
+ assert isinstance(bq_hook.get_sqlalchemy_engine(), sqlalchemy.engine.base.Engine) is True
Review comment:
added a test case for it
##########
File path: tests/providers/google/cloud/hooks/test_bigquery.py
##########
@@ -946,6 +947,16 @@ def test_dbapi_get_sqlalchemy_engine(self):
):
self.hook.get_sqlalchemy_engine()
+ @mock.patch(
+ 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook._get_credentials_and_project_id',
+ return_value=(CREDENTIALS, PROJECT_ID),
+ )
+ def test_dbapi_get_sqlalchemy_engine_success(
+ self, mock_insert, mock_get_service, mock_get_creds_and_proj_id
+ ):
+ bq_hook = BigQueryHook(use_legacy_sql=False)
+ assert isinstance(bq_hook.get_sqlalchemy_engine(), sqlalchemy.engine.base.Engine) is True
Review comment:
don't know how to set `extra__google_cloud_platform__key_path` into connection for testing
remove this unit test for now
--
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] david30907d commented on a change in pull request #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
david30907d commented on a change in pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#discussion_r805329368
##########
File path: tests/providers/google/cloud/hooks/test_bigquery.py
##########
@@ -946,6 +947,16 @@ def test_dbapi_get_sqlalchemy_engine(self):
):
self.hook.get_sqlalchemy_engine()
+ @mock.patch(
+ 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook._get_credentials_and_project_id',
+ return_value=(CREDENTIALS, PROJECT_ID),
+ )
+ def test_dbapi_get_sqlalchemy_engine_success(
+ self, mock_insert, mock_get_service, mock_get_creds_and_proj_id
+ ):
+ bq_hook = BigQueryHook(use_legacy_sql=False)
+ assert isinstance(bq_hook.get_sqlalchemy_engine(), sqlalchemy.engine.base.Engine) is True
Review comment:
don't know how to set `extra__google_cloud_platform__key_path` into connection for testing
remove this unit test for now
--
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 #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21542:
URL: https://github.com/apache/airflow/pull/21542
--
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 #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#issuecomment-1038016260
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main 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] david30907d commented on a change in pull request #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
david30907d commented on a change in pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#discussion_r805308244
##########
File path: tests/providers/google/cloud/hooks/test_bigquery.py
##########
@@ -946,6 +947,16 @@ def test_dbapi_get_sqlalchemy_engine(self):
):
self.hook.get_sqlalchemy_engine()
+ @mock.patch(
+ 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook._get_credentials_and_project_id',
+ return_value=(CREDENTIALS, PROJECT_ID),
+ )
+ def test_dbapi_get_sqlalchemy_engine_success(
+ self, mock_insert, mock_get_service, mock_get_creds_and_proj_id
+ ):
+ bq_hook = BigQueryHook(use_legacy_sql=False)
+ assert isinstance(bq_hook.get_sqlalchemy_engine(), sqlalchemy.engine.base.Engine) is True
Review comment:
added a test case 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
[GitHub] [airflow] david30907d commented on a change in pull request #21542: (bigquery-hook): forgot to pass engine_kwargs in
Posted by GitBox <gi...@apache.org>.
david30907d commented on a change in pull request #21542:
URL: https://github.com/apache/airflow/pull/21542#discussion_r805308216
##########
File path: airflow/providers/google/cloud/hooks/bigquery.py
##########
@@ -163,6 +163,8 @@ def get_sqlalchemy_engine(self, engine_kwargs=None):
:param engine_kwargs: Kwargs used in :func:`~sqlalchemy.create_engine`.
:return: the created engine.
"""
+ if engine_kwargs is None:
+ engine_kwargs = {}
Review comment:
copied this block from [DbAPI.py](https://github.com/apache/airflow/blob/b40dffa08547b610162f8cacfa75847f3c4ca364/airflow/hooks/dbapi.py#L91-L99)
--
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