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/08/26 11:58:36 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #25980: Deprecate AWS S3 Connection Type
Taragolis opened a new pull request, #25980:
URL: https://github.com/apache/airflow/pull/25980
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 and inform users switch to Amazon Web Services Connection.
--
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] dstandish commented on pull request #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1242405716
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.
--
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] dstandish commented on a diff in pull request #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956456616
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -352,9 +352,13 @@ def _read_credentials_from_connection(self) -> Tuple[Optional[str], Optional[str
class AwsGenericHook(BaseHook, Generic[BaseAwsConnection]):
"""
- Interact with AWS.
+ Generic Class for interact with AWS.
Review Comment:
```suggestion
Generic class for interacting with AWS.
```
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
are you deprecating the _hook_ or the _conn type_?
here it looks like you're deprecating the hook?
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,13 +37,16 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
# Use server-side encryption for logs stored in S3
encrypt_s3_logs = False
-In the above example, Airflow will try to use ``S3Hook('MyS3Conn')``.
+In the above example, Airflow will try to use ``S3Hook(aws_conn_id='aws_s3_conn')``.
+
+Local test s3 remote logging
+''''''''''''''''''''''''''''
You can also use `LocalStack <https://localstack.cloud/>`_ to emulate Amazon S3 locally.
To configure it, you must additionally set the endpoint url to point to your local stack.
-You can do this via the Connection Extra ``host`` field.
-For example, ``{"host": "http://localstack:4572"}``
+You can do this via the Connection Extra ``endpoint_url`` field.
+For example, ``{"endpoint_url": "http://localstack:4572"}``
Review Comment:
what does this change have to do with deprecating the conn type?
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1242407785
> 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...
--
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] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969806558
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ elif self.conn_type != "aws":
+ warnings.warn(
+ f"{self.conn_repr} expected connection type 'aws', got {self.conn_type!r}. "
+ "This connection might not work correctly. "
Review Comment:
Do you mean in general? or specifically with regard to the web interface?
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
Review Comment:
conn type, if not read from DB, can be None.
e.g.
```
import os
os.environ['AIRFLOW_CONN_HELLO']='{"login": "me"}'
from airflow.hooks.base import BaseHook
assert BaseHook.get_connection('hello').conn_type is None
assert Connection(conn_id='blah').conn_type is None
```
so it would be safer to convert to string first
```suggestion
if str(self.conn_type).lower() == "s3":
```
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,11 +37,11 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
# Use server-side encryption for logs stored in S3
encrypt_s3_logs = False
-In the above example, Airflow will try to use ``S3Hook('MyS3Conn')``.
+In the above example, Airflow will try to use ``S3Hook(aws_conn_id='aws_s3_conn')``.
Review Comment:
```suggestion
In the above example, Airflow will try to use ``S3Hook(aws_conn_id='my_s3_conn')``.
```
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
Review Comment:
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?
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,11 +37,11 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
Review Comment:
```suggestion
remote_log_conn_id = my_s3_conn
```
i like minimizing the chance that a user will think that the name has magic... and keeping something like "my" is one way to do that.
--
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] ferruzzi commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969866539
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
Review Comment:
I didn't particularly agree that the old wording needed to be changed, but seeing this suggestion, I do like it better. :+1:
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1245242749
> Am I understand correctly?
Correct !
--
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] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969838773
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
Review Comment:
ok, i offered a concrete suggestion. i think the the word "deprecated" is really the one that has the potential to confuse. it makes it sound like "you can't use s3 anymore". but really, all we're saying is, s3 connections should now be stored as aws connections.
--
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] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969842853
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
Review Comment:
ah i see
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1252004441
@Taragolis ? can you add a changelog entry pls ?
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r975408211
##########
airflow/providers/amazon/provider.yaml:
##########
@@ -548,8 +548,6 @@ extra-links:
- airflow.providers.amazon.aws.links.logs.CloudWatchEventsLink
connection-types:
- - hook-class-name: airflow.providers.amazon.aws.hooks.s3.S3Hook
- connection-type: s3
Review Comment:
Also remove s3 from providers connection-type, otherwise webserver (provider manager?) warn about inconsistency and in UI appear two Amazon Web Services connections.
--
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] dstandish commented on a diff in pull request #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956596936
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
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...
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1232875657
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.
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1245236755
@potiuk @potiuk Sorry for late response have a hectic week.
Let's summarise, right now we want to:
1. Remove Amazon S3 connection type entirely from the UI.
2. 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](https://github.com/apache/airflow/blob/ffee6bceb32eba159a7a25a4613d573884a6a58d/airflow/providers/amazon/aws/utils/connection_wrapper.py#L130-L135)
Am I understand correctly?
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1241415888
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")` rather `than 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.
--
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] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r975554293
##########
airflow/providers/amazon/CHANGELOG.rst:
##########
@@ -24,6 +24,14 @@
Changelog
---------
+.. warning::
+ In this version of provider Amazon S3 Connection (``conn_type="s3"``) removed due to the fact that it was always
+ an alias to :ref:`Amazon Web Services Connection <howto/connection:aws>` (``conn_type="aws"``).
+ This might only have effect to testing connection in the UI/API.
+ In order to restore ability to test connection you need to change connection type from **Amazon S3**
+ to **Amazon Web Services** manually.
Review Comment:
```suggestion
In practice the only impact is you won't be able to ``test`` the connection in the web UI / API.
In order to restore ability to test connection you need to change connection type from **Amazon S3** (``conn_type="s3"``)
to **Amazon Web Services** (``conn_type="aws"``) manually.
```
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1245249107
> Correct !
Coll I will make changes in PR
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969840119
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ elif self.conn_type != "aws":
+ warnings.warn(
+ f"{self.conn_repr} expected connection type 'aws', got {self.conn_type!r}. "
+ "This connection might not work correctly. "
Review Comment:
This is in general how it work right now, if user not provide expected `conn_type == "aws"` then inform user that it might not work correctly.
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25980:
URL: https://github.com/apache/airflow/pull/25980
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1228402974
cc: @o-nikolas @vincbeck @ferruzzi
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956479640
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,13 +37,16 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
# Use server-side encryption for logs stored in S3
encrypt_s3_logs = False
-In the above example, Airflow will try to use ``S3Hook('MyS3Conn')``.
+In the above example, Airflow will try to use ``S3Hook(aws_conn_id='aws_s3_conn')``.
+
+Local test s3 remote logging
+''''''''''''''''''''''''''''
You can also use `LocalStack <https://localstack.cloud/>`_ to emulate Amazon S3 locally.
To configure it, you must additionally set the endpoint url to point to your local stack.
-You can do this via the Connection Extra ``host`` field.
-For example, ``{"host": "http://localstack:4572"}``
+You can do this via the Connection Extra ``endpoint_url`` field.
+For example, ``{"endpoint_url": "http://localstack:4572"}``
Review Comment:
Just noticed that in the documentation page contain outdated info and decide to change it in one go rather than separate PR. Ig it is a problem I could revert it back.
--
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] dstandish commented on a diff in pull request #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956596003
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
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 :)
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956479640
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,13 +37,16 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
# Use server-side encryption for logs stored in S3
encrypt_s3_logs = False
-In the above example, Airflow will try to use ``S3Hook('MyS3Conn')``.
+In the above example, Airflow will try to use ``S3Hook(aws_conn_id='aws_s3_conn')``.
+
+Local test s3 remote logging
+''''''''''''''''''''''''''''
You can also use `LocalStack <https://localstack.cloud/>`_ to emulate Amazon S3 locally.
To configure it, you must additionally set the endpoint url to point to your local stack.
-You can do this via the Connection Extra ``host`` field.
-For example, ``{"host": "http://localstack:4572"}``
+You can do this via the Connection Extra ``endpoint_url`` field.
+For example, ``{"endpoint_url": "http://localstack:4572"}``
Review Comment:
Just noticed that in the documentation page contain outdated info and decide to change it in one go rather than separate PR. If it is a problem I will revert it back.
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956602770
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
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.
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969835611
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
Review Comment:
If conn_type is None or empty than replaced to `aws`
https://github.com/apache/airflow/blob/bdb4492525f4cb1bd129d5977f8f3fb4c0c6b850/airflow/providers/amazon/aws/utils/connection_wrapper.py#L125-L126
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956478482
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
`hook_name` only use in UI and CLI (`airflow providers hooks`) for custom name of Connection as it described in base hook module
https://github.com/apache/airflow/blob/5c52bbf32d81291b57d051ccbd1a2479ff706efc/airflow/hooks/base.py#L108-L111
--
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 #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956483167
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -115,7 +115,7 @@ class S3Hook(AwsBaseHook):
"""
conn_type = 's3'
- hook_name = 'Amazon S3'
+ hook_name = 'Amazon S3 (Deprecated)'
Review Comment:
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.
--
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] dstandish commented on a diff in pull request #25980: Deprecate AWS S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r956597599
##########
docs/apache-airflow-providers-amazon/logging/s3-task-handler.rst:
##########
@@ -37,13 +37,16 @@ To enable this feature, ``airflow.cfg`` must be configured as follows:
# id that provides access to the storage location.
remote_logging = True
remote_base_log_folder = s3://my-bucket/path/to/logs
- remote_log_conn_id = MyS3Conn
+ remote_log_conn_id = aws_s3_conn
# Use server-side encryption for logs stored in S3
encrypt_s3_logs = False
-In the above example, Airflow will try to use ``S3Hook('MyS3Conn')``.
+In the above example, Airflow will try to use ``S3Hook(aws_conn_id='aws_s3_conn')``.
+
+Local test s3 remote logging
+''''''''''''''''''''''''''''
You can also use `LocalStack <https://localstack.cloud/>`_ to emulate Amazon S3 locally.
To configure it, you must additionally set the endpoint url to point to your local stack.
-You can do this via the Connection Extra ``host`` field.
-For example, ``{"host": "http://localstack:4572"}``
+You can do this via the Connection Extra ``endpoint_url`` field.
+For example, ``{"endpoint_url": "http://localstack:4572"}``
Review Comment:
i see... it was already deprecated... no prob, thanks
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -137,11 +137,22 @@ def __post_init__(self, conn: "Connection"):
self.extra_config = deepcopy(conn.extra_dejson)
if self.conn_type != "aws":
- warnings.warn(
- f"{self.conn_repr} expected connection type 'aws', got {self.conn_type!r}.",
- UserWarning,
- stacklevel=2,
- )
+ if self.conn_type.lower() == "s3":
+ warnings.warn(
+ f"{self.conn_repr}: use 's3' connection type instead of 'aws' "
+ "is deprecated and will be removed in a future releases. "
+ "Please switch to Amazon Web Services Connection type.",
Review Comment:
```suggestion
f"{self.conn_repr}: Connection type 's3' is deprecated. "
"Please update your connection and set `conn_type='aws'`.",
```
##########
tests/providers/amazon/aws/utils/test_connection_wrapper.py:
##########
@@ -77,13 +77,24 @@ def test_expected_aws_connection_type(self, conn_type):
wrap_conn = AwsConnectionWrapper(conn=mock_connection_factory(conn_type=conn_type))
assert wrap_conn.conn_type == "aws"
- @pytest.mark.parametrize("conn_type", ["AWS", "boto3", "s3", "emr", "google", "google-cloud-platform"])
+ @pytest.mark.parametrize("conn_type", ["AWS", "boto3", "emr", "google", "google-cloud-platform"])
def test_unexpected_aws_connection_type(self, conn_type):
warning_message = f"expected connection type 'aws', got '{conn_type}'"
with pytest.warns(UserWarning, match=warning_message):
wrap_conn = AwsConnectionWrapper(conn=mock_connection_factory(conn_type=conn_type))
assert wrap_conn.conn_type == conn_type
+ @pytest.mark.parametrize("conn_type", ["s3", "S3"])
+ def test_deprecated_s3_connection_type(self, conn_type):
+ warning_message = (
+ r"use 's3' connection type instead of 'aws' "
+ r"is deprecated and will be removed in a future releases\. "
+ r"Please switch to Amazon Web Services Connection type\."
+ )
Review Comment:
```suggestion
warning_message = (
"Connection type 's3' is deprecated. Please update your connection and set `conn_type='aws'`."
)
```
--
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 #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25980:
URL: https://github.com/apache/airflow/pull/25980#issuecomment-1252042030
@potiuk Sure, I will try to add it during the day
--
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] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25980:
URL: https://github.com/apache/airflow/pull/25980#discussion_r969837538
##########
airflow/providers/amazon/aws/utils/connection_wrapper.py:
##########
@@ -127,9 +127,18 @@ def __post_init__(self, conn: "Connection"):
self.password = conn.password
self.extra_config = deepcopy(conn.extra_dejson)
- if self.conn_type != "aws":
+ if self.conn_type.lower() == "s3":
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'`.",
Review Comment:
```suggestion
f"{self.conn_repr} has connection type 's3', which has been replaced by connection type 'aws'. "
"Please update your connection to have `conn_type='aws'`.",
```
--
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