Remove Amazon S3 Connection Type #25980

Taragolis · 2022-08-26T11:58:33Z

AWS S3 Connection it is just kind of alias for Amazon Web Services Connection and most possible exists for historical reason.

All community provided packages expected Amazon Web Services Connection.
Due to the fact that AWS S3 Connection do not give any advantages IMHO better mark it ~~deprecated~~ remove and inform users switch to Amazon Web Services Connection.

Taragolis · 2022-08-26T11:59:23Z

cc: @o-nikolas @vincbeck @ferruzzi

vincbeck

As usual, thanks for the cleanup!

airflow/providers/amazon/aws/hooks/base_aws.py

dstandish · 2022-08-26T21:29:49Z

airflow/providers/amazon/aws/hooks/s3.py

    conn_type = 's3'
-    hook_name = 'Amazon S3'
+    hook_name = 'Amazon S3 (Deprecated)'


are you deprecating the hook or the conn type?

here it looks like you're deprecating the hook?

hook_name only use in UI and CLI (airflow providers hooks) for custom name of Connection as it described in base hook module

airflow/airflow/hooks/base.py

Lines 108 to 111 in 5c52bbf

Additionally hook_name should be set when you want the hook to have a custom name in the UI selection

Name. If not specified, conn_name will be used.

I do not know the better way to mark connection as deprecated in the UI.

So it is additional way for notify users that better to use Amazon Web Services instead of Amazon S3. In additional to documentation (simple page) and warning in task logs.

ok sure, yeah it's weird that it's called hook name but it really means conn name... but it's not the only weird thing :)

do you not need to change conn_type as part of this deprecation? forgive me for not recalling all the details of how this works...

I don't think so. Just want to keep same behaviour in the ui so if user already create connection in the UI before then conn_type would s3.

But if change also conn_type I don't think old connection would associate with S3Hook and can't test connection in UI/API.

The main idea is remove conn_type and hook_name in the future release and after that this Connection Type removed from selected list in UI.

docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst

dstandish

Thought about this some more. I had myself convinced that we should simply remove the conn type immediately, thinking it was just a UI feature, because it would not break anything so there's no real reason to keep around for a deprecation period. And I was about to type that up in a comment....

HOWEVER, it then occurred to me that there is a feature where we can get the hook, given a conn. i.e. a method get_hook on `Connection.

It also exists as a method on BaseHook.

And, importantly, it is used in "generic transfer" operator.

So, i think if you remove s3 hook, you'd actually break usage of s3 with generic transfer operator. So maybe we shouldn't deprecate s3 conn type at all. Can you look into this and let me know what you think?

docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst

tests/providers/amazon/aws/utils/test_connection_wrapper.py

airflow/providers/amazon/aws/utils/connection_wrapper.py

Taragolis · 2022-08-31T12:31:30Z

Sorry I've been sick for a couple of days.

I had myself convinced that we should simply remove the conn type immediately, thinking it was just a UI feature, because it would not break anything so there's no real reason to keep around for a deprecation period.

Personally for me it doesn't matter remove it now or later. Might be helpful for user to know about deprecation state of Connection in the UI.

And, importantly, it is used in "generic transfer" operator.
So, i think if you remove s3 hook, you'd actually break usage of s3 with generic transfer operator. So maybe we shouldn't deprecate s3 conn type at all. Can you look into this and let me know what you think?

Hmmm... might be I miss something. But do we actually have this generic transfer operator? All operator which uses for transfer in/out to S3 uses aws_conn_id with "aws_default" or None as connection id.

potiuk · 2022-09-09T02:05:42Z

I'd remove it. It might break someone's workflow (https://xkcd.com/1172/) but I tihnk we have not promised any "special" behaviour here.

I can imagine users who have S3 connections defined and use get_hook() to instantate the hook dynamically. This has been used for example in GenerictTransfer indeed but also I've seen it used in custom operators to depend on the conn_type to return particular Hook. We use it heavily in all SQL operators. BUT SQL operators have DBApiHook as base and follow the same protocols (and we unified them even more now via common.sql provider).

S3 Has no such promisses, no common interface, if there is any similarity with similar (GCS/Azure storage) hooks, this is at most incidental and I think this is about time that we don't care about those incidental similarities. Not having common "protocol" makes it next to impossible to use the hooks "dynamically" created by get_hook() depending on the connection type (though I Imagine someone doing '(S3Hook) get_hook("s3_conn")ratherthan S3Hook(conn_id=s3_conn)`. We would break the former workflow if we remove S3 conn_type - but as long as we describe it in changelog (including how ot conver it) and bump major version of Amazon provider, I have completely no problem with it.

Even the GenericTransfer @dstandish mentioned is Generic-ish, not truly Generic. It assumes that source Hook has "get_records" method and target hook has "run" (if preoperator is set) and "insert_rows' method. And when you look at the implementations - only SQL hooks follow it, so this really "Generic SQL transfer" operator.

I personally think "get_hook()" method only makes sense in SQL in our case and it useless elsewhere (though some individual workflows might use it and custom Hooks can take advantage of it).

I think personally that unless we have a well defined protocol to follow, any "generic" use of conn_type is next to useless - and in S3 case, I woudl not worry about deleting it.

However there is one thing that I think SHOULD be handled - rather than deprecatng it, I'd simply convert it dynamically to AWS automatically. currently, when someone has it defined and opens it in UI the configuration (keys etc) will be editable and saved. If we remove conn_type, the first time it is opened in the UI, I believe it will be converted dynamically to a "default" connection (not sure) and some information might be lost. So I would try to handle the scenario when somoene has S3 connection defined in the DB, opens it and it gets automatically converted to AWS one - not sure how to do it best though.

dstandish · 2022-09-09T19:59:35Z

ok sure, let's do it then.

@potiuk rather than "auto updating" the conn type, why don't we just warn the user? e.g. in base_aws.py we can add something like

                if str(connection.conn_type).lower() == 's3':
                    warnings.warn(
                        f"connection '{connection.conn_id}' has conn_type='s3', which is a deprecated conn "
                        f"type. type 's3' is deprecated.  Please update it to have conn type 'aws'."
                    )

after it retrieves the conn.

then it seems fair to immediately remove this conn type.

potiuk · 2022-09-09T20:02:08Z

ok sure, let's do it then.

@potiuk rather than "auto updating" the conn type, why don't we just warn the user? e.g. in base_aws.py we can add something like
                if str(connection.conn_type).lower() == 's3':
                    warnings.warn(
                        f"connection '{connection.conn_id}' has conn_type='s3', which is a deprecated conn "
                        f"type. type 's3' is deprecated.  Please update it to have conn type 'aws'."
                    )
after it retrieves the conn.

then it seems fair to immediately remove this conn type.

yep. Warning is better. I also do not like "auto" modifications...

Taragolis · 2022-09-13T10:50:44Z

@potiuk @potiuk Sorry for late response have a hectic week.
Let's summarise, right now we want to:

Remove Amazon S3 connection type entirely from the UI.
Inform user to switch from conn_type = "s3" to conn_type = "aws". BTW right now if user try to use connection with any type except aws it will inform with UserWarning

Am I understand correctly?

potiuk · 2022-09-13T10:56:50Z

Am I understand correctly?

Correct !

Taragolis · 2022-09-13T11:02:51Z

Correct !

Cool I will make changes in PR

airflow/providers/amazon/aws/utils/connection_wrapper.py

docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst

dstandish · 2022-09-13T16:03:30Z

