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/07/29 23:25:46 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #25416: Hide unused fields for Amazon Web Services connection
Taragolis opened a new pull request, #25416:
URL: https://github.com/apache/airflow/pull/25416
Follow up #25256
Seems like `host`, `port`, `schema` never used in AWS Connection, disable it 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] vincbeck commented on pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
vincbeck commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203198874
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
--
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 pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203188758
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. :+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] ferruzzi commented on a diff in pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r937023309
##########
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
--------
+**Snippet for create Connection as URI**:
+ .. code-block:: python
+
+ import os
+ from airflow.models.connection import Connection
+
+
+ conn = Connection(
+ conn_id="sample_aws_connection",
+ conn_type="aws",
+ login="AKIAIOSFODNN7EXAMPLE", # Reference to AWS Access Key ID
+ password="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", # Reference to AWS Secret Access Key
+ extra={
+ # Specify extra parameters here
+ "region_name": "eu-central-1",
+ },
+ )
+
+ # Generate Environment Variable Name and Connection URI
+ env_key, conn_uri = f"AIRFLOW_CONN_{conn.conn_id.upper()}", conn.get_uri()
Review Comment:
Nitpick, is there a reason these two assignments are on the same line?
##########
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
--------
Review Comment:
:1st_place_medal: Nice addition
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -582,6 +582,34 @@ def decorator_f(self, *args, **kwargs):
return retry_decorator
+ @staticmethod
+ def get_ui_field_behaviour() -> Dict[str, Any]:
+ """Returns custom UI field behaviour for AWS Connection."""
+ return {
Review Comment:
Very minor nitpick, the flipping back and forth between single and double quotes in the return value is odd.
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r933760449
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -582,6 +582,18 @@ def decorator_f(self, *args, **kwargs):
return retry_decorator
+ @staticmethod
+ def get_ui_field_behaviour() -> Dict[str, Any]:
+ """Returns custom field behaviour."""
+ return {
+ "hidden_fields": ['host', 'schema', 'port'],
+ "relabeling": {},
+ "placeholders": {
Review Comment:
@josh-fell Oh, it my second attempt for changing UI behaviour for AWS Connection. Previous one I even did not create PR in the end. Right now I tried to make it simple as possible only to show that host, port and schema not use in connection and follow the [Amazon Web Services Connection documentation](https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/connections/aws.html#configuring-the-connection).
The root issue is the fact that user could define `aws_access_key_id` and `aws_secret_access_key` in three different places.
In login/password:
```python
Connection(
conn_id="aws_conn_1",
conn_type="aws",
login="AKIAIOSFODNN7EXAMPLE",
password="login="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY",
)
```
In extra fields `aws_access_key_id` and `aws_secret_access_key`
```python
Connection(
conn_id="aws_conn_2",
conn_type="aws",
extra={
"aws_access_key_id": "AKIAIOSFODNN7EXAMPLE",
"aws_secret_access_key": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY",
}
)
```
In extra `session_kwargs` keys `aws_access_key_id` and `aws_secret_access_key` (not directly documented but this is mentioned in boto3 documentation)
```python
Connection(
conn_id="aws_conn_3",
conn_type="aws",
extra={
"session_kwargs": {
"aws_access_key_id": "AKIAIOSFODNN7EXAMPLE",
"aws_secret_access_key": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY",
}
}
)
```
I thought rename `login` and `password` to `aws_access_key_id` and `aws_secret_access_key` might confuse user more.
@eladkal The same things with extra columns in UI, such as `region_name`. Right now it is only possible add as `extra__{conn_type}__{field_name}`, which:
1. Would not work with AWS Connection right now
2. Required some changes for resolve cases such as
```python
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",
}
}
)
```
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r937037521
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -582,6 +582,34 @@ def decorator_f(self, *args, **kwargs):
return retry_decorator
+ @staticmethod
+ def get_ui_field_behaviour() -> Dict[str, Any]:
+ """Returns custom UI field behaviour for AWS Connection."""
+ return {
Review Comment:
Good point. I'm forget that `black` in airflow configure with `skip-string-normalization = true`
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1204116108
Update for review.
Seems like errors in CI not related to this 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] ferruzzi commented on a diff in pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r937022152
##########
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
--------
Review Comment:
:+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] josh-fell commented on pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1204662024
>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.
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r938122856
##########
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
--------
+**Snippet for create Connection as URI**:
+ .. code-block:: python
+
+ import os
+ from airflow.models.connection import Connection
+
+
+ conn = Connection(
+ conn_id="sample_aws_connection",
+ conn_type="aws",
+ login="AKIAIOSFODNN7EXAMPLE", # Reference to AWS Access Key ID
+ password="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", # Reference to AWS Secret Access Key
+ extra={
+ # Specify extra parameters here
+ "region_name": "eu-central-1",
+ },
+ )
+
+ # Generate Environment Variable Name and Connection URI
+ env_key, conn_uri = f"AIRFLOW_CONN_{conn.conn_id.upper()}", conn.get_uri()
Review Comment:
I think good use case is unpack from tuple but not in this situation. Thanks for mention
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203706947
@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
https://github.com/apache/airflow/blob/298be502c35006b7c3f011b676dbb4db0633bc74/airflow/providers/snowflake/hooks/snowflake.py#L85-L109
However I stoped by couple reasons:
1. AwsHook at that moment had a lot of ways to did the same things.
2. 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:
```python
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",
}
}
)
```
3. 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
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25416:
URL: https://github.com/apache/airflow/pull/25416
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r938065071
##########
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
--------
+**Snippet for create Connection as URI**:
+ .. code-block:: python
+
+ import os
+ from airflow.models.connection import Connection
+
+
+ conn = Connection(
+ conn_id="sample_aws_connection",
+ conn_type="aws",
+ login="AKIAIOSFODNN7EXAMPLE", # Reference to AWS Access Key ID
+ password="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", # Reference to AWS Secret Access Key
+ extra={
+ # Specify extra parameters here
+ "region_name": "eu-central-1",
+ },
+ )
+
+ # Generate Environment Variable Name and Connection URI
+ env_key, conn_uri = f"AIRFLOW_CONN_{conn.conn_id.upper()}", conn.get_uri()
Review Comment:
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.
--
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] vincbeck commented on pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
vincbeck commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203232991
Yes please :)
--
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] eladkal commented on a diff in pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r933724076
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -582,6 +582,18 @@ def decorator_f(self, *args, **kwargs):
return retry_decorator
+ @staticmethod
+ def get_ui_field_behaviour() -> Dict[str, Any]:
+ """Returns custom field behaviour."""
+ return {
+ "hidden_fields": ['host', 'schema', 'port'],
+ "relabeling": {},
+ "placeholders": {
Review Comment:
I agree
Having access_key, secret_key, region would make it easier to define the 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] potiuk commented on pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203110660
@o-nikolas @ferruzzi @vincbeck - WDYT?
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #25416:
URL: https://github.com/apache/airflow/pull/25416#issuecomment-1203223954
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](https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/connections/aws.html#configuring-the-connection)?
If everything fine, I will push updates tomorrow.
--
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] josh-fell commented on a diff in pull request #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r933690350
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -582,6 +582,18 @@ def decorator_f(self, *args, **kwargs):
return retry_decorator
+ @staticmethod
+ def get_ui_field_behaviour() -> Dict[str, Any]:
+ """Returns custom field behaviour."""
+ return {
+ "hidden_fields": ['host', 'schema', 'port'],
+ "relabeling": {},
+ "placeholders": {
Review Comment:
Rather than placeholders, would it make sense to relabel the form fields instead?
--
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 #25416: Hide unused fields for Amazon Web Services connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25416:
URL: https://github.com/apache/airflow/pull/25416#discussion_r937031237
##########
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
--------
+**Snippet for create Connection as URI**:
+ .. code-block:: python
+
+ import os
+ from airflow.models.connection import Connection
+
+
+ conn = Connection(
+ conn_id="sample_aws_connection",
+ conn_type="aws",
+ login="AKIAIOSFODNN7EXAMPLE", # Reference to AWS Access Key ID
+ password="wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", # Reference to AWS Secret Access Key
+ extra={
+ # Specify extra parameters here
+ "region_name": "eu-central-1",
+ },
+ )
+
+ # Generate Environment Variable Name and Connection URI
+ env_key, conn_uri = f"AIRFLOW_CONN_{conn.conn_id.upper()}", conn.get_uri()
Review Comment:
Oh... No reason. Just a copy-paste from my personal scratch file.
--
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