Hide unused fields for Amazon Web Services connection #25416

Taragolis · 2022-07-29T23:25:44Z

Follow up #25256

Seems like host, port, schema never used in AWS Connection, disable it in UI.

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

potiuk · 2022-08-02T19:10:48Z

ferruzzi · 2022-08-02T20:35:16Z

Neat. I didn't realize that was an option. I'll have to look into what else that may affect, but I think it's a safe change based on my current understanding. 👍

vincbeck · 2022-08-02T20:47:05Z

Thanks again for doing these changes! I like that. But I agree with @eladkal, I think renaming these fields to access_key and secret_key would be less confusing for the user

Taragolis · 2022-08-02T21:18:48Z

Due to the fact that @vincbeck @eladkal @josh-fell agreed that renaming would be less confusing users I prepared this option.

Also one question, do you think it is also required rename Login and Password in documentation?

If everything fine, I will push updates tomorrow.

vincbeck · 2022-08-02T21:29:55Z

Yes please :)

Taragolis · 2022-08-03T09:27:14Z

@gmcrocetti regarding to #25494

Again, my personal thought, I still would suggest to hide host because it never (at least since Airflow 1.9) use in AwsHook. And might be better replace extra['host'] by extra['endpoint_url'] for maintain uniformity with boto3.client and boto3.resource.

Couple month ago I've also played with extra fields in UI, by same way as it implemented in SnowflakeHook

airflow/airflow/providers/snowflake/hooks/snowflake.py

Lines 85 to 109 in 298be50

    
           @staticmethod 
        
           def get_connection_form_widgets() -> Dict[str, Any]: 
        
               """Returns connection widgets to add to connection form""" 
        
               from flask_appbuilder.fieldwidgets import BS3TextAreaFieldWidget, BS3TextFieldWidget 
        
               from flask_babel import lazy_gettext 
        
               from wtforms import BooleanField, StringField 
        
               return { 
        
                   "extra__snowflake__account": StringField(lazy_gettext('Account'), widget=BS3TextFieldWidget()), 
        
                   "extra__snowflake__warehouse": StringField( 
        
                       lazy_gettext('Warehouse'), widget=BS3TextFieldWidget() 
        
                   ), 
        
                   "extra__snowflake__database": StringField(lazy_gettext('Database'), widget=BS3TextFieldWidget()), 
        
                   "extra__snowflake__region": StringField(lazy_gettext('Region'), widget=BS3TextFieldWidget()), 
        
                   "extra__snowflake__role": StringField(lazy_gettext('Role'), widget=BS3TextFieldWidget()), 
        
                   "extra__snowflake__private_key_file": StringField( 
        
                       lazy_gettext('Private key (Path)'), widget=BS3TextFieldWidget() 
        
                   ), 
        
                   "extra__snowflake__private_key_content": StringField( 
        
                       lazy_gettext('Private key (Text)'), widget=BS3TextAreaFieldWidget() 
        
                   ), 
        
                   "extra__snowflake__insecure_mode": BooleanField( 
        
                       label=lazy_gettext('Insecure mode'), description="Turns off OCSP certificate checks" 
        
                   ), 
        
               }

However I stoped by couple reasons:

AwsHook at that moment had a lot of ways to did the same things.
All custom fields stored in Connection['extra'] as extra__{conn_type}__{field_name} in this case we need also care about some funny cases such as:

Connection(
    conn_id="aws_conn_hell",
    conn_type="aws",
    extra={
        "region_name": "us-east-1",
        "extra__aws__region_name": "eu-west-1",
        "session_kwargs": {
             "region_name": "cn-north-1",
        }
    }
)

Right now BaseSessionFactory supports custom SAML auth and WebIdentity by Google and both of them also have their own attributes in Connection['extra']

So I would suggest to make AwsHook and BaseSessionFactory use only generic fields in Connection, make easier way to add custom federation Auth and only after that we may add additional fields in the UI

Taragolis · 2022-08-03T15:33:48Z

Update for review.

Seems like errors in CI not related to this PR

ferruzzi

Overall, I like it. Left a couple of small suggestions, but no blocking fixes. 👍

ferruzzi · 2022-08-03T18:48:32Z

docs/apache-airflow-providers-amazon/connections/aws.rst

+    )
+
+    # Generate Environment Variable Name and Connection URI
+    env_key, conn_uri = f"AIRFLOW_CONN_{conn.conn_id.upper()}", conn.get_uri()


Nitpick, is there a reason these two assignments are on the same line?

Oh... No reason. Just a copy-paste from my personal scratch file.

I don't particularly hate it, and I know it's functionally the same thing, it's just not something I see in this codebase so I tend to avoid it for style consistency, that's all.

I think good use case is unpack from tuple but not in this situation. Thanks for mention

ferruzzi · 2022-08-03T18:49:02Z

docs/apache-airflow-providers-amazon/connections/aws.rst

@@ -104,6 +104,33 @@ If you are configuring the connection via a URI, ensure that all components of t
 Examples
 --------



🥇 Nice addition

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

vincbeck

LGTM. Again, thanks for all these changes!

josh-fell · 2022-08-04T01:40:39Z

Again, my personal thought, I still would suggest to hide host because it never (at least since Airflow 1.9) use in AwsHook. And might be better replace extra['host'] by extra['endpoint_url'] for maintain uniformity with boto3.client and boto3.resource.

Sounds reasonable. As part of #25494, host could simply be renamed to endpoint_url and the placeholder in Extra for host and just be moved up accordingly should that PR be something that should be implemented. I'll defer to you all with this though.

boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Jul 29, 2022

josh-fell reviewed Jul 29, 2022

View reviewed changes

airflow/providers/amazon/aws/hooks/base_aws.py Show resolved Hide resolved

Taragolis force-pushed the aws-connection-hide-unused-ui-fields branch 2 times, most recently from d97c480 to d44d9c2 Compare August 1, 2022 12:37

Taragolis mentioned this pull request Aug 3, 2022

Deprecate usage of extra[host] in AWS's connection #25494

Merged

Taragolis force-pushed the aws-connection-hide-unused-ui-fields branch from d44d9c2 to baf87ec Compare August 3, 2022 14:07

Taragolis requested a review from mik-laj as a code owner August 3, 2022 14:07

ferruzzi reviewed Aug 3, 2022

View reviewed changes

Taragolis added 3 commits August 3, 2022 23:16

Hide unused fields for Amazon Web Services connection

56764cc

Rename login and password, more placeholders and update docs

044b502

Minor changes in codestyle

7d5e7ff

Taragolis force-pushed the aws-connection-hide-unused-ui-fields branch from baf87ec to 7d5e7ff Compare August 3, 2022 19:16

vincbeck approved these changes Aug 3, 2022

View reviewed changes

josh-fell approved these changes Aug 4, 2022

View reviewed changes

potiuk approved these changes Aug 4, 2022

View reviewed changes

potiuk merged commit 171aaf0 into apache:main Aug 4, 2022

Taragolis deleted the aws-connection-hide-unused-ui-fields branch August 6, 2022 22:09

This was referenced Aug 10, 2022

Status of testing Providers that were prepared on August 10, 2022 #25634

Closed

Status of testing Providers that were prepared on August 10, 2022 #25640

Closed

eladkal mentioned this pull request Aug 23, 2022

Unable to override aws_default connection values from UI #18276

Closed

2 tasks

		@@ -104,6 +104,33 @@ If you are configuring the connection via a URI, ensure that all components of t
		Examples
		--------

Hide unused fields for Amazon Web Services connection #25416

Hide unused fields for Amazon Web Services connection #25416

Uh oh!

Conversation

Taragolis commented Jul 29, 2022

Uh oh!

Uh oh!

potiuk commented Aug 2, 2022

Uh oh!

ferruzzi commented Aug 2, 2022

Uh oh!

vincbeck commented Aug 2, 2022

Uh oh!

Taragolis commented Aug 2, 2022

Uh oh!

vincbeck commented Aug 2, 2022

Uh oh!

Taragolis commented Aug 3, 2022

Uh oh!

Taragolis commented Aug 3, 2022

Uh oh!

ferruzzi left a comment

Choose a reason for hiding this comment

Uh oh!

ferruzzi Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Taragolis Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

ferruzzi Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Taragolis Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

ferruzzi Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

josh-fell commented Aug 4, 2022

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!