Skip to content

Refactor SlackWebhookHook in order to use slack_sdk instead of HttpHook methods #26452

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.

Sign up for GitHub

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

Jump to bottom
Merged
potiuk merged 5 commits into from
Sep 22, 2022
Merged

Refactor SlackWebhookHook in order to use slack_sdk instead of HttpHook methods #26452

potiuk merged 5 commits into from
Sep 22, 2022

Conversation

@Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Sep 17, 2022 •
edited
Loading

Fundamental refactor of SlackWebhookHook with breaking changes.
I tried to minimise breaking changes, however main (and hope the only once) braking change SlackWebhookHook not anymore inherit from airflow.providers.http.hooks.http.HttpHook anymore and use slack_sdk.WebhookClient instead

Additional changes:

  1. Warn user that it is not safe to specify webhook token (url) directly in the Hook and suggest to switch to Airflow Connections.
  2. Deprecate specify webhook message attribute in __init__ method and mainly use hook attributes only for configure slack_sdk.WebhookClient. It is still possible to set as Hook arguments.
  3. Inform users (always) that it is not possible to change channel, username and icon by use Slack Incoming Webhook. Users might previously use for two reasons:
    a. Hook and Operators not cover this part
    b. Hook initially created for use with Legacy Incoming Webhooks based on Slack Integration
  4. Change SlackWebhookHook arguments in Operators
  5. Add documentation for Slack Incoming Webhook connection type

image

Fundamental also mean that main files related to SlackWebhookHook (airflow/providers/slack/hooks/slack_webhook.py,
tests/providers/slack/hooks/test_slack_webhook.py) re-created from scratch

@Taragolis Taragolis requested a review from mik-laj as a code owner September 17, 2022 13:23
@Taragolis Taragolis force-pushed the slack-webhook-refactoring branch from 675da95 to 7208126 Compare September 18, 2022 08:09
@Taragolis Taragolis mentioned this pull request Sep 18, 2022
Merged
@Taragolis Taragolis force-pushed the slack-webhook-refactoring branch from 7208126 to 20c81d7 Compare September 18, 2022 09:35
@Taragolis
Copy link
Contributor Author

Taragolis commented Sep 18, 2022

Even with this PR it is not possible to get rid of apache-airflow-providers-http dependency.
Mainly because SlackWebhookOperator inherit from SimpleHttpOperator

airflow/airflow/providers/slack/operators/slack_webhook.py

Lines 28 to 30 in 706a618

class SlackWebhookOperator(SimpleHttpOperator):
"""

But actually it not use any feature of SimpleHttpOperator just because it overwrite execute method where all internal stuff of SimpleHttpOperator happen.

Changes in SlackWebhookOperator could be done after this PR. In fact this PR contain a lot of changes so it would be difficult to track changes if it done in single PR.

airflow/airflow/providers/slack/operators/slack_webhook.py

Lines 94 to 110 in 706a618

def execute(self, context: Context) -> None:
"""Call the SlackWebhookHook to post the provided Slack message"""
self.hook = SlackWebhookHook(
self.http_conn_id,
self.webhook_token,
self.message,
self.attachments,
self.blocks,
self.channel,
self.username,
self.icon_emoji,
self.icon_url,
self.link_names,
self.proxy,
)
self.hook.execute()

@potiuk
Copy link
Member

potiuk commented Sep 19, 2022

Slack seems to be a beast - can you plese rebase :) ?

@potiuk
Copy link
Member

potiuk commented Sep 19, 2022

BTW. Thanks from trying to unentangle the mess :D

@Taragolis Taragolis force-pushed the slack-webhook-refactoring branch from 20c81d7 to 18fbda0 Compare September 19, 2022 13:28
Taragolis
Taragolis commented Sep 19, 2022
View reviewed changes
airflow/providers/slack/hooks/slack_webhook.py Outdated
Comment on lines +338 to +345
Copy link
Contributor Author

@Taragolis Taragolis Sep 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option create separate methods send_legacy and send_text_legacy which allow send to Legacy Incoming Webhook without any warnings, might be helpful if there is no possible for user change Legacy Incoming Webhook.

Copy link
Member

@potiuk potiuk Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there are good reason why it would not be possible, or just " I had no time to change it?". If that's the latter, then this is no problem to have warnings. Warnings are there to be annoying enough to gently push the user to get rid of the legacy code and make investment in doing so. The only reason for allowing to silence warnings easily is when there migtht be really good reasons and use cases that cannot be handled with the "new way" of doing things.

Copy link
Contributor Author

@Taragolis Taragolis Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one case when user do not have access to create new applications in slack workspace.
One day Slack Webhook based on Slack integrations will be removed anyway so in user perspective better started to migrated from legacy service to 2 successors: Slack API and Slack Incoming Webhook based on API/App

This might be the later feature if someone requested it and better warn user right now always.

@potiuk
Copy link
Member

potiuk commented Sep 22, 2022

@Taragolis - are you working on making some of the changes discussed ? I am gearing up to prepare a new provider's wave, so it would be great to merge this one before.

@Taragolis
Copy link
Contributor Author

Taragolis commented Sep 22, 2022

@Taragolis - are you working on making some of the changes discussed ? I am gearing up to prepare a new provider's wave, so it would be great to merge this one before.

Yep. Plan to finish changes by today

@Taragolis Taragolis force-pushed the slack-webhook-refactoring branch from 9451c8b to bbe3be3 Compare September 22, 2022 12:35
@Taragolis
Copy link
Contributor Author

Taragolis commented Sep 22, 2022

Finally all checks passed. Today it takes a bit longer rather than usual.

potiuk
potiuk approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor!

@potiuk potiuk merged commit 95a5fc7 into apache:main Sep 22, 2022
@Taragolis Taragolis mentioned this pull request Sep 24, 2022
Merged
This was referenced Sep 28, 2022
Closed
Closed
@Taragolis Taragolis deleted the slack-webhook-refactoring branch January 14, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@vincbeck vincbeck vincbeck approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

No one assigned

Labels

area:providers kind:documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Taragolis @potiuk @vincbeck