-
Notifications
You must be signed in to change notification settings - Fork 175
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 snowflake to application parameter to configuration #1266
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- did we pick a name for our application?
- I think we should mention that we send it in our docs for snowflake...
@@ -49,6 +49,9 @@ def _read_private_key(private_key: str, password: Optional[str] = None) -> bytes | |||
) | |||
|
|||
|
|||
snowflake_application_id = "dltHub_dlt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the application id we picked for dlt
? @VioletM ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make the var uppercase and import it into the test so we don't have any magic strings there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -39,11 +39,18 @@ username = "loader" | |||
host = "kgiotue-wn98412" | |||
warehouse = "COMPUTE_WH" | |||
role = "DLT_LOADER_ROLE" | |||
application = "dltHub_dlt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should put it here, because for most users it's not relevant and it just will be a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it I will remove this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise a small comment LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! but now tests must pass
To allow snowflake to track users using dlt we need to pass application parameter as query parameter.
If the value is set toskip
we do not include it (some users might want to disable). @rudolfix wdyt?Will use
""
as a way to skip the addition ofapplication
parameter.Config example
Resolves: #1257