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 Snowflake exporter RSA key-pair authentication #890

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Caleb-Hurshman
Copy link

PR Description

This PR updates the wrapped Snowflake exporter to match new config options to enable RSA key-pair authentication.

Which issue(s) this PR fixes

#10493

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

Copy link
Contributor

@StefanKurek StefanKurek left a comment

Choose a reason for hiding this comment

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

I think probably ok as is, with a couple of comments to make minor improvements IMO

CHANGELOG.md Show resolved Hide resolved
@Caleb-Hurshman Caleb-Hurshman marked this pull request as ready for review May 22, 2024 13:37
| Name | Type | Description | Default | Required |
| ---------------------- | -------- | -------------------------------------------------------------------------------------------- | ---------------- | -------- |
| `account_name` | `string` | The account to collect metrics for. | | yes |
| `username` | `string` | The username for the user used when querying metrics. | | yes |
Copy link
Contributor

Choose a reason for hiding this comment

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

username, password, private_key_path and private_key_password should all be optional from not on, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, we should add some documentation explaining how to use these variables. Like, one or the other, precedence order, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed on the first point, but there are scenarios where these are required, so some thought this qualified as 'required'. I had an earlier version with more details outside of the table, but opted to add to the field descriptions instead. I'll add these comments back in, let me know what you think.

Comment on lines 29 to 33
account_name = ACCOUNT_NAME
username = USERNAME
private_key_path = /PATH/TO/rsa_key.p8
private_key_password = PASSWORD
warehouse = WAREHOUSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
account_name = ACCOUNT_NAME
username = USERNAME
private_key_path = /PATH/TO/rsa_key.p8
private_key_password = PASSWORD
warehouse = WAREHOUSE
account_name = <ACCOUNT_NAME>
username = <USERNAME>
private_key_path = <PATH_TO_rsa_key.p8>
private_key_password = <PASSWORD>
warehouse = <WAREHOUSE>

For consistency with the direction the docs are taking, we should use the placeholder variable notation defined here: https://grafana.com/docs/writers-toolkit/write/style-guide/write-for-developers/#placeholder-variables

Example with this style implemented: https://grafana.com/docs/alloy/latest/reference/components/discovery.azure/#example

This is still a work in progress... and many topics still need to be update.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label May 23, 2024
Copy link
Contributor

@gaantunes gaantunes left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants