[GitHub] [airflow] Taragolis commented on a diff in pull request #28706: Update provide_bucket_name() decorator to handle new conn_type

[GitBox <gi...@apache.org>]

Taragolis commented on code in PR #28706:
URL: https://github.com/apache/airflow/pull/28706#discussion_r1062824388


##########
docs/apache-airflow-providers-amazon/connections/aws.rst:
##########
@@ -66,6 +66,8 @@ Extra (optional)
     Specify the extra parameters (as json dictionary) that can be used in AWS
     connection. All parameters are optional.
 
+    * ``s3_bucket_name``: Name of the S3 bucket that the hook should reference
+

Review Comment:
   I'd like the idea for move this setting in separate extra rather than use `schema`
   However I don't think service level parameter should stored in top level of AWS Connection Extra.
   
   The reason is simple - `boto3` provide access to 300+ AWS Services, and if we store it in top level, then we have a huge chance to have a mess in the future like we have now mess with credentials and assuming roles
   
   ```python
   Python 3.9.10 (main, Feb 25 2022, 16:54:01) 
   Type 'copyright', 'credits' or 'license' for more information
   IPython 8.7.0 -- An enhanced Interactive Python. Type '?' for help.
   PyDev console: using IPython 8.7.0
   Python 3.9.10 (main, Feb 25 2022, 16:54:01) 
   [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
   
   import boto3, botocore
   
   botocore.__version__
   Out[3]: '1.29.43'
   
   boto3.__version__
   Out[4]: '1.26.43'
   
   session = boto3.session.Session(region_name="us-east-1")
   len(session.get_available_services())
   Out[6]: 336
   ```
   
   I'd suggest to move it in separate parameter (dictionary) which store per service level, e.g.
   
   ```json
   {
     "service_config": {
       "s3": {
         "bucket_name": "awesome-bucket"
       },
       "sts": {
         "endpoint_url": "https://example.org"
       },
       "emr": {
         "job_flow_overrides": {"Name": "PiCalc", "ReleaseLabel": "emr-6.7.0"},
         "endpoint_url": "https://emr.example.org"
       }
   }
   
   ```
   
   This changes also might help with this PR: https://github.com/apache/airflow/pull/28545 (cc: @confusedpublic)
   And in long term allow us discontinue [Amazon Elastic MapReduce (EMR) Connection](https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/connections/emr.html#amazon-elastic-mapreduce-emr-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