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

Outsource Snowflake authentication for Cortex to snowflakeauth #145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atheriel
Copy link
Contributor

@atheriel atheriel commented Nov 6, 2024

This is a draft because I'm not actually sure this is a good idea yet and the API of snowflakeauth is still WIP.

This commit drops all of the Snowflake-specific authentication logic in favour of using the new snowflakeauth package. This has a big advantage in that it works with Snowflake connections and credentials created by the Snowflake Python packages and their CLI.

Calling chat_cortex() with no credentials makes our support for these files explicit:

Error in `chat_cortex()`:
! The `account` argument is required when ~/.config/snowflake/connections.toml is missing or empty.
ℹ Pass `account` or define a [default] section with an account field in ~/.config/snowflake/connections.toml.

However, this is a breaking change in that (1) some environment variables that previously furnished credentials no longer do; and (2) the signature of chat_cortex() has changed. The reason to drop the environment variable support is that these specific environment variables are not used by Snowflake's Python tooling -- so I think this is the right choice.

Existing unit tests do cover regressions in the Authorization headers we pass to Snowflake, though I did have to modify them a bit to account for the API changes.

This commit drops all of the Snowflake-specific authentication logic in
favour of using the new `snowflakeauth` package. This has a big
advantage in that it works with Snowflake connections and credentials
created by the Snowflake Python packages and their CLI.

However, this is a breaking change in that (1) some environment
variables that previously furnished credentials no longer do; and (2)
the signature of `chat_cortex()` has changed. The reason to drop the
environment variable support is that these specific environment
variables are not used by Snowflake's Python tooling -- so I think this
is the right choice.

Existing unit tests do cover regressions in the Authorization headers we
pass to Snowflake, though I did have to modify them a bit to account for
the API changes.

Signed-off-by: Aaron Jacobs <[email protected]>
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.

1 participant