Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dynamic fields to Snowflake connection #14724

Merged
merged 1 commit into from Mar 16, 2021

Conversation

sunkickr
Copy link
Contributor

@sunkickr sunkickr commented Mar 11, 2021

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 Add custom Salesforce connection type #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:
image

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 #14631 (comment) where something went wrong during rebase and the PR closed.

@boring-cyborg boring-cyborg bot added area:dev-tools provider:snowflake Issues related to Snowflake provider labels Mar 11, 2021
@github-actions
Copy link

The Workflow run 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*.

@sunkickr
Copy link
Contributor Author

@kaxil thank you!

@sunkickr
Copy link
Contributor Author

The run associated with this PR appears to have failed because of secuity issue?
Running job on this worker disallowed by security policy

@kaxil
Copy link
Member

kaxil commented Mar 12, 2021

Just re-started the test, let's see

@kaxil
Copy link
Member

kaxil commented Mar 12, 2021

Just re-started the test, let's see.

GH is experiencing issues at the moment: https://www.githubstatus.com/

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@sunkickr
Copy link
Contributor Author

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

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! As discussed it may be worth to decouple aws from sf but that's definitely out of scope here 👌

@turbaszek
Copy link
Member

@sunkickr can you please rebase onto current master to rerun all builds?

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 15, 2021
@github-actions
Copy link

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.

@sunkickr
Copy link
Contributor Author

@sunkickr can you please rebase onto current master to rerun all builds?

Thank you for the review! I can do that

@sunkickr sunkickr force-pushed the snowflake-connection-fields branch from 771a7e8 to 57b913d Compare March 15, 2021 19:41
add dynamic fields to snowflake connection

add dynamic fields to snowflake connection

add dynamic fields to snowflake connection
@sunkickr sunkickr force-pushed the snowflake-connection-fields branch from a102fd5 to 3b76592 Compare March 16, 2021 12:51
@kaxil kaxil merged commit e4bf8f3 into apache:master Mar 16, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 16, 2021

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge provider:snowflake Issues related to Snowflake provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants