[GitHub] [airflow] dstandish commented on a diff in pull request #25980: Remove Amazon S3 Connection Type

[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