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/04/21 12:00:12 UTC
[GitHub] [airflow] serkef opened a new pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
serkef opened a new pull request #5519:
URL: https://github.com/apache/airflow/pull/5519
Make sure you have checked _all_ steps below.
### Jira
- [x] My PR addresses the following [Airflow Jira](https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "\[AIRFLOW-XXX\] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-4543
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
* Support (exclusively) official slack library v2.
* Retain existing functionality and interface
* Slightly change structure (eg remove some properties)
* Remove a redundant check that was made in both Hook and Operator
### Tests
- [ x ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
* tests.operators.test_slack_operator
* tests.hooks.test_slack_hook.SlackHookTestCase
### Commits
- [x] My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [x] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the [Updating.md](https://github.com/apache/airflow/blob/master/UPDATING.md) so we can assign it to a appropriate release
### Code Quality
- [x] Passes `flake8`
----------------------------------------------------------------
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] serkef commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418941185
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack.
+ Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
+ self.client = WebClient(token, **client_args)
Review comment:
Done.
I would assume it's more efficient to create the connection on init to possibly reuse. Do we have a common pattern in airflow hooks to create the connections during the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418718905
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack.
+ Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
+ self.client = WebClient(token, **client_args)
Review comment:
Let's not create a client in `__init__`
----------------------------------------------------------------
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] serkef commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r412384446
##########
File path: setup.py
##########
@@ -256,7 +256,7 @@ def write_version(filename: str = os.path.join(*["airflow", "git_version"])):
segment = ['analytics-python>=1.2.9']
sendgrid = ['sendgrid>=5.2.0,<6']
sentry = ['sentry-sdk>=0.8.0', "blinker>=1.1"]
-slack = ['slackclient>=1.0.0,<2.0.0']
+slack = ['slackclient>=2.0.0,<3.0.0']
Review comment:
v.2.5.0 is already out. Shall I restrict minimum version?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418983365
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack.
+ Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
+ self.client = WebClient(token, **client_args)
Review comment:
Happy to merge it now as it is.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r421892844
##########
File path: tests/providers/slack/hooks/test_slack.py
##########
@@ -19,82 +19,154 @@
import unittest
import mock
+from slack.errors import SlackApiError
from airflow.exceptions import AirflowException
from airflow.providers.slack.hooks.slack import SlackHook
class TestSlackHook(unittest.TestCase):
- def test_init_with_token_only(self):
+
+ def test___get_token_with_token_only(self):
+ """ tests `__get_token` method when only token is provided """
+ # Given
test_token = 'test_token'
- slack_hook = SlackHook(token=test_token, slack_conn_id=None)
+ test_conn_id = None
+
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
- self.assertEqual(slack_hook.token, test_token)
+ # Run
+ hook = DummySlackHook()
+
+ # Assert
+ output = hook._SlackHook__get_token(test_token, test_conn_id) # pylint: disable=E1101
+ expected = test_token
+ self.assertEqual(output, expected)
@mock.patch('airflow.providers.slack.hooks.slack.SlackHook.get_connection')
- def test_init_with_valid_slack_conn_id_only(self, get_connection_mock):
+ def test___get_token_with_valid_slack_conn_id_only(self, get_connection_mock):
+ """ tests `__get_token` method when only connection is provided """
+ # Given
+ test_token = None
+ test_conn_id = 'x'
test_password = 'test_password'
+
+ # Mock
get_connection_mock.return_value = mock.Mock(password=test_password)
- test_slack_conn_id = 'test_slack_conn_id'
- slack_hook = SlackHook(token=None, slack_conn_id=test_slack_conn_id)
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
Review comment:
Pushed a commit to PR: https://github.com/apache/airflow/pull/5519/commits/d81d6e947ba364b0dd9e730507e51ff2a0d3d3e2
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418982750
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack.
+ Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
+ self.client = WebClient(token, **client_args)
Review comment:
I had a brain-fade moment actually 🤦 . I thought the client creation was inside Operator's init method, so to avoid DB calls while DAG Parsing I suggested to move it outside `__init__`. It won't matter with 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] serkef commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r420312499
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack.
+ Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
+ self.client = WebClient(token, **client_args)
Review comment:
I moved this back to init, to give the possibility to operators to use the client directly.
----------------------------------------------------------------
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] kaxil edited a comment on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-622995610
The following diff should also check and fix the issue:
```diff
diff --git a/airflow/providers/slack/hooks/slack.py b/airflow/providers/slack/hooks/slack.py
index 3a0cd693c..d4d558e7a 100644
--- a/airflow/providers/slack/hooks/slack.py
+++ b/airflow/providers/slack/hooks/slack.py
@@ -80,7 +80,7 @@ class SlackHook(BaseHook):
:param api_params: parameters of the API
"""
slack_client = WebClient(self.token, **self.client_args)
- return_code = slack_client.api_call(method, **api_params)
+ return_code = slack_client.api_call(method, json=api_params)
try:
return_code.validate()
diff --git a/tests/providers/slack/hooks/test_slack.py b/tests/providers/slack/hooks/test_slack.py
index 1594d8b1d..cfb5c79dc 100644
--- a/tests/providers/slack/hooks/test_slack.py
+++ b/tests/providers/slack/hooks/test_slack.py
@@ -160,3 +160,13 @@ class TestSlackHook(unittest.TestCase):
except AirflowException as exc:
self.assertIn("foo", str(exc))
self.assertIn("bar", str(exc))
+
+ @mock.patch('airflow.providers.slack.hooks.slack.WebClient.api_call', autospec=True)
+ @mock.patch('airflow.providers.slack.hooks.slack.WebClient')
+ def test_api_call(self, mock_slack_client, mock_slack_api_call):
+ slack_hook = SlackHook(token='test_token')
+ test_api_params = {'channel': 'test_channel'}
+
+ slack_hook.call("chat.postMessage", test_api_params)
+ mock_slack_api_call.assert_called_once_with(
+ mock_slack_client, "chat.postMessage", json=test_api_params)
```
----------------------------------------------------------------
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] serkef commented on issue #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on issue #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-617329045
@kaxil can you help a bit? Why are the CI actions not running? Also I see conflicts on the files that I resolved when I rebased. I'm probably missing something...
----------------------------------------------------------------
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] kaxil commented on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-624234996
Docs build is failing: https://github.com/apache/airflow/pull/5519/checks?check_run_id=647041208
----------------------------------------------------------------
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] kaxil commented on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-622995610
The following diff should also check and fix the issue:
```diff
diff --git a/airflow/providers/slack/hooks/slack.py b/airflow/providers/slack/hooks/slack.py
index 3a0cd693c..d4d558e7a 100644
--- a/airflow/providers/slack/hooks/slack.py
+++ b/airflow/providers/slack/hooks/slack.py
@@ -80,7 +80,7 @@ class SlackHook(BaseHook):
:param api_params: parameters of the API
"""
slack_client = WebClient(self.token, **self.client_args)
- return_code = slack_client.api_call(method, **api_params)
+ return_code = slack_client.api_call(method, json=api_params)
try:
return_code.validate()
diff --git a/tests/providers/slack/hooks/test_slack.py b/tests/providers/slack/hooks/test_slack.py
index 1594d8b1d..cfb5c79dc 100644
--- a/tests/providers/slack/hooks/test_slack.py
+++ b/tests/providers/slack/hooks/test_slack.py
@@ -160,3 +160,13 @@ class TestSlackHook(unittest.TestCase):
except AirflowException as exc:
self.assertIn("foo", str(exc))
self.assertIn("bar", str(exc))
+
+ @mock.patch('airflow.providers.slack.hooks.slack.WebClient.api_call', autospec=True)
+ @mock.patch('airflow.providers.slack.hooks.slack.WebClient')
+ def test_api_call(self, mock_slack_client, mock_slack_api_call):
+ slack_hook = SlackHook(token='test_token')
+ test_api_params = {'channel': 'd'}
+
+ slack_hook.call("chat.postMessage", test_api_params)
+ mock_slack_api_call.assert_called_once_with(
+ mock_slack_client, "chat.postMessage", json=test_api_params)
```
----------------------------------------------------------------
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] kaxil commented on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-622989835
Check this out: https://github.com/slackapi/python-slackclient/wiki/Migrating-to-2.x
**Before 2.0**:
```python
from slackclient import SlackClient
client = SlackClient(os.environ["SLACK_API_TOKEN"])
client.api_call('chat.postMessage',
timeout=30,
channel='C0123456',
text="Hi!")
```
**After 2.0**:
```python
import slack
client = slack.WebClient(os.environ["SLACK_API_TOKEN"], timeout=30)
client.api_call('chat.postMessage', json={
'channel': 'C0123456',
'text': 'Hi!'})
# Note: That while the above is allowed, the more efficient way to call that API is like this:
client.chat_postMessage(
channel='C0123456',
text='Hi!')
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r421153513
##########
File path: tests/providers/slack/hooks/test_slack.py
##########
@@ -19,82 +19,154 @@
import unittest
import mock
+from slack.errors import SlackApiError
from airflow.exceptions import AirflowException
from airflow.providers.slack.hooks.slack import SlackHook
class TestSlackHook(unittest.TestCase):
- def test_init_with_token_only(self):
+
+ def test___get_token_with_token_only(self):
+ """ tests `__get_token` method when only token is provided """
+ # Given
test_token = 'test_token'
- slack_hook = SlackHook(token=test_token, slack_conn_id=None)
+ test_conn_id = None
+
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
- self.assertEqual(slack_hook.token, test_token)
+ # Run
+ hook = DummySlackHook()
+
+ # Assert
+ output = hook._SlackHook__get_token(test_token, test_conn_id) # pylint: disable=E1101
+ expected = test_token
+ self.assertEqual(output, expected)
@mock.patch('airflow.providers.slack.hooks.slack.SlackHook.get_connection')
- def test_init_with_valid_slack_conn_id_only(self, get_connection_mock):
+ def test___get_token_with_valid_slack_conn_id_only(self, get_connection_mock):
+ """ tests `__get_token` method when only connection is provided """
+ # Given
+ test_token = None
+ test_conn_id = 'x'
test_password = 'test_password'
+
+ # Mock
get_connection_mock.return_value = mock.Mock(password=test_password)
- test_slack_conn_id = 'test_slack_conn_id'
- slack_hook = SlackHook(token=None, slack_conn_id=test_slack_conn_id)
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
Review comment:
I mean creating SlackHook is fine isn't it ? As the WebClient does not call any external service by itself until `WebClient.call` is run
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418718000
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
Review comment:
```suggestion
:type use_session: bool
```
----------------------------------------------------------------
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] kaxil edited a comment on issue #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-617128959
@serkef Would you like to reopen and work on this PR? Airflow Master requires Python 3.6+ so this would be a good fit.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418718102
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,29 +28,50 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type base_url: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ `https://www.slack.com/api/`
Review comment:
```suggestion
``https://www.slack.com/api/``
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r421892088
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,40 +28,88 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
+
+ Examples:
+
+ .. code-block:: python
+
+ # Create hook
+ slack_hook = SlackHook(token="xxx") # or slack_hook = SlackHook(slack_conn_id="slack")
+
+ # Call generic API with parameters (errors are handled by hook)
+ # For more details check https://api.slack.com/methods/chat.postMessage
+ slack_hook.call("chat.postMessage", json={"channel": "#random", "text": "Hello world!"})
+
+ # Call method from Slack SDK (you have to handle errors yourself)
+ # For more details check https://slack.dev/python-slackclient/basic_usage.html#sending-a-message
+ slack_hook.client.chat_postMessage(channel="#random", text="Hello world!")
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type use_session: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ ``https://www.slack.com/api/``
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack. Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
- self.token = self.__get_token(token, slack_conn_id)
+ token = self.__get_token(token, slack_conn_id)
Review comment:
```suggestion
self.token = self.__get_token(token, slack_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] kaxil commented on issue #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-617341114
> @kaxil can you help a bit? Why are the CI actions not running? Also I see conflicts on the files that I resolved when I rebased. I'm probably missing something...
That is strange. Can you rebase on latest Master please, looks like there are some conflicts. Hopefully once you do that, GA should run
----------------------------------------------------------------
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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r421892012
##########
File path: tests/providers/slack/hooks/test_slack.py
##########
@@ -19,82 +19,154 @@
import unittest
import mock
+from slack.errors import SlackApiError
from airflow.exceptions import AirflowException
from airflow.providers.slack.hooks.slack import SlackHook
class TestSlackHook(unittest.TestCase):
- def test_init_with_token_only(self):
+
+ def test___get_token_with_token_only(self):
+ """ tests `__get_token` method when only token is provided """
+ # Given
test_token = 'test_token'
- slack_hook = SlackHook(token=test_token, slack_conn_id=None)
+ test_conn_id = None
+
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
- self.assertEqual(slack_hook.token, test_token)
+ # Run
+ hook = DummySlackHook()
+
+ # Assert
+ output = hook._SlackHook__get_token(test_token, test_conn_id) # pylint: disable=E1101
+ expected = test_token
+ self.assertEqual(output, expected)
@mock.patch('airflow.providers.slack.hooks.slack.SlackHook.get_connection')
- def test_init_with_valid_slack_conn_id_only(self, get_connection_mock):
+ def test___get_token_with_valid_slack_conn_id_only(self, get_connection_mock):
+ """ tests `__get_token` method when only connection is provided """
+ # Given
+ test_token = None
+ test_conn_id = 'x'
test_password = 'test_password'
+
+ # Mock
get_connection_mock.return_value = mock.Mock(password=test_password)
- test_slack_conn_id = 'test_slack_conn_id'
- slack_hook = SlackHook(token=None, slack_conn_id=test_slack_conn_id)
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
Review comment:
There is no need of dummy subclasses, you can initialize the Slack directly, it is not calling slack API in the __init__ is 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] kaxil commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r418987364
##########
File path: airflow/providers/slack/hooks/slack.py
##########
@@ -27,40 +28,62 @@
# noinspection PyAbstractClass
class SlackHook(BaseHook):
"""
+ Creates a Slack connection, to be used for calls.
Takes both Slack API token directly and connection that has Slack API token.
-
If both supplied, Slack API token will be used.
+ Exposes also the rest of slack.WebClient args.
:param token: Slack API token
+ :type token: str
:param slack_conn_id: connection that has Slack API token in the password field
+ :type slack_conn_id: str
+ :param use_session: A boolean specifying if the client should take advantage of
+ connection pooling. Default is True.
+ :type use_session: bool
+ :param base_url: A string representing the Slack API base URL. Default is
+ ``https://www.slack.com/api/``
+ :type base_url: str
+ :param timeout: The maximum number of seconds the client will wait
+ to connect and receive a response from Slack. Default is 30 seconds.
+ :type timeout: int
"""
- def __init__(self, token: Optional[str] = None, slack_conn_id: Optional[str] = None) -> None:
+
+ def __init__(
+ self,
+ token: Optional[str] = None,
+ slack_conn_id: Optional[str] = None,
+ **client_args: Any,
+ ) -> None:
super().__init__()
self.token = self.__get_token(token, slack_conn_id)
+ self.client_args = client_args
def __get_token(self, token, slack_conn_id):
if token is not None:
return token
- elif slack_conn_id is not None:
+
+ if slack_conn_id is not None:
conn = self.get_connection(slack_conn_id)
if not getattr(conn, 'password', None):
raise AirflowException('Missing token(password) in Slack connection')
return conn.password
- else:
- raise AirflowException('Cannot get token: '
- 'No valid Slack token nor slack_conn_id supplied.')
+
+ raise AirflowException('Cannot get token: '
+ 'No valid Slack token nor slack_conn_id supplied.')
def call(self, method: str, api_params: dict) -> None:
"""
- Calls the Slack client.
+ Creates and calls the Slack client.
:param method: method
:param api_params: parameters of the API
"""
- slack_client = SlackClient(self.token)
+ slack_client = WebClient(self.token, **self.client_args)
return_code = slack_client.api_call(method, **api_params)
Review comment:
It worked for me after the following change:
```suggestion
return_code = slack_client.api_call(method, json=api_params)
```
----------------------------------------------------------------
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] serkef commented on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-624219960
@kaxil thanks for testing this. I made the required changes.
> Have you tested that the `SlackAPIPostOperator` still works with attachments?
Yes, I tested an example DAG locally with the follow task:
```
run_this = SlackAPIPostOperator(
task_id="post_hello",
dag=dag,
token="xxx",
text="Hello",
channel="#welcome",
attachments=[{"pretext": "pre-hello", "text": "text-world"}],
)
```
And result
----------------------------------------------------------------
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] serkef commented on a change in pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
serkef commented on a change in pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#discussion_r421559066
##########
File path: tests/providers/slack/hooks/test_slack.py
##########
@@ -19,82 +19,154 @@
import unittest
import mock
+from slack.errors import SlackApiError
from airflow.exceptions import AirflowException
from airflow.providers.slack.hooks.slack import SlackHook
class TestSlackHook(unittest.TestCase):
- def test_init_with_token_only(self):
+
+ def test___get_token_with_token_only(self):
+ """ tests `__get_token` method when only token is provided """
+ # Given
test_token = 'test_token'
- slack_hook = SlackHook(token=test_token, slack_conn_id=None)
+ test_conn_id = None
+
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
- self.assertEqual(slack_hook.token, test_token)
+ # Run
+ hook = DummySlackHook()
+
+ # Assert
+ output = hook._SlackHook__get_token(test_token, test_conn_id) # pylint: disable=E1101
+ expected = test_token
+ self.assertEqual(output, expected)
@mock.patch('airflow.providers.slack.hooks.slack.SlackHook.get_connection')
- def test_init_with_valid_slack_conn_id_only(self, get_connection_mock):
+ def test___get_token_with_valid_slack_conn_id_only(self, get_connection_mock):
+ """ tests `__get_token` method when only connection is provided """
+ # Given
+ test_token = None
+ test_conn_id = 'x'
test_password = 'test_password'
+
+ # Mock
get_connection_mock.return_value = mock.Mock(password=test_password)
- test_slack_conn_id = 'test_slack_conn_id'
- slack_hook = SlackHook(token=None, slack_conn_id=test_slack_conn_id)
+ # Creating a dummy subclass is the easiest way to avoid running init
+ # which is actually using the method we are testing
+ class DummySlackHook(SlackHook):
+ def __init__(self, *args, **kwargs): # pylint: disable=W0231
+ pass
Review comment:
I want to test the `get_token` which is called in the `__init__` so this is why I'm creating a dummy subclass with empty init. Is there a better way 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-622989485
Unfortunately, our tests didn't catch this too :(
Can you fix the error please
----------------------------------------------------------------
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] kaxil commented on issue #5519: [AIRFLOW-4543] Update slack operator to support slackclient v2
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #5519:
URL: https://github.com/apache/airflow/pull/5519#issuecomment-617128959
@serkef Would like to reopen and work on this PR? Airflow Master requires Python 3.6+ so this would be a good fit.
----------------------------------------------------------------
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