-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Implements SqlToSlackApiFileOperator #26374
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
chat.postMessagemethod via SlackHook.call methodFull list for what could be use for send message into Slack Channel
Send File in Slack Channel (or Workspace) supported by Airflow:
files.uploadmethod via SlackHook.send_file or SlackHook.call methodsWhich parameters could be provided for different methods
Slack API Method chat.postMessage (Mainstream)
Slack API Method files.upload (Mainstream)
Slack Incoming Webhook (Mainstream)
There is no information about end list of parameters, due to the code of WebhookClient.send from
slack_sdkonly this parameters allowed (but not for 100% sure)Slack Webhook based on Legacy Integration (Legacy)
Even less information than Slack Incoming Webhook. Due to investigation this parameters supported
And this additional parameters might supported
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
textfromchat.postMessageandinitial_commentfromfiles.uploadevenchannelandchannelsworking 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.SlackWebhookHookwith warnings about parameters which supported by Legacy onlyIf 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_fieldsfor add Jinja filter in runtime