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 2021/03/05 22:51:33 UTC

[GitHub] [airflow] sunkickr opened a new pull request #14631: Add Dynamic fields to Snowflake Connection 

sunkickr opened a new pull request #14631:
URL: https://github.com/apache/airflow/pull/14631


   Adds form fields and custom form behavior for the Snowflake connection so that it is more obvious to new users what fields need to be filled out. I have divided the inputs into three main categories and created three proposals. The code in this PR implements proposal 1. Also the doc_strings for the hook are updated to reflect the params along with helpful information using sphinx directives. 
   
   Fields/inputs Used by the average user
   
   - Conn Id
   - Conn Type
   - Host
   - Schema
   - Login
   - Password
   - account
   - database
   - region
   
   Fields/inputs not used by user
   
   - Port
   
   Fields/inputs not used by user but reflected in code(not used by average user)
   
   - role
   - authenticator
   - private_key_field
   - session_parameters
   - aws_access_key_id
   - aws_secret_access_key
   
   ## Proposal 1
   
   - Add fields used by the average user, remove fields not used by the users, and allow users to use extras for fields that are not used by average user
   - Add placeholders so users have a little more information on fields
       - ex. Login: "your snowflake username"
   
   screenshot:
   ![image](https://user-images.githubusercontent.com/63181127/110177087-67064380-7dd2-11eb-917b-6597ba3724a1.png)
   
   
   ## Proposal 2
   
   - Add all fields used reflected as options in the code except AWS, remove fields not used by the users, allow users to use extras for AWS
   - Add placeholders so users have a little more information on fields
       - ex. Login: "your snowflake username"
   
   ## Proposal 3
   
   - Add all fields used reflected as options in the code, remove fields not used by the users including extras
   - Add placeholders so users have a little more information on fields
       - ex. Login: "your snowflake username"
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794009405


   > We should not suggest to save secrets in extra field if it's not encrypted.
   > We have the same issue with Salesforce #8766
   
   I don't think there is anything sensitive in the extra field here, is there?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] eladkal commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-791789281


   We should not suggest to save secrets in extra field if it's not encrypted.
   We have the same issue with Salesforce https://github.com/apache/airflow/issues/8766


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794039212


   > > We should not suggest to save secrets in extra field if it's not encrypted.
   > > We have the same issue with Salesforce #8766
   > 
   > I don't think there is anything sensitive in the extra field here, is there @eladkal?
   > 
   > ```
   >         account = conn.extra_dejson.get('extra__snowflake__account', '')
   >         warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
   >         database = conn.extra_dejson.get('extra__snowflake__database', '')
   >         region = conn.extra_dejson.get('extra__snowflake__region', '')
   > ```
   > 
   > Those all seem safe.
   
   This may be the code that he is worried about
   https://github.com/apache/airflow/blob/99aab051600715a1ad029ce45a197a8492e5a151/airflow/providers/snowflake/hooks/snowflake.py#L136-L139 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-791781061


   [The Workflow run](https://github.com/apache/airflow/actions/runs/625939193) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] eladkal commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794284493


   >This may be the code that @eladkal is worried about
   > https://github.com/apache/airflow/blob/99aab051600715a1ad029ce45a197a8492e5a151/airflow/providers/snowflake/hooks/snowflake.py#L136-L139
   
   Yes. I raised this because proposal 1 means we recognize that the aws secret should be saved in the extra field.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589789265



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       Something like this
   
   ```python
   if conn.extra_dejson.get('account', ''):
       warnings.warn(
           "Passing Snowflake account via extras is deprecated, set the account field instead'",
           DeprecationWarning,
           stacklevel=2,
       )
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589779902



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       world this be a proper fix? 
   ```python
   account = conn.extra_dejson.get('extra__snowflake__account', '') \
               or conn.extra_dejson.get('account', '')
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-791782163


   [The Workflow run](https://github.com/apache/airflow/actions/runs/625944985) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-792791717


   https://github.com/apache/airflow/blob/b718495e4caecb753742c3eb22919411a715f24a/tests/core/test_providers_manager.py#L81-L82 It appears that this issue as been fixed?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589816718



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       thankyou!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589752564



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       This is a breaking change, current users who use `account` / `warehouse` / `database` or `region` won't work




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589789265



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       Something like this
   
   ```python
   account = conn.extra_dejson.get('extra__snowflake__account', '')
   dep_account = conn.extra_dejson.get('account', '')
   if dep_account:
       warnings.warn(
           "Passing Snowflake account via extras is deprecated, set the account field instead'",
           DeprecationWarning,
           stacklevel=2,
       )
       account = dep_account
   
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr closed pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr closed pull request #14631:
URL: https://github.com/apache/airflow/pull/14631


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] petedejoy commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
petedejoy commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794462769


   So do we think it makes sense to expose the AWS secret as a separate optional field on the form? A bit counter-intuitive, but I don't see any other way to design around it without building a feature that allows the user to encrypt extras.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-791781322


   [The Workflow run](https://github.com/apache/airflow/actions/runs/625903367) is cancelling this PR. Building image for the PR has been cancelled


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] petedejoy edited a comment on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
petedejoy edited a comment on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794462769


   So do we think it makes sense to expose the AWS secret as a separate optional field on the form? A bit counter-intuitive, but I don't see any other way to design around it without building a feature that allows the user to encrypt extras.
   
   Edit: Unless we think it's worth investing in building a `Secret?` checkbox that allows the user to obfuscate values in extras?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r590737015



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       Should I just make sure it works with both names for now? Without the deprecation warning?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r589816718



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       thank you!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r590561537



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       If there is a way that would be good but currently it is used in a some providers:
   
   ```
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:47:    - use in cluster configuration by using ``extra__kubernetes__in_cluster`` in connection
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:48:    - use custom config by providing path to the file using ``extra__kubernetes__kube_config_path``
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:50:        ``extra__kubernetes__kube_config`` in connection
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:77:            "extra__kubernetes__in_cluster": BooleanField(lazy_gettext('In cluster configuration')),
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:78:            "extra__kubernetes__kube_config_path": StringField(
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:81:            "extra__kubernetes__kube_config": StringField(
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:84:            "extra__kubernetes__namespace": StringField(
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:108:        in_cluster = extras.get("extra__kubernetes__in_cluster")
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:109:        kubeconfig_path = extras.get("extra__kubernetes__kube_config_path")
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:110:        kubeconfig = extras.get("extra__kubernetes__kube_config")
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:115:                "Invalid connection configuration. Options extra__kubernetes__kube_config_path, "
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:116:                "extra__kubernetes__kube_config, extra__kubernetes__in_cluster are mutually exclusive. "
   airflow/providers/cncf/kubernetes/hooks/kubernetes.py:213:        namespace = extras.get("extra__kubernetes__namespace", "default")
   airflow/providers/google/ads/hooks/ads.py:134:        secret = secret_conn.extra_dejson["extra__google_cloud_platform__keyfile_dict"]
   airflow/providers/google/cloud/hooks/cloud_sql.py:402:GCP_CREDENTIALS_KEY_PATH = "extra__google_cloud_platform__key_path"
   airflow/providers/google/cloud/hooks/cloud_sql.py:403:GCP_CREDENTIALS_KEYFILE_DICT = "extra__google_cloud_platform__keyfile_dict"
   airflow/providers/google/cloud/hooks/cloud_sql.py:531:            project_id = connection.extra_dejson.get('extra__google_cloud_platform__project')
   airflow/providers/google/cloud/hooks/dataprep.py:50:        self._token = extra_dejson.get("extra__dataprep__token")
   airflow/providers/google/cloud/hooks/dataprep.py:51:        self._base_url = extra_dejson.get("extra__dataprep__base_url", "https://api.clouddataprep.com")
   airflow/providers/google/cloud/operators/cloud_sql.py:1102:                'extra__google_cloud_platform__project'
   airflow/providers/google/cloud/utils/credentials_provider.py:63:    extras = "extra__google_cloud_platform"
   airflow/providers/google/common/hooks/base_google.py:173:            "extra__google_cloud_platform__project": StringField(
   airflow/providers/google/common/hooks/base_google.py:176:            "extra__google_cloud_platform__key_path": StringField(
   airflow/providers/google/common/hooks/base_google.py:179:            "extra__google_cloud_platform__keyfile_dict": PasswordField(
   airflow/providers/google/common/hooks/base_google.py:182:            "extra__google_cloud_platform__scope": StringField(
   airflow/providers/google/common/hooks/base_google.py:185:            "extra__google_cloud_platform__num_retries": IntegerField(
   airflow/providers/google/common/hooks/base_google.py:293:        long_f = f'extra__google_cloud_platform__{f}'
   airflow/providers/grpc/hooks/grpc.py:63:            "extra__grpc__auth_type": StringField(
   airflow/providers/grpc/hooks/grpc.py:66:            "extra__grpc__credential_pem_file": StringField(
   airflow/providers/grpc/hooks/grpc.py:69:            "extra__grpc__scopes": StringField(
   airflow/providers/grpc/hooks/grpc.py:163:        full_field_name = f'extra__grpc__{field_name}'
   airflow/providers/jdbc/hooks/jdbc.py:50:            "extra__jdbc__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
   airflow/providers/jdbc/hooks/jdbc.py:51:            "extra__jdbc__drv_clsname": StringField(
   airflow/providers/jdbc/hooks/jdbc.py:69:        jdbc_driver_loc: Optional[str] = conn.extra_dejson.get('extra__jdbc__drv_path')
   airflow/providers/jdbc/hooks/jdbc.py:70:        jdbc_driver_name: Optional[str] = conn.extra_dejson.get('extra__jdbc__drv_clsname')
   airflow/providers/yandex/hooks/yandex.py:49:            "extra__yandexcloud__service_account_json": PasswordField(
   airflow/providers/yandex/hooks/yandex.py:56:            "extra__yandexcloud__service_account_json_path": StringField(
   airflow/providers/yandex/hooks/yandex.py:63:            "extra__yandexcloud__oauth": PasswordField(
   airflow/providers/yandex/hooks/yandex.py:69:            "extra__yandexcloud__folder_id": StringField(
   airflow/providers/yandex/hooks/yandex.py:75:            "extra__yandexcloud__public_ssh_key": StringField(
   airflow/providers/yandex/hooks/yandex.py:135:        long_f = f'extra__yandexcloud__{field_name}'
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-792337205


   @eladkal I agree the easiest solution I see would be too add encrypted fields for all secrets. Is there a better solution I am missing here?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r590783709



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       hmm sure




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-793081462


   > https://github.com/apache/airflow/issues/12881
   
   yup it's fixed


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb edited a comment on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794009405


   > We should not suggest to save secrets in extra field if it's not encrypted.
   > We have the same issue with Salesforce #8766
   
   I don't think there is anything sensitive in the extra field here, is there @eladkal?
   
   ```
           account = conn.extra_dejson.get('extra__snowflake__account', '')
           warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
           database = conn.extra_dejson.get('extra__snowflake__database', '')
           region = conn.extra_dejson.get('extra__snowflake__region', '')
   ```
   
   Those all seem safe.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr edited a comment on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr edited a comment on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-794039212


   > > We should not suggest to save secrets in extra field if it's not encrypted.
   > > We have the same issue with Salesforce #8766
   > 
   > I don't think there is anything sensitive in the extra field here, is there @eladkal?
   > 
   > ```
   >         account = conn.extra_dejson.get('extra__snowflake__account', '')
   >         warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
   >         database = conn.extra_dejson.get('extra__snowflake__database', '')
   >         region = conn.extra_dejson.get('extra__snowflake__region', '')
   > ```
   > 
   > Those all seem safe.
   
   This may be the code that @eladkal is worried about
   https://github.com/apache/airflow/blob/99aab051600715a1ad029ce45a197a8492e5a151/airflow/providers/snowflake/hooks/snowflake.py#L136-L139 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-791770889


   Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
   Here are some useful points:
   - Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
   - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
   - Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโ€™s a heavy docker but it ships with a working Airflow and a lot of integrations.
   - Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
   - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
   - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
   Apache Airflow is a community-driven project and together we are making it better ๐Ÿš€.
   In case of doubts contact the developers at:
   Mailing List: dev@airflow.apache.org
   Slack: https://s.apache.org/airflow-slack
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr edited a comment on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr edited a comment on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-792791717


   https://github.com/apache/airflow/blob/b718495e4caecb753742c3eb22919411a715f24a/tests/core/test_providers_manager.py#L81-L82 It appears that this issue has been fixed?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on a change in pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#discussion_r590443571



##########
File path: airflow/providers/snowflake/hooks/snowflake.py
##########
@@ -58,10 +135,10 @@ def _get_conn_params(self) -> Dict[str, Optional[str]]:
         conn = self.get_connection(
             self.snowflake_conn_id  # type: ignore[attr-defined] # pylint: disable=no-member
         )
-        account = conn.extra_dejson.get('account', '')
-        warehouse = conn.extra_dejson.get('warehouse', '')
-        database = conn.extra_dejson.get('database', '')
-        region = conn.extra_dejson.get("region", '')
+        account = conn.extra_dejson.get('extra__snowflake__account', '')
+        warehouse = conn.extra_dejson.get('extra__snowflake__warehouse', '')
+        database = conn.extra_dejson.get('extra__snowflake__database', '')
+        region = conn.extra_dejson.get('extra__snowflake__region', '')

Review comment:
       Hold off on deprecating the old name -- I'd like to see if it's possible to not "leak" the `extra__snowflake__` part -- that's a UI abstraction that has crept in that shouldn't be in the extra JSON.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr commented on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-796810257


   I attempted to rebase my branch onto apache master and it added all commits that have been merged this week.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] sunkickr edited a comment on pull request #14631: Add Dynamic fields to Snowflake Connection

Posted by GitBox <gi...@apache.org>.
sunkickr edited a comment on pull request #14631:
URL: https://github.com/apache/airflow/pull/14631#issuecomment-792337205


   @eladkal I agree, the easiest solution I see would be too add encrypted fields for all secrets. Is there a better solution I am missing here?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org