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/11 15:43:32 UTC
[GitHub] [airflow] sunkickr opened a new pull request #14724: Add dynamic fields to Snowflake connection
sunkickr opened a new pull request #14724:
URL: https://github.com/apache/airflow/pull/14724
Adds form fields and custom form behavior for the Snowflake connection so it is more obvious to new users what fields need to be filled out. Also the doc_strings for the hook are updated to reflect the params along with helpful information using sphinx directives. I have categorized the fields below to explain the change.
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(secret)
Proposal
- 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 unless the input is a secret because of #8766
- Add fields for `aws_accces_key_id` and `aws_secret_access_key` because aws secret key is a secret
- Add placeholders so users have a little more information on fields
ex. Login: "your snowflake username"
screenshot:
Ideally `aws_accces_key_id` and `aws_secret_access_key` would be stored in Extra because they are not used by the average user. A feature in the UI that allows users to obfuscate the Extra field is needed for secretes to be stored there.
This PR is related to https://github.com/apache/airflow/pull/14631#event-4445426662 where something went wrong during rebase and the PR closed.
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#discussion_r592471970
##########
File path: airflow/operators/bash.py
##########
@@ -31,7 +31,7 @@
class BashOperator(BaseOperator):
- r"""
Review comment:
I don't remember doing this?
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#discussion_r592471970
##########
File path: airflow/operators/bash.py
##########
@@ -31,7 +31,7 @@
class BashOperator(BaseOperator):
- r"""
Review comment:
I don't remember doing this?
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-797170050
@kaxil 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] sunkickr commented on pull request #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-799037407
> @sunkickr I'm not sure if having AWS stuff in Snowflake connection is good idea. In future there can be any XtoSnowflake operator, will we be extending the list?
@turbaszek I agree that the AWS stuff shouldn't be in the connection, but the S3ToSnowflakeOperator is currently written in a way that requires this(I think). I thought rewriting the S3ToSnowflakeOperator was beyond the scope of this PR. Other transfers could have users set up a connection for each service so we wouldn't need to extend the list.
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-799691341
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-800491424
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-797678115
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-797043571
[The Workflow run](https://github.com/apache/airflow/actions/runs/644112028) 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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-797673874
The run associated with this PR appears to have failed because of secuity issue?
`Running job on this worker disallowed by security policy`
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#discussion_r592478806
##########
File path: airflow/operators/bash.py
##########
@@ -31,7 +31,7 @@
class BashOperator(BaseOperator):
- r"""
+ """
Execute a Bash script, command or set of commands.
Review comment:
I don't remember doing this?
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#discussion_r592836529
##########
File path: tests/core/test_providers_manager.py
##########
@@ -174,6 +172,13 @@
'extra__yandexcloud__public_ssh_key',
'extra__yandexcloud__service_account_json',
'extra__yandexcloud__service_account_json_path',
+ 'extra__snowflake__account',
+ 'extra__snowflake__warehouse',
+ 'extra__snowflake__database',
+ 'extra__snowflake__region',
+ 'extra__snowflake__aws_access_key_id',
+ 'extra__snowflake__aws_secret_access_key',
+
Review comment:
```suggestion
```
This is causing Static check failure: https://github.com/apache/airflow/pull/14724/checks?check_run_id=2089572707#step:9:270
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-799693617
> @sunkickr can you please rebase onto current master to rerun all builds?
Thank you for the review! I can 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#issuecomment-799687734
@sunkickr can you please rebase onto current master to rerun all builds?
----------------------------------------------------------------
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 merged pull request #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #14724:
URL: https://github.com/apache/airflow/pull/14724
----------------------------------------------------------------
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 #14724: Add dynamic fields to Snowflake connection
Posted by GitBox <gi...@apache.org>.
sunkickr commented on a change in pull request #14724:
URL: https://github.com/apache/airflow/pull/14724#discussion_r592478806
##########
File path: airflow/operators/bash.py
##########
@@ -31,7 +31,7 @@
class BashOperator(BaseOperator):
- r"""
+ """
Execute a Bash script, command or set of commands.
Review comment:
I don't remember doing this?
----------------------------------------------------------------
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