airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Implements SqlToSlackApiFileOperator

Open Taragolis opened this issue 3 years ago • 10 comments
trafficstars

closes: #9145 follow-up: #24660

Rather than extend existed SqlToSlackOperator I've decided to create new transfer operator SqlToSlackApiFileOperator.

cc: @eladkal

Taragolis avatar Sep 13 '22 19:09 Taragolis

Rather than extend existed SqlToSlackOperator I've decided to create new transfer operator SqlToSlackApiFileOperator.

Can you explain why?

eladkal avatar Sep 13 '22 20:09 eladkal

Can you explain why?

  1. SqlToSlackOperator intends to use Slack Webhook ether based on Slack App or Slack Legacy Integrations but both won't support send file, which only allowed by Slack API. Slack Incoming Webhooks and Slack API are not not interchangeable and do not have common interface.
  2. Expected different parameters, so if extend current operator than required 4-5 additional parameters which won't use in case of use send as a regular message.
  3. I thought also easier maintain something simple rather than with complicated logic and a lot of additional conditions

Taragolis avatar Sep 13 '22 21:09 Taragolis

I thought also easier maintain something simple rather than with complicated logic and a lot of additional conditions

Im not sure this makes it simpler? If I'm using current operator to send message and now I want the exact same results just in file it require to switch to another operator. Its not very intuative and a potential source for confusion.

But I'd like to hear also @alexkruc point of view as the author of the operator.

eladkal avatar Sep 13 '22 21:09 eladkal

If I'm using current operator to send message and now I want the exact same results just in file it require to switch to another operator

As well as create new connection with different type.

If create all in once operator than:

  • slack_filename, slack_initial_comment, slack_initial_comment, slack_title, df_kwargs - parameters won't use if user want to send in slack as a message
  • slack_channels - multiple channels (0 to many) supported only by Slack API, Slack Incoming Webhook based on Slack APP do not supported change channels at all, Slack Webhooks based on Legacy Integration could assign only one channel (slack_channel)
  • slack_webhook_token - token are different for Slack Incoming Webhook and Slack API
  • slack_message, results_df_name - won't use if user want to send as a File

Taragolis avatar Sep 13 '22 21:09 Taragolis

Hi all :) Please note that I'm not a commiter, so my replay might lack details - I think that this discussion is showing us a bit of an underlying problem with the way Airflow works with Slack. There are 2 hooks that are entirely different with the parameter they accept and their behavior.. (link) I think that this is causing a lot of confusion with the development of new operators/flows, as there is no strict guideline on when to use which Slack hook or whether one of them is deprecated. So I think that what should be done in the long run is to consolidate the Slack hook in a way that allows a single hook to be used with the webhook integration or with the Slack API.

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?

alexkruc avatar Sep 15 '22 07:09 alexkruc

Right now there are 3 ways to send formatted message to slack

  1. Use Slack API and chat.postMessage. Supported by Python Slack SDK
  2. Use Slack Incoming Webhook based on Slack App. Supported by Python Slack SDK
  3. Use Incoming Webhook based on Slack Integration. Slack integration itself is legacy tech for a while.

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 xoxb-1234567890123-09876543210987-AbCdEfGhIjKlMnOpQrStUvWx) and send requests to https://www.slack.com/api/ (by default)

Slack Incoming Webhook and Incoming Webhook based on Slack Integration use URL (the same format: https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX) for auth.

The main differences between Slack Incoming Webhook and Legacy version that for legacy integration you could change username, channel, icon_url and icon_link. If you tried to send this parameters to Slack Incoming Webhook it just ignored and user wouldn't notified.

Taragolis avatar Sep 15 '22 08:09 Taragolis

Use Incoming Webhook based on Slack Integration. Slack integration itself is legacy tech for a while.

Does this mean that the SlackWebhookHook is deprecated?

eladkal avatar Sep 18 '22 08:09 eladkal

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:

  • 2019-2020: in one of the project we used SlackWebhookHook for send callbacks, and it work fine we could change channel and displayed username. We did not create webhook url by our own, someone just gave it to us
  • 2021: another project, we were creating webhook url by our own.... and nothing actually worked as expected. So we just switch to Slack API
  • end of 2021-early 2022: one guy told me "Did you want a magic? This is two Slack Webhook URL, looks similar use in the same Slack Workspace but one of them allow change channel and username and another not"

Taragolis avatar Sep 18 '22 09:09 Taragolis

What a mess :) . @eladkal - > I am ffne with this approach

potiuk avatar Sep 19 '22 12:09 potiuk

This is a mess but because this is a feature and not a bug fix I prefer maybe that we first get https://github.com/apache/airflow/pull/26452 revewed and merged and sort out how to use slack in general and then revist this one? However I'm OK also with moving a head with this and possible make changes/fixes later if we feel this is important to get it for next release

eladkal avatar Sep 19 '22 12:09 eladkal

@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.

potiuk avatar Nov 11 '22 18:11 potiuk