airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Refactor SlackWebhookHook in order to use `slack_sdk` instead of HttpHook methods

Open Taragolis opened this issue 3 years ago • 5 comments

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 avatar Sep 17 '22 13:09 Taragolis

Even with this PR it is not possible to get rid of apache-airflow-providers-http dependency. Mainly because SlackWebhookOperator inherit from SimpleHttpOperator https://github.com/apache/airflow/blob/706a618014a6f94d5ead0476f26f79d9714bf93d/airflow/providers/slack/operators/slack_webhook.py#L28-L30

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. https://github.com/apache/airflow/blob/706a618014a6f94d5ead0476f26f79d9714bf93d/airflow/providers/slack/operators/slack_webhook.py#L94-L110

Taragolis avatar Sep 18 '22 20:09 Taragolis

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

potiuk avatar Sep 19 '22 13:09 potiuk

BTW. Thanks from trying to unentangle the mess :D

potiuk avatar Sep 19 '22 13:09 potiuk

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

potiuk avatar Sep 22 '22 09:09 potiuk

@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 avatar Sep 22 '22 09:09 Taragolis

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

Taragolis avatar Sep 22 '22 16:09 Taragolis