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 2022/09/13 19:02:29 UTC

[GitHub] [airflow] Taragolis opened a new pull request, #26374: Implements SqlToSlackApiFileOperator

Taragolis opened a new pull request, #26374:
URL: https://github.com/apache/airflow/pull/26374

   closes: #9145
   follow-up: #24660
   
   Rather than extend existed `SqlToSlackOperator` I've decided to create new transfer operator `SqlToSlackApiFileOperator`.
   
   cc: @eladkal 
       


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1250220289

   > Use [Incoming Webhook based on Slack Integration](https://taragolisworkspace.slack.com/apps/A0F7XDUAZ-incoming-webhooks?tab=more_info). Slack integration itself is [legacy tech](https://api.slack.com/legacy/custom-integrations) for a while.
   
   Does this mean that the SlackWebhookHook is deprecated?


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r1014676679


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   @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 ?



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r1015388601


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   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](https://api.slack.com/block-kit) 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



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] potiuk commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1250930190

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


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r978577850


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   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**:
   1. Slack API `chat.postMessage` method via [SlackHook.call](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack.py#L198-L213)  method
   2. Slack Incoming Webhook via [SlackHook.send_dict](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L317-L347), [SlackHook.send](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L349-L397) and [SlackHook.send_text](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L399-L415) methods
   3. Slack Webhook based on Legacy Integration via [SlackHook.send_dict](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L317-L347), [SlackHook.send](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L349-L397) and [SlackHook.send_text](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack_webhook.py#L399-L415) methods
   
   Full list for what could be use for [send message into Slack Channel](https://api.slack.com/messaging/sending#sending_methods)
   
   **Send File in Slack Channel (or Workspace) supported by Airflow**:
   1. Slack API `files.upload` method via [SlackHook.send_file](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack.py#L215-L265) or [SlackHook.call](https://github.com/apache/airflow/blob/55d11464c047d2e74f34cdde75d90b633a231df2/airflow/providers/slack/hooks/slack.py#L198-L213) methods
   
   ## Which parameters could be provided for different methods
   
   ### Slack API Method [chat.postMessage](https://api.slack.com/methods/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](https://api.slack.com/methods/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](https://api.slack.com/messaging/webhooks) (Mainstream)
   
   There is no information about end list of parameters, due to the code of [WebhookClient.send](https://github.com/slackapi/python-slack-sdk/blob/0782f566a0c26a534965d4a1127a3b402a4d726b/slack_sdk/webhook/client.py#L78-L122) 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](https://taragolisworkspace.slack.com/apps/A0F7XDUAZ-incoming-webhooks?tab=settings&next_id=0) (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 |



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1245952818

   > 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
   
   


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1247796063

   Right now there are 3 ways to send formatted message to slack
   1. Use [Slack API](https://api.slack.com/) and [chat.postMessage](https://api.slack.com/methods/chat.postMessage). Supported by [Python Slack SDK](https://slack.dev/python-slack-sdk/web/index.html)
   2. Use [Slack Incoming Webhook](https://api.slack.com/messaging/webhooks) based on Slack App. Supported by [Python Slack SDK](https://slack.dev/python-slack-sdk/webhook/index.html)
   3. Use [Incoming Webhook based on Slack Integration](https://taragolisworkspace.slack.com/apps/A0F7XDUAZ-incoming-webhooks?tab=more_info). Slack integration itself is [legacy tech](https://api.slack.com/legacy/custom-integrations) for a while.
   
   And only Slack API use for [other integration](https://api.slack.com/methods) with slack.
   
   **Authentification**
   
   Slack API use [access token](https://api.slack.com/authentication/token-types) 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.


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] potiuk commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r981181793


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   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 :)



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1245986706

   > 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
   


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] alexkruc commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
alexkruc commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1247711120

   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](https://github.com/apache/airflow/tree/main/airflow/providers/slack/hooks))
   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? 


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] potiuk commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1312059105

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


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r978100846


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   What are our options if we want to avoid creating a new operator for files?



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r981226123


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   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?



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1250228341

   > 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" 


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] Taragolis commented on a diff in pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #26374:
URL: https://github.com/apache/airflow/pull/26374#discussion_r981304880


##########
airflow/providers/slack/transfers/sql_to_slack.py:
##########
@@ -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):

Review Comment:
   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.



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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1245967553

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


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1245920592

   > Rather than extend existed SqlToSlackOperator I've decided to create new transfer operator SqlToSlackApiFileOperator.
   
   Can you explain why?
   


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] eladkal commented on pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #26374:
URL: https://github.com/apache/airflow/pull/26374#issuecomment-1250935746

   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


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [airflow] potiuk merged pull request #26374: Implements SqlToSlackApiFileOperator

Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26374:
URL: https://github.com/apache/airflow/pull/26374


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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org