-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Implements SqlToSlackApiFileOperator #26374
Conversation
Can you explain why? |
|
Im not sure this makes it simpler? But I'd like to hear also @alexkruc point of view as the author of the operator. |
As well as create new connection with different type. If create all in once operator than:
|
Hi all :) My take on this specific matter is until we don't have a unified Slack hook, having 2 operators that both of them use different Slack hooks is valid because the hooks are different in their nature, and their configuration is different as well. If we want to solve this, the solution should be on the SlackHook layer IMO. @eladkal WDYT? |
Right now there are 3 ways to send formatted message to slack
And only Slack API use for other integration with slack. Authentification Slack API use access token based on the type of integration (e.g. for Bots Slack Incoming Webhook and Incoming Webhook based on Slack Integration use URL (the same format: The main differences between Slack Incoming Webhook and Legacy version that for legacy integration you could change |
Does this mean that the SlackWebhookHook is deprecated? |
Not at all. It might use something that actually has no affect anymore and it is depend on what user actually use. I've tried to do some significant changes in SlackWebhookHook (#26452) Personally I've also have an issue with slack in the past:
|
What a mess :) . @eladkal - > I am ffne with this approach |
This is a mess but because this is a feature and not a bug fix I prefer maybe that we first get #26452 revewed and merged and sort out how to use slack in general and then revist this one? |
438c468
to
5579228
Compare
@@ -165,3 +193,115 @@ def execute(self, context: Context) -> None: | |||
self._render_and_send_slack_message(context, df) | |||
|
|||
self.log.debug('Finished sending SQL data to Slack') | |||
|
|||
|
|||
class SqlToSlackApiFileOperator(BaseSqlToSlackOperator): |
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.
What are our options if we want to avoid creating a new operator for files?
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.
Let me just summarise what we have right not, what we use right now and what Slack supports
Send Message in Slack Channel supported by Airflow:
- Slack API
chat.postMessage
method via SlackHook.call method - Slack Incoming Webhook via SlackHook.send_dict, SlackHook.send and SlackHook.send_text methods
- Slack Webhook based on Legacy Integration via SlackHook.send_dict, SlackHook.send and SlackHook.send_text methods
Full list for what could be use for send message into Slack Channel
Send File in Slack Channel (or Workspace) supported by Airflow:
- Slack API
files.upload
method via SlackHook.send_file or SlackHook.call methods
Which parameters could be provided for different methods
Slack API Method chat.postMessage (Mainstream)
parameter | required | scope |
---|---|---|
token | Yes | headers |
channel | Yes | dict payload |
attachments | At least one of attachments blocks text | dict payload |
blocks | At least one of attachments blocks text | dict payload |
text | At least one of attachments blocks text | dict payload |
as_user | No | dict payload |
icon_emoji | No | dict payload |
icon_url | No | dict payload |
link_names | No | dict payload |
metadata | No | dict payload |
mrkdwn | No | dict payload |
boolean | No | dict payload |
parse | No | dict payload |
reply_broadcast | No | dict payload |
thread_ts | No | dict payload |
unfurl_links | No | dict payload |
unfurl_media | No | dict payload |
username | No | dict payload |
Slack API Method files.upload (Mainstream)
parameter | required | scope |
---|---|---|
token | Yes | headers |
channels | No | dict payload |
content | No (if file provided) | dict payload |
file | No (if content provided) | multipart/form-data |
filename | No | dict payload |
filetype | No | dict payload |
initial_comment | No | dict payload |
thread_ts | No | dict payload |
title | No | dict payload |
Slack Incoming Webhook (Mainstream)
There is no information about end list of parameters, due to the code of WebhookClient.send from slack_sdk
only this parameters allowed (but not for 100% sure)
parameter | required | scope |
---|---|---|
token | Yes | URL |
attachments | At least one of attachments blocks text | dict payload |
blocks | At least one of attachments blocks text | dict payload |
text | At least one of attachments blocks text | dict payload |
response_type | No | dict payload |
replace_original | No | dict payload |
delete_original | No | dict payload |
unfurl_links | No | dict payload |
unfurl_media | No | dict payload |
Slack Webhook based on Legacy Integration (Legacy)
Even less information than Slack Incoming Webhook. Due to investigation this parameters supported
parameter | required | scope |
---|---|---|
token | Yes | URL |
channel | No | dict payload |
attachments | At least one of attachments blocks text | dict payload |
blocks | At least one of attachments blocks text | dict payload |
text | At least one of attachments blocks text | dict payload |
icon_emoji | No | dict payload |
icon_url | No | dict payload |
username | No | dict payload |
unfurl_links | No | dict payload |
And this additional parameters might supported
parameter | required | scope |
---|---|---|
response_type | No | dict payload |
replace_original | No | dict payload |
delete_original | No | dict payload |
unfurl_media | No | dict payload |
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.
Wow! . Quick question then - I am preparing for Provider's release. SHould I merge this one (code looks cool but I guess I need TL;DR; if the current state in this PR is "Releasable" if we merge it). I guess so, but wanted to get the feeling of others involved in the disucssion :)
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 dont think we should add operator just to send file. I understand the challange but I think we should try to mitigate this in the operator itself.
If Slack offers 3 diffrent approches than maybe we should have base class and 3 operators one per approch? Then each operator will be able to levrage the full capabilities of what slack offers?
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 think we do not need to rush especially if we do not have agreement how to do it in a better way.
IMHO. For send SQL response as a message we could actually do by three different way without turn into the pain 🤣
most of the major parameters are presented in all 3 APIs requests.
For files situation are different we can use only Slack API and internally it do not have same parameters from different methods. Most close it text
from chat.postMessage
and initial_comment
from files.upload
even channel
and channels
working differently.
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.
@Taragolis Under the assumption that we do not want to add a new operator for this - what are our options?
I think we are having hard time here since the capabilities are not clear on the hook level.
If slack has 3 different APIs to send message maybe we should have 3 hooks ?
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.
Initially when I started refactor slack provider I've also want to create additional hook but after deeply investigate I've found that Legacy and based on slack API Incoming Webhook has almost the same interface.
Legacy supports additional features which is not available in new one: change icons, username and channels.
And it is impossible (just my personal findings) to determine witch Webhook URL use for Legacy Incoming Webhook and which one for new one - even HTTP responses almost the same. That is why I combine usage in airflow.providers.slack.hooks.slack_webhook.SlackWebhookHook
with warnings about parameters which supported by Legacy only
If we want to just send message as a text than we might create some of generic interface because all three methods support blocks and fallback messages.
With send as file (upload) it is quite difficult because it is different method of API with different parameters and it only supported by Slack API not Webhook. And in case of send as file we do not need overwrite render_template_fields
for add Jinja filter in runtime
5579228
to
725b9b3
Compare
725b9b3
to
c280a98
Compare
@Taragolis @eladkal -> do you think that one is ready for prime time ? We could stil merge it and make it part of the November provider's wave. |
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 think this is the best we can do at this point.
@Taragolis thank you for the through research!
(I am still not a fan of adding additional operator just to send files but this is not a reflaction on your work). Much appriciated!
Will merge when CI is green
closes: #9145
follow-up: #24660
Rather than extend existed
SqlToSlackOperator
I've decided to create new transfer operatorSqlToSlackApiFileOperator
.cc: @eladkal