-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Refactor Slack API Hook and add new Connection type #25852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e1632b4 to
f83d1ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This additional indent required to proper formatted block in documentation and PyCharm
Without additional space
https://airflow.apache.org/docs/apache-airflow-providers-slack/stable/_api/airflow/providers/slack/hooks/slack/index.html#airflow.providers.slack.hooks.slack.SlackHook
Might be possible avoid this "hack"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also might be better just remove this method.
It not used after changes and it is hardly possible that someone override it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create separate class for use in further steps
- Refactor
airflow.providers.slack.hooks.slack_webhook.SlackWebhookHookwhich mostly designed for use with legacy Slack Integrations - Create new hook and connection which will use
slack_sdk.webhook.WebhookClientand create integration with Slack Incoming Webhook based on Slack API/App
eb00fb8 to
ed8a940
Compare
ed8a940 to
7ae8172
Compare

Refactor Slack API hook
__init__methodtokenin Hook argument. IMHO, there is not a good idea provide directly token better to use Connection.Connection UI and Testing Connection