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

snowflake task plus tests #1343

Merged
merged 3 commits into from
Aug 11, 2019

Conversation

akravetz
Copy link

@akravetz akravetz commented Aug 10, 2019

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

This PR adds a SnowflakeQuery Task, which supports parameterized queries to a snowflake data warehouse. It also adds appropriate tests (no substantial decrease in project test coverage) and updates the changelog.

Why is this PR important?

Snowflake is a commonly used data warehouse provider.

@marvin-robot
Copy link
Member

Here I am, brain the size of a planet and they ask me to welcome you to Prefect.

So, welcome to the community @akravetz! 🎉 🎉

@akravetz akravetz mentioned this pull request Aug 10, 2019
@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #1343 into master will decrease coverage by 0.05%.
The diff coverage is 93.75%.

@jlowin
Copy link
Member

jlowin commented Aug 11, 2019

This is great to see! Thanks, @akravetz!

Could you please add yourself to the contributors section of the changelog, and also add your new tasks to the documentation? You'll need to update outline.toml for the API reference and also add a short page to the task_library section. It may be helpful to mimic the relevant sections for Redis.

):
self.account = account
self.user = user
self.password = password
Copy link
Member

Choose a reason for hiding this comment

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

You may want to replace this explicit password with a Prefect secret, so that users don't have to hardcode the value into the task. Here is how that's handled in a Dropbox task -- users supply the name of the secret (either in __init__ or as a run-time argument) and the Secret by that name is accessed when the task runs.

Copy link
Author

@akravetz akravetz Aug 11, 2019

Choose a reason for hiding this comment

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

Comment & a question.

Comment: Makes total sense. If it's alright with you, I'd take the pattern from S3Download which mimics how vault stores secrets (ie 1 secret maps to all necessary information for login: username, password, and account).

Question: Is the plan to require use of Prefect's secret manager? My initial thought here was to use the pattern in the postgres task, where the user passes raw credentials. That way users can leverage whatever secret management infrastructure they have in place (be it Vault, AWS Secret Manager, Prefect's secret manager, etc).

If the idea is that users can leverage whatever secret management infrastructure they have, I'd prefer to leave as is in order to decouple secret management from prefect tasks.

Copy link
Member

Choose a reason for hiding this comment

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

That's an excellent question. The intent is definitely NOT to require Prefect's secret manager.

However this does raise an interesting question for discussion @cicdw -- we don't want to encourage users to pass secrets from task to task, because there's always a possibility of the secret becoming cached as a task result. To @akravetz's point, we definitely also don't want to require Prefect secrets. Perhaps there's a way for us to make Prefect secrets more pluggable, so that we could use a Prefect secret to deliver the value at runtime but the actual retrieval of that secret could be any preferred infrastructure?

In light of that let's not hold up this great PR for this!

Copy link
Member

Choose a reason for hiding this comment

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

Yea - that's an interesting point! My gut reaction is that we can encourage people to subclass Secret and override the get method - this seems like the simplest approach. To do so, we might introduce a secrets base class that we also inherit from for Prefect Secrets. Let's open an issue for this to discuss further.

Also, as an aside I didn't catch that the postgres tasks used plain passwords and have opened an issue to update that.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

This is great - thank you @akravetz !

I'm happy to defer the password situation for a later refactor if you'd prefer.

@cicdw cicdw merged commit 62d70a8 into PrefectHQ:master Aug 11, 2019
abrookins pushed a commit that referenced this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants