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 2020/11/24 15:06:23 UTC
[GitHub] [airflow] zhongjiajie opened a new pull request #7903: [WIP] Add property conn_name and connection to dbapi_hook
zhongjiajie opened a new pull request #7903:
URL: https://github.com/apache/airflow/pull/7903
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-656666942
Thanks @turbaszek @feluelle, now I rebase and push to restart the CI, will merge when it pass
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-920473615
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] stale[bot] commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-641929153
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-656714235
So my suggestion is to remove `conn_name_attr` and add a `*_conn_id` parameter to init. WDYT @turbaszek @zhongjiajie ?
As it is now I am not a fan of it anymore I am sorry @zhongjiajie but this looks really too complex, don't you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#discussion_r452870681
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -69,20 +69,20 @@ def __init__(
self._connect_kwargs = connect_kwargs
@property
- def connection(self):
+ def connection_(self):
Review comment:
```suggestion
def connection(self):
```
You can overwrite it.
##########
File path: airflow/providers/odbc/hooks/odbc.py
##########
@@ -69,20 +69,20 @@ def __init__(
self._connect_kwargs = connect_kwargs
@property
- def connection(self):
+ def connection_(self):
"""
``airflow.Connection`` object with connection id ``odbc_conn_id``
"""
if not self._connection:
- self._connection = self.get_connection(getattr(self, self.conn_name_attr))
+ self._connection = self.connection
Review comment:
```suggestion
self._connection = super().connection
```
##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -120,14 +120,15 @@ def _get_aws_credentials(self):
intended to be used by external import and export statements
"""
- if self.snowflake_conn_id: # pylint: disable=no-member
- connection_object = self.get_connection(self.snowflake_conn_id) # pylint: disable=no-member
- if 'aws_secret_access_key' in connection_object.extra_dejson:
- aws_access_key_id = connection_object.extra_dejson.get(
- 'aws_access_key_id')
- aws_secret_access_key = connection_object.extra_dejson.get(
- 'aws_secret_access_key')
- return aws_access_key_id, aws_secret_access_key
+ connection_object = self.connection
+ if 'aws_secret_access_key' in connection_object.extra_dejson:
+ aws_access_key_id = connection_object.extra_dejson.get(
+ 'aws_access_key_id')
+ aws_secret_access_key = connection_object.extra_dejson.get(
+ 'aws_secret_access_key')
+ return aws_access_key_id, aws_secret_access_key
+ else:
+ return None, None
Review comment:
This is unrelated.
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -181,8 +182,8 @@ def setUp(self):
)
self.db_hook = MySqlHook()
- self.db_hook.get_connection = mock.Mock()
- self.db_hook.get_connection.return_value = self.connection
+ self.db_hook._connection = mock.Mock()
+ self.db_hook._connection = self.connection
Review comment:
```suggestion
self.db_hook.connection = mock.Mock(return_value=self.connection)
```
##########
File path: tests/providers/mysql/hooks/test_mysql.py
##########
@@ -52,8 +52,8 @@ def setUp(self):
)
self.db_hook = MySqlHook()
- self.db_hook.get_connection = mock.Mock()
- self.db_hook.get_connection.return_value = self.connection
+ self.db_hook._connection = mock.Mock()
+ self.db_hook._connection = self.connection
Review comment:
```suggestion
self.db_hook.connection = mock.Mock(return_value=self.connection)
```
##########
File path: tests/providers/odbc/hooks/test_odbc.py
##########
@@ -17,35 +17,41 @@
# specific language governing permissions and limitations
# under the License.
#
-import json
+import unittest
from unittest import mock
from urllib.parse import quote_plus, urlparse
import pyodbc
+from parameterized import parameterized
from airflow.models import Connection
from airflow.providers.odbc.hooks.odbc import OdbcHook
-class TestOdbcHook:
- def get_hook(self=None, hook_params=None, conn_params=None):
- hook_params = hook_params or {}
- conn_params = conn_params or {}
- connection = Connection(
- **{
- **dict(login='login', password='password', host='host', schema='schema', port=1234),
- **conn_params,
- }
- )
+class TestOdbcHook(unittest.TestCase):
+
+ def setUp(self):
+ super().setUp()
+
+ self.conn = conn = mock.MagicMock()
+ self.conn.login = 'login'
+ self.conn.password = 'password'
+ self.conn.host = 'host'
+ self.conn.schema = 'schema'
+ self.conn.port = 1234
Review comment:
Please use the `Connection` class to init a connection like in the other cases.
For example:
```python
self.conn = mock.Mock(return_value=Connection(...))
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on issue #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-616594810
Two solutions CI passed, we have **two fix solutions** here, I prefer **the second one**, and it also the PR final status
* Add `# type: ignore` in L76 in dbapi_hook to avoid `error: Signature of "get_connection" incompatible with supertype "BaseHook"`
```diff
- def get_connection(self, conn_id: Optional[str] = None) -> Connection:
+ def get_connection(self, conn_id: Optional[str] = None) -> Connection: # type: ignore
```
* Using property function `connection` in dbapi_hook to get connection, which complete fix Signature error, and make dbapi_hook more easier.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #7903:
URL: https://github.com/apache/airflow/pull/7903
--
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] zhongjiajie removed a comment on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie removed a comment on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654977536
Ok, we have +1 here, if no one disapproval here I will merge this patch to master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie edited a comment on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie edited a comment on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654977519
Ok, we have +1 here, if no one disapproval here in a few day I will merge this patch to master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903: [WIP] Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#discussion_r459507907
##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -120,14 +120,15 @@ def _get_aws_credentials(self):
intended to be used by external import and export statements
"""
- if self.snowflake_conn_id: # pylint: disable=no-member
- connection_object = self.get_connection(self.snowflake_conn_id) # pylint: disable=no-member
- if 'aws_secret_access_key' in connection_object.extra_dejson:
- aws_access_key_id = connection_object.extra_dejson.get(
- 'aws_access_key_id')
- aws_secret_access_key = connection_object.extra_dejson.get(
- 'aws_secret_access_key')
- return aws_access_key_id, aws_secret_access_key
+ connection_object = self.connection
+ if 'aws_secret_access_key' in connection_object.extra_dejson:
+ aws_access_key_id = connection_object.extra_dejson.get(
+ 'aws_access_key_id')
+ aws_secret_access_key = connection_object.extra_dejson.get(
+ 'aws_secret_access_key')
+ return aws_access_key_id, aws_secret_access_key
+ else:
+ return None, None
Review comment:
I have to change it to pass CI, I have no idea why previous code could pass the CI, mypy will raise error cause `aws_access_key_id, aws_secret_access_key` maybe not define
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654977519
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654978821
Please rebase once again - just to be sure. :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-656718659
I think we should keep `conn_name_attr` but change property name which confused users, like `connection` as you said above. Cause we need dbapi_hook as parent class to handle some RDBMS like hook.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on a change in pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on a change in pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#discussion_r415272770
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,29 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get the name of the database connection attribute which identifies the connection.
+ """
+ return getattr(self, self.conn_name_attr)
+
+ @property
+ def connection(self) -> Connection:
+ """
+ Get the database connection object.
+
+ :return: Connection
+ """
+ return super().get_connection(self.conn_name)
Review comment:
Haha, I think so, and I open this to hear other committer idea
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-775415431
@zhongjiajie Are you going to carry on with this PR? Where did we get to with it when we left off?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#discussion_r415253247
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,29 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get the name of the database connection attribute which identifies the connection.
+ """
+ return getattr(self, self.conn_name_attr)
+
+ @property
+ def connection(self) -> Connection:
+ """
+ Get the database connection object.
+
+ :return: Connection
+ """
+ return super().get_connection(self.conn_name)
Review comment:
```suggestion
return self.get_connection(self.conn_name)
```
should also work, doesn't it?
Actually I think this option (the second one) is better. 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stale[bot] closed pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #7903:
URL: https://github.com/apache/airflow/pull/7903
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stale[bot] commented on pull request #7903: [WIP] Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-695603746
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#discussion_r415251335
##########
File path: airflow/hooks/dbapi_hook.py
##########
@@ -62,15 +63,29 @@ def __init__(self, *args, **kwargs):
raise AirflowException("conn_name_attr is not defined")
elif len(args) == 1:
setattr(self, self.conn_name_attr, args[0])
- elif self.conn_name_attr not in kwargs:
- setattr(self, self.conn_name_attr, self.default_conn_name)
else:
- setattr(self, self.conn_name_attr, kwargs[self.conn_name_attr])
+ setattr(self, self.conn_name_attr, kwargs.get(self.conn_name_attr, self.default_conn_name))
+
+ @property
+ def conn_name(self) -> str:
+ """
+ Get the name of the database connection attribute which identifies the connection.
+ """
+ return getattr(self, self.conn_name_attr)
+
+ @property
+ def connection(self) -> Connection:
+ """
+ Get the database connection object.
+
+ :return: Connection
+ """
+ return super().get_connection(self.conn_name)
Review comment:
I would prefer going for the first option and don't add another `connection` property. And use `self.get_connection(self.conn_name)` in hooks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] stale[bot] closed pull request #7903: [WIP] Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #7903:
URL: https://github.com/apache/airflow/pull/7903
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle edited a comment on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
feluelle edited a comment on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654978821
Please rebase once again - just to be sure. :)
It is very long ago since you last pushed a commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654176235
@feluelle @potiuk @kaxil @kaxil @turbaszek I think I need one approving review here. I checked and find out there no `DbApiHook` subclass add during this time, which mean this patch cover/change all `DbApiHook` subclass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhongjiajie commented on pull request #7903: Add property conn_name and connection to dbapi_hook
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on pull request #7903:
URL: https://github.com/apache/airflow/pull/7903#issuecomment-654127593
Reopen this stale PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org