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/08/05 19:38:06 UTC
[GitHub] [airflow] Hasan-J opened a new pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Hasan-J opened a new pull request #10192:
URL: https://github.com/apache/airflow/pull/10192
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
closes: #10135
Deprecates the use of `BaseHook.get_connections`
- [ ] Unit tests coverage for changes
- [ ] Relevant documentation is updated including usage instructions.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-669442900
Still need to go over the tests and docs
And wasn't sure if we should modify the hooks in a separate issue or not!
- `airflow.airflow.providers.apache.hdfs.hooks.hdfs.py`
- `airflow.providers.apache.hdfs.hooks.webhdfs.py`
- `airflow.providers.apache.hive.hooks.hive.py`
----------------------------------------------------------------
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 a change in pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#discussion_r532555442
##########
File path: airflow/hooks/base_hook.py
##########
@@ -42,7 +43,12 @@ def get_connections(cls, conn_id: str) -> List[Connection]:
:param conn_id: connection id
:return: array of connections
"""
- return Connection.get_connections_from_secrets(conn_id)
+ warnings.warn(
+ "`BaseHook.get_connections` method will be deprecated in the future."
+ "Please use `BaseHook.get_connection` instead.",
+ PendingDeprecationWarning,
Review comment:
```suggestion
PendingDeprecationWarning,
stacklevel=2,
```
##########
File path: airflow/secrets/metastore.py
##########
@@ -31,12 +31,25 @@ class MetastoreBackend(BaseSecretsBackend):
# pylint: disable=missing-docstring
@provide_session
- def get_connections(self, conn_id, session=None) -> List['Connection']:
+ def get_connection(self, conn_id, session=None) -> Optional['Connection']:
from airflow.models.connection import Connection
- conn_list = session.query(Connection).filter(Connection.conn_id == conn_id).all()
+ conn = session.query(Connection).filter(Connection.conn_id == conn_id).first()
session.expunge_all()
- return conn_list
+ return conn
+
+ # pylint: disable=missing-docstring
+ @provide_session
+ def get_connections(self, conn_id, session=None) -> List['Connection']:
+ warnings.warn(
+ "This method is deprecated. Please use "
+ "`airflow.secrets.metastore.MetastoreBackend.get_connection`.",
+ PendingDeprecationWarning,
Review comment:
```suggestion
PendingDeprecationWarning,
stacklevel=3,
```
(+1 here because of the decorator.)
##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -310,16 +313,27 @@ def _local_variables(self) -> Dict[str, str]:
return secrets
@property
- def _local_connections(self) -> Dict[str, List[Any]]:
+ def _local_connections(self) -> Dict[str, 'Connection']:
if not self.connections_file:
self.log.debug("The file for connection is not specified. Skipping")
# The user may not specify any file.
return {}
return load_connections_dict(self.connections_file)
- def get_connections(self, conn_id: str) -> List[Any]:
+ def get_connection(self, conn_id: str) -> Optional['Connection']:
if conn_id in self._local_connections:
- return [self._local_connections[conn_id]]
+ return self._local_connections[conn_id]
+ return None
+
+ def get_connections(self, conn_id: str) -> List[Any]:
+ warnings.warn(
+ "This method is deprecated. Please use "
+ "`airflow.secrets.local_filesystem.LocalFilesystemBackend.get_connection`.",
+ PendingDeprecationWarning,
Review comment:
```suggestion
PendingDeprecationWarning,
stacklevel=2,
```
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -62,9 +63,26 @@ def get_connections(self, conn_id: str) -> List['Connection']:
conn_uri = self.get_conn_uri(conn_id=conn_id)
if not conn_uri:
- return []
+ return None
conn = Connection(conn_id=conn_id, uri=conn_uri)
- return [conn]
+ return conn
+
+ def get_connections(self, conn_id: str) -> List['Connection']:
+ """
+ Return connection object with a given ``conn_id``.
+
+ :param conn_id: connection id
+ :type conn_id: str
+ """
+ warnings.warn(
+ "This method is deprecated. Please use "
+ "`airflow.secrets.base_secrets.BaseSecretsBackend.get_connection`.",
+ PendingDeprecationWarning,
Review comment:
```suggestion
PendingDeprecationWarning,
stacklevel=2,
```
----------------------------------------------------------------
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] Hasan-J commented on a change in pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on a change in pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#discussion_r465965967
##########
File path: airflow/hooks/base_hook.py
##########
@@ -44,17 +44,21 @@ def get_connections(cls, conn_id: str) -> List[Connection]:
:param conn_id: connection id
:return: array of connections
"""
+ warnings.warn(
+ "This method is deprecated. Please use `airflow.hooks.base_hook.BaseHook.get_connection`.",
+ DeprecationWarning
+ )
return secrets.get_connections(conn_id)
Review comment:
So, this is the only change we needed!
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#discussion_r465964827
##########
File path: airflow/hooks/base_hook.py
##########
@@ -44,17 +44,21 @@ def get_connections(cls, conn_id: str) -> List[Connection]:
:param conn_id: connection id
:return: array of connections
"""
+ warnings.warn(
+ "This method is deprecated. Please use `airflow.hooks.base_hook.BaseHook.get_connection`.",
+ DeprecationWarning
+ )
return secrets.get_connections(conn_id)
Review comment:
```suggestion
return [secrets.get_connection(conn_id)]
```
----------------------------------------------------------------
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 #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735765842
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 master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #10192:
URL: https://github.com/apache/airflow/pull/10192
----------------------------------------------------------------
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 commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735644144
Yep. We've changed the message slightly yesterday (it was a bit misleading) - suggesting the way that you've just done (force-pushing the change).
----------------------------------------------------------------
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] mik-laj commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-671097020
I added `airflow connection get` command: https://github.com/apache/airflow/pull/10214
----------------------------------------------------------------
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] mik-laj commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-686823492
> Do you think we can totally remove the use of get_connections before Airflow2.0?
We should keep backward-compatibility with runtime warning. In Airlfow 2.1, we can remove all deprecated methods.
----------------------------------------------------------------
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] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735722112
Ready for merge.
----------------------------------------------------------------
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] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735625380
@ashb Does this need to be re-run again !
----------------------------------------------------------------
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] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735928563
Oh, it's merged. But it's taking so long for the ci to finish XD
----------------------------------------------------------------
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 #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-733966465
[The Workflow run](https://github.com/apache/airflow/actions/runs/384078973) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-733966203
[The Workflow run](https://github.com/apache/airflow/actions/runs/384077929) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] Hasan-J commented on a change in pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on a change in pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#discussion_r532260640
##########
File path: setup.py
##########
@@ -221,7 +221,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'sphinxcontrib-httpdomain>=1.7.0',
"sphinxcontrib-redoc>=1.6.0",
"sphinxcontrib-spelling==5.2.1",
- f"sphinx-airflow-theme @ {_SPHINX_AIRFLOW_THEME_URL}",
+ # f"sphinx-airflow-theme @ {_SPHINX_AIRFLOW_THEME_URL}",
Review comment:
Yes, mb.
----------------------------------------------------------------
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 a change in pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#discussion_r532120916
##########
File path: setup.py
##########
@@ -221,7 +221,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'sphinxcontrib-httpdomain>=1.7.0',
"sphinxcontrib-redoc>=1.6.0",
"sphinxcontrib-spelling==5.2.1",
- f"sphinx-airflow-theme @ {_SPHINX_AIRFLOW_THEME_URL}",
+ # f"sphinx-airflow-theme @ {_SPHINX_AIRFLOW_THEME_URL}",
Review comment:
```suggestion
f"sphinx-airflow-theme @ {_SPHINX_AIRFLOW_THEME_URL}",
```
Guessing this was debugging?
----------------------------------------------------------------
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] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-733520691
There are some changes that need to be addressed. I'll pick this up in about 8 hours from now.
If it's too urgent, someone could take it, yep.
----------------------------------------------------------------
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 #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-732801488
I'm marking this as "critical" priority and putting it for 2.0.0beta4 release -- if we don't get it in before then we'll need to keep this until Airflow 3.0!
@Hasan-J Are you able to carry on working on this PR (possibly just rebase it) -- if we don't hear from you in a day or two one of us will pick this up.
----------------------------------------------------------------
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 #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735307055
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-669461720
In the case of `airflow connections list --include-sectes`, I think we should now add a new command `airflow connection get` which will only let you download one entry.
----------------------------------------------------------------
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] Hasan-J commented on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J commented on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-674414271
Do you think we can totally remove the use of `get_connections` before Airflow2.0?
----------------------------------------------------------------
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] Hasan-J edited a comment on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J edited a comment on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-669442900
Still need to go over the tests and docs
And wasn't sure if we should modify the hooks in a separate issue or not!
- `airflow.airflow.providers.apache.hdfs.hooks.hdfs.py`
- `airflow.providers.apache.hdfs.hooks.webhdfs.py`
- `airflow.providers.apache.hive.hooks.hive.py`
Plus the cli command for listing connections:
`airflow.cli.commands.connection_command.py`
----------------------------------------------------------------
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] Hasan-J removed a comment on pull request #10192: Deprecate BaseHook.get_connections method (#10135)
Posted by GitBox <gi...@apache.org>.
Hasan-J removed a comment on pull request #10192:
URL: https://github.com/apache/airflow/pull/10192#issuecomment-735928563
Oh, it's merged. But it's taking so long for the ci to finish XD
----------------------------------------------------------------
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