airflow/providers/amazon/aws/utils/connection_wrapper.py

            warnings.warn(
-                f"{self.conn_repr} expected connection type 'aws', got {self.conn_type!r}.",
+                f"{self.conn_repr} has connection type 's3', which is deprecated. "
+                "Please update your connection and set `conn_type='aws'`.",


One thing .... it seems that we may cause some confusion or unnecessary alarm with this. Because.... nothing about this connection needs to be changed except the conn_type ... Seems maybe it would be worth adding a sentence emphasizing that somehow? what do you think?

airflow/providers/amazon/aws/utils/connection_wrapper.py

potiuk

LGTM. But I think changelog entry is needed in Provider (one of the few cases when a note is needed before we prepare providers).

potiuk · 2022-09-20T08:19:18Z

@Taragolis ? can you add a changelog entry pls ?

Taragolis · 2022-09-20T08:51:50Z

@potiuk Sure, I will try to add it during the day

Taragolis · 2022-09-20T14:02:41Z

airflow/providers/amazon/provider.yaml

-  - hook-class-name: airflow.providers.amazon.aws.hooks.s3.S3Hook
-    connection-type: s3


Also remove s3 from providers connection-type, otherwise webserver (provider manager?) warn about inconsistency and in UI appear two Amazon Web Services connections.

airflow/providers/amazon/CHANGELOG.rst

Taragolis requested a review from mik-laj as a code owner August 26, 2022 11:58

boring-cyborg bot added area:providers kind:documentation provider:amazon-aws AWS/Amazon - related issues labels Aug 26, 2022

Taragolis force-pushed the deprecate-s3-conn-type branch from 7db54ae to 780ffcd Compare August 26, 2022 12:02

vincbeck approved these changes Aug 26, 2022

View reviewed changes

dstandish reviewed Aug 26, 2022

View reviewed changes

dstandish reviewed Aug 28, 2022

View reviewed changes

docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst Outdated Show resolved Hide resolved

tests/providers/amazon/aws/utils/test_connection_wrapper.py Outdated Show resolved Hide resolved

airflow/providers/amazon/aws/utils/connection_wrapper.py Outdated Show resolved Hide resolved

Taragolis force-pushed the deprecate-s3-conn-type branch from 629ef50 to 2a9dce5 Compare August 31, 2022 12:21

Taragolis marked this pull request as draft September 13, 2022 11:03

Taragolis force-pushed the deprecate-s3-conn-type branch from 2a9dce5 to 8ea3502 Compare September 13, 2022 12:23

Taragolis changed the title ~~Deprecate AWS S3 Connection Type~~ Remove AWS S3 Connection Type Sep 13, 2022

Taragolis marked this pull request as ready for review September 13, 2022 12:24

Taragolis changed the title ~~Remove AWS S3 Connection Type~~ Remove Amazon S3 Connection Type Sep 13, 2022

Taragolis force-pushed the deprecate-s3-conn-type branch from 8ea3502 to bdb4492 Compare September 13, 2022 14:19

dstandish reviewed Sep 13, 2022

View reviewed changes

airflow/providers/amazon/aws/utils/connection_wrapper.py Outdated Show resolved Hide resolved

Taragolis force-pushed the deprecate-s3-conn-type branch from bdb4492 to 407f8f9 Compare September 13, 2022 19:27

dstandish approved these changes Sep 13, 2022

View reviewed changes

potiuk approved these changes Sep 18, 2022

View reviewed changes

Taragolis added 2 commits September 20, 2022 17:03

Remove Amazon S3 Connection Type

6328c7e

Update documentation and remove connection from provider list

31c84cc

Taragolis force-pushed the deprecate-s3-conn-type branch from 407f8f9 to 31c84cc Compare September 20, 2022 13:59

Taragolis commented Sep 20, 2022

View reviewed changes

dstandish approved these changes Sep 20, 2022

View reviewed changes

airflow/providers/amazon/CHANGELOG.rst Outdated Show resolved Hide resolved

Update changelog

5c3d1fe

potiuk approved these changes Sep 21, 2022

View reviewed changes

potiuk merged commit cf73cb7 into apache:main Sep 21, 2022

Taragolis deleted the deprecate-s3-conn-type branch September 21, 2022 21:33

This was referenced Sep 21, 2022

Add oracledb thick mode support for oracle provider #26576

Merged

Fix bad test for unknown hook type #26579

Merged

potiuk mentioned this pull request Sep 28, 2022

Status of testing Providers that were prepared on September 28, 2022 #26752

Closed

89 tasks

matthewblock mentioned this pull request Nov 10, 2022

Removal of s3 conn_type renders provide_bucket_name() useless #27587

Closed

2 tasks

o-nikolas mentioned this pull request Jan 19, 2023

Add transfer operator S3 to (generic) SQL #28964

Closed

eladkal mentioned this pull request Feb 10, 2023

Add support of a different AWS connection for DynamoDB #29452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Amazon S3 Connection Type #25980

Remove Amazon S3 Connection Type #25980

Taragolis commented Aug 26, 2022 •

edited

Loading

Taragolis commented Aug 26, 2022

vincbeck left a comment

dstandish Aug 26, 2022

Taragolis Aug 26, 2022

Taragolis Aug 26, 2022

dstandish Aug 27, 2022

dstandish Aug 27, 2022

Taragolis Aug 27, 2022

dstandish left a comment •

edited

Loading

Taragolis commented Aug 31, 2022

potiuk commented Sep 9, 2022

dstandish commented Sep 9, 2022

potiuk commented Sep 9, 2022

Taragolis commented Sep 13, 2022

potiuk commented Sep 13, 2022

Taragolis commented Sep 13, 2022 •

edited

Loading

dstandish Sep 13, 2022

potiuk left a comment

potiuk commented Sep 20, 2022

Taragolis commented Sep 20, 2022

Taragolis Sep 20, 2022


	Additionally hook_name should be set when you want the hook to have a custom name in the UI selection
	Name. If not specified, conn_name will be used.

		- hook-class-name: airflow.providers.amazon.aws.hooks.s3.S3Hook
		connection-type: s3

Remove Amazon S3 Connection Type #25980

Remove Amazon S3 Connection Type #25980

Conversation

Taragolis commented Aug 26, 2022 • edited Loading

Taragolis commented Aug 26, 2022

vincbeck left a 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

dstandish left a comment • edited Loading

Choose a reason for hiding this comment

Taragolis commented Aug 31, 2022

potiuk commented Sep 9, 2022

dstandish commented Sep 9, 2022

potiuk commented Sep 9, 2022

Taragolis commented Sep 13, 2022

potiuk commented Sep 13, 2022

Taragolis commented Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Sep 20, 2022

Taragolis commented Sep 20, 2022

Choose a reason for hiding this comment

Taragolis commented Aug 26, 2022 •

edited

Loading

dstandish left a comment •

edited

Loading

Taragolis commented Sep 13, 2022 •

edited

Loading