Adding generic SqlToSlackOperator #24663

alexkruc · 2022-06-26T07:17:53Z

This PR is adding a generic SqlToSlackOperator, meaning everything that uses DbApiHook and implements get_pandas_df will now be able to send the results to Slack.
As part of this PR, I've also deprecated the current PrestoToSlackOperator and SnowflakeToSlackOperator - they now inherit from SqlToSlackOperator.

closes: #24243
reference: existing PrestoToSlackOperator and SnowflakeToSlackOperator, and the generic SqlToS3Operator.

tests/system/providers/slack/example_sql_to_slack.py

tests/providers/presto/transfers/test_presto_to_slack.py

tests/providers/snowflake/transfers/test_snowflake_to_slack.py

Co-authored-by: eladkal <[email protected]>

potiuk · 2022-06-26T10:18:17Z

Rebased to fix selective check problem from #24665 after it's merged.

eladkal · 2022-06-26T11:05:56Z

@alexkruc please check test failures

…ow into add_sql_to_slack_operator

airflow/providers/presto/transfers/presto_to_slack.py

docs/apache-airflow-providers-slack/operators/sql_to_slack.rst

Taragolis · 2022-06-26T11:36:55Z

Just an idea: #24660 (comment) make sending to slack more generic

Might be some side effect and multiple inheritance required in some cases or not.

airflow/providers/slack/transfers/sql_to_slack.py

Taragolis · 2022-06-26T11:51:57Z

@eladkal let's keep track here

The generic operator will handle this - if implemented right there will be no need for individual classes in each provider, again if you have comments on this lets please take them to #24663

I would suggest more generic way, SqlToSlackOperator this generic only for DBApi.

If we have something that I mentioned before: base class in slack-provider BasePandasDataFrameToSlackOperator without implementation get_pandas_df

It could help to build not only SqlToSlackOperator but also some user-classes like AwsDataWranglerDynamoDBToSlack, LocalDataFrameToSlack, etc.

Taragolis · 2022-06-26T12:04:40Z

Additional benefits you need to test implementation how actually works part with sending DataFrame in slack-provider (consumer), and how to produce this DataFrame in actual implementation, like in SqlToSlackOperator.

If any bugs appear in slack part, we need need to fix it slack-provider, if on producer part, only need to fix on producer.
Same work with new features.

Again, just an idea and I only talk about benefits without negative part of this idea.

eladkal · 2022-06-26T12:06:05Z

yes we can, but this is a further enhancement. If alex wants to handle this as well he is more than welcome :)
personally I'm OK with leaving it to a followup PR. It's OK to make a change in a few smaller PR.. it makes the review easier

Taragolis · 2022-06-26T12:36:26Z

yes we can, but this is a further enhancement. If alex wants to handle this as well he is more than welcome :)
personally I'm OK with leaving it to a followup PR. It's OK to make a change in a few smaller PR.. it makes the review easier

Totally agree that it should be further enhancement, otherwise it easily took ages to implements SqlToSlackOperator, and deprecate in Snowflake and Presto.

I will look on this PR more close on Monday. Probably I would suggest something that I've already found during SnowflakeToSlack enhancement.

Taragolis · 2022-06-27T08:13:55Z

airflow/providers/presto/transfers/presto_to_slack.py

+        'webhook_token' attribute needs to be specified in the 'Extra' JSON field against the slack_conn_id.py
+        Mutually exclusive with 'slack_conn_id'


webhook_token in Extra marked as Deprecated

airflow/airflow/providers/slack/hooks/slack_webhook.py

Lines 103 to 111 in b678dc2

extra = conn.extra_dejson

web_token = extra.get('webhook_token', '')

if web_token:

warnings.warn(

"'webhook_token' in 'extra' is deprecated. Please use 'password' field",

DeprecationWarning,

stacklevel=2,

)

airflow/providers/presto/transfers/presto_to_slack.py

…rams

eladkal · 2022-06-28T14:11:09Z

airflow/providers/slack/transfers/sql_to_slack.py

+    def _get_hook(self) -> DbApiHook:
+        self.log.debug("Get connection for %s", self.sql_conn_id)
+        conn = BaseHook.get_connection(self.sql_conn_id)
+        hook = conn.get_hook(hook_params=self.sql_hook_params)


@potiuk need your advise on this one.

The hook_params was added in #18718 / #19849 which were released on 2.3.0 The Slack provider min version is 2.2.0 so I think this code line should have cause failures?

2.2.0:
https://github.com/apache/airflow/blob/2.2.0/airflow/models/connection.py#L292

2.3.0:
https://github.com/apache/airflow/blob/2.3.0/airflow/models/connection.py#L321

I wonder how it make sense that the CI is green?

~~@alexkruc in any case we don't need the hook_params here.. the goal is just to check if the hook has the needed function, the user params are not required for that.~~

@alexkruc I think we have two options here.

backport the get_hook function into the operator code (temporarily, we will remove it once the provider will be Airflow>=2.3 compatible)

Leave the PrestoToSlackOperator SnowflakeToSlackOperator code as is with just a deprecation warning, in that solution we should also check Airflow version when using the operator as it works only with 2.3 (you can check from airflow.version import version )

I think option 1 is prefered as otherwise it means only users of Airflow>=2.3 will be able to use this new operator.

Yep. 1) is preferred.

A fix to pre-commit to make sure we don't have this case is in #24706
Thanks @potiuk

@potiuk is there a way to make the new pre commit ignore a specific row?
The pre-commit fails the PR but @alexkruc checks the user Airflow version and have different code to handle base on version.
We can use only the backported version if there is no other option

Reader as in myself? ;)
I'll take a look, will love to get familiar a bit with the pre-commit flow :)
I'll update here if I'll get stuck or something.

I just thought that checking for versions and matching will be easier to read for future operator maintainers, and also it's easier to deprecate later (just remove the if block), I think that using the generic methods (as in conn.get_hook(hook_params=....)) as much as possible is the better way here :)

@potiuk @eladkal added the ignore lines for compatibility check fix, you can find it in this commit:
f3a29fb
It should run as part of this PR and we can check that it works e2e :) (although it worked locally)

Cool! That's what I meant. One small NIT: since you made a "generic" exclusion - let's make it # ignore airflow compat check, We have some 2.2, some 2.3 and some 2.4 checks already foreseen to be there for quite some time.

BTW. Pre-comits are SUPER easy :D

Great! thanks for the comment :) I changed it here - efd07a5

BTW. Pre-comits are SUPER easy :D

I agree! and besides being easy, they are also kinda fun 😄 The thing is, that as a user, and as a young contributor, I'm not often exposed to this, so it's nice learning something new about the Airflow eco-system ;)

…n for `SqlToS3Operator`

airflow/providers/slack/transfers/sql_to_slack.py

eladkal · 2022-06-28T19:51:35Z

airflow/providers/slack/transfers/sql_to_slack.py

+            # For supporting Airflow versions < 2.3, we backport "get_hook()" method. This should be removed
+            # when "apache-airflow-providers-slack" will depend on Airflow >= 2.3. Git reference:
+            # https://github.com/apache/airflow/blob/main/airflow/providers/slack/provider.yaml#L38
+            hook = _backported_get_hook(conn)


Shouldn't we pass the sql_hook_params as well? this is the whole reason for backporing the function isn't it?

your'e right :) fixed it :)

…ow into add_sql_to_slack_operator

Co-authored-by: eladkal <[email protected]>

potiuk · 2022-06-28T21:35:31Z

Needs rebase :(

…ow into add_sql_to_slack_operator

eladkal

LGTM

adding SqlToSlackOperator

259c193

alexkruc requested review from turbaszek, potiuk and mik-laj as code owners June 26, 2022 07:17

boring-cyborg bot added area:providers kind:documentation provider:snowflake Issues related to Snowflake provider labels Jun 26, 2022

eladkal requested changes Jun 26, 2022

View reviewed changes

alexkruc and others added 3 commits June 26, 2022 12:17

adding SqlToSlackOperator

a46a309

Update tests/system/providers/slack/example_sql_to_slack.py

6e7b54a

Co-authored-by: eladkal <[email protected]>

Update tests/system/providers/slack/example_sql_to_slack.py

4f67dc1

Co-authored-by: eladkal <[email protected]>

potiuk force-pushed the add_sql_to_slack_operator branch from ad4159c to 4f67dc1 Compare June 26, 2022 10:17

eladkal mentioned this pull request Jun 26, 2022

SnowflakeToSlackOperator: Use Slack API and send result as file #24660

Closed

Merge branch 'add_sql_to_slack_operator' of github.com:alexkruc/airfl…

cacb00d

…ow into add_sql_to_slack_operator

eladkal reviewed Jun 26, 2022

View reviewed changes

airflow/providers/presto/transfers/presto_to_slack.py Outdated Show resolved Hide resolved

airflow/providers/presto/transfers/presto_to_slack.py Outdated Show resolved Hide resolved

docs/apache-airflow-providers-slack/operators/sql_to_slack.rst Outdated Show resolved Hide resolved

eladkal reviewed Jun 26, 2022

View reviewed changes

airflow/providers/slack/transfers/sql_to_slack.py Show resolved Hide resolved

Taragolis reviewed Jun 27, 2022

View reviewed changes

Changes based on CR

dc1acf6

eladkal reviewed Jun 27, 2022

View reviewed changes

airflow/providers/presto/transfers/presto_to_slack.py Outdated Show resolved Hide resolved

alexkruc added 4 commits June 27, 2022 15:23

removing the logger initialization, using self.log

6589682

removing typo in PrestoToSlackOperator documentation

aac9fe1

removing mutual exclusive comment in PrestoToSlackOperator Slack pa…

4bb47db

…rams

Fixing CI tests

0cbf0d2

alexkruc and others added 7 commits June 27, 2022 18:41

Merge branch 'main' into add_sql_to_slack_operator

9b8c76d

Merge branch 'main' into add_sql_to_slack_operator

21cd6ff

changed slackhook related behaviour

e0c2421

fixing ci tests

9607fcd

removing unused imports and fixing conn_type in tests

3247f09

fix ci operator params

a8031d0

removing mutually exclusive comments for slack connection

de63ee0

eladkal reviewed Jun 28, 2022

View reviewed changes

alexkruc and others added 2 commits June 28, 2022 22:32

backporting connection.get_hook method and removing default slack_con…

387f478

…n for `SqlToS3Operator`

Merge branch 'main' into add_sql_to_slack_operator

7867a04

eladkal reviewed Jun 28, 2022

View reviewed changes

alexkruc and others added 4 commits June 28, 2022 23:00

adding sql_hook_params to the backport get_hook method

fe59554

Merge branch 'add_sql_to_slack_operator' of github.com:alexkruc/airfl…

dc4d85e

…ow into add_sql_to_slack_operator

Update airflow/providers/slack/transfers/sql_to_slack.py

d6cc88a

Co-authored-by: eladkal <[email protected]>

Update airflow/providers/slack/transfers/sql_to_slack.py

748eabc

Co-authored-by: eladkal <[email protected]>

alexkruc and others added 5 commits June 29, 2022 09:14

testing exception thrown

bf522b3

Merge branch 'add_sql_to_slack_operator' of github.com:alexkruc/airfl…

c8670b4

…ow into add_sql_to_slack_operator

Merge branch 'main' into add_sql_to_slack_operator

3462a7f

fix indentation and ci tests

f33699c

adding skip tests for pre-commit hooks on 2.2 comp

f3a29fb

alexkruc requested a review from ashb as a code owner June 29, 2022 08:39

changed ignore compatibility check comment name

efd07a5

eladkal approved these changes Jun 29, 2022

View reviewed changes

eladkal merged commit 13908c2 into apache:main Jun 29, 2022

eladkal mentioned this pull request Jun 29, 2022

Enhance SqlToSlackOperator to support attachments #9145

Closed

potiuk mentioned this pull request Jul 13, 2022

Status of testing Providers that were prepared on July 13, 2022 #25037

Closed

95 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding generic SqlToSlackOperator #24663

Adding generic SqlToSlackOperator #24663

alexkruc commented Jun 26, 2022

potiuk commented Jun 26, 2022

eladkal commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Taragolis commented Jun 26, 2022

eladkal commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Taragolis Jun 27, 2022

eladkal Jun 28, 2022 •

edited

Loading

eladkal Jun 28, 2022

potiuk Jun 28, 2022

eladkal Jun 28, 2022

eladkal Jun 29, 2022

alexkruc Jun 29, 2022

alexkruc Jun 29, 2022

potiuk Jun 29, 2022

potiuk Jun 29, 2022

alexkruc Jun 29, 2022

eladkal Jun 28, 2022

alexkruc Jun 28, 2022

potiuk commented Jun 28, 2022

eladkal left a comment

		'webhook_token' attribute needs to be specified in the 'Extra' JSON field against the slack_conn_id.py
		Mutually exclusive with 'slack_conn_id'

	extra = conn.extra_dejson
	web_token = extra.get('webhook_token', '')

	if web_token:
	warnings.warn(
	"'webhook_token' in 'extra' is deprecated. Please use 'password' field",
	DeprecationWarning,
	stacklevel=2,
	)

Adding generic SqlToSlackOperator #24663

Adding generic SqlToSlackOperator #24663

Conversation

alexkruc commented Jun 26, 2022

potiuk commented Jun 26, 2022

eladkal commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Taragolis commented Jun 26, 2022

eladkal commented Jun 26, 2022

Taragolis commented Jun 26, 2022

Choose a reason for hiding this comment

eladkal Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Jun 28, 2022

eladkal left a comment

Choose a reason for hiding this comment

eladkal Jun 28, 2022 •

edited

Loading