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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
