-
Notifications
You must be signed in to change notification settings - Fork 117
Use Snowflake config.toml/connection.toml files #975
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
base: main
Are you sure you want to change the base?
Conversation
Add support for loading connection parameters from TOML config files (~/.snowflake/connections.toml and config.toml) following the Python Snowflake connector conventions. New features: - `connection_name` parameter to load a named connection profile - `connections_file_path` parameter to specify custom config location - Support for SNOWFLAKE_CONNECTIONS env var (full TOML override) - Support for SNOWFLAKE_DEFAULT_CONNECTION_NAME env var - File permission security checks (error on writable, warn on readable) - Graceful fallback when toml package not installed Connection resolution: - Case 1: connection_name provided → load and merge with kwargs - Case 2: No args → load default connection from config - Case 3: Programmatic params → use params only, skip config loading SNOWFLAKE_ACCOUNT env var is only used in Case 3 (programmatic mode), not when loading from config files. Requires the toml package (Suggests) for config file parsing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Two issues fixed: 1. When config file had `user` + `authenticator` (no password), the `uid` and `authenticator` values ended up in both `args` and `auth`, causing "formal argument matched by multiple actual arguments" error. Fixed by nulling auth params from `args` before merging with `auth`. 2. `snowflake_auth_args()` wasn't returning `authenticator` when using uid + authenticator (externalbrowser/SNOWFLAKE_JWT), so the authenticator from config file was lost and the "Failed to detect ambient credentials" error was thrown. Added tests for config files with user + authenticator (no password). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
| if (!is.null(database)) programmatic_params$database <- database | ||
| if (!is.null(schema)) programmatic_params$schema <- schema | ||
| if (!is.null(uid)) programmatic_params$uid <- uid | ||
| if (!is.null(pwd)) programmatic_params$pwd <- pwd |
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.
We interpret NULL as "not provided", which is a little different than how the Snowflake Connector for Python interprets None. In Python, you can pass None to un-set a parameter that's set in connections.toml; in R, this will have no effect.
If we care about this, we could:
- Change the
is.nullcheck to amissing()check, and remove= NULLfrom the signature. Downside is it makes it look like those parameters are required. Perhaps if we put them after...that will be a hint that they're not. - Continue to treat
NULLas missing but haveNAmean unset. This doesn't feel very idiomatic.
I have to admit it was slightly difficult for me to think of a case where this behavior is desirable or a user would be surprised that a NULL value is ignored, so I just left it.
Two issues fixed: 1. SNOWFLAKE_CONNECTIONS env var was preventing reading of default_connection_name from config.toml. Now each config option is resolved independently, matching Python connector behavior. 2. Missing toml package gave misleading "connection not found" error instead of "toml package required" when config files existed. Now errors appropriately when TOML parsing is actually needed. Implementation uses delayedAssign() for lazy promise-based evaluation: - Only parses TOML sources that are actually needed - toml package error only triggers when parsing is required - Clean precedence logic via %||% chains, no complex conditionals 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
simonpcouch
left a comment
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 have strong reflexes here on the big idea, but trust that this is a reasonable approach! Reviewing for R code style etc.
Could you add a NEWS entry?
| connection_name = NULL, | ||
| connections_file_path = NULL, | ||
| account = NULL, |
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 believe this would break passing account by position. Could we move the new arguments to after pwd (or the ellipses, if you think those should always be named)?
| #' @return NULL invisibly; throws error or warning on bad permissions | ||
| #' @noRd | ||
| check_toml_file_permissions <- function(file_path) { | ||
| # Skip on Windows |
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.
| # Skip on Windows |
An example "what" comment
| sprintf( | ||
| "Invalid connection_name '%s', known ones are [%s]", | ||
| connection_name, | ||
| paste0("'", known_names, "'", collapse = ", ") | ||
| ), |
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.
Could we use cli::cli_abort() with variable substitutions here? Fine with occasional sprintf()s, but we should lean on that dependency for ad-hoc paste0()s. This prompt should be enough for Claude to get all the way there.
|
|
||
| #' Check if toml package is installed | ||
| #' @noRd | ||
| check_toml_installed <- function() { |
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.
We @import rlang, so you should be able to use check_installed a la
check_installed("toml", "to load Snowflake configuration files")
| expect_equal(config$default_connection_name, "staging") | ||
| }) | ||
|
|
||
| test_that("Case 1: Named connection loads and merges with kwargs", { |
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.
| test_that("Case 1: Named connection loads and merges with kwargs", { | |
| test_that("Named connection loads and merges with kwargs", { |
Similar feeling on the enumeration here
| config_dir <- tempfile() | ||
| dir.create(config_dir) | ||
| withr::defer(unlink(config_dir, recursive = TRUE)) |
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.
| config_dir <- tempfile() | |
| dir.create(config_dir) | |
| withr::defer(unlink(config_dir, recursive = TRUE)) | |
| config_dir <- withr::local_tempdir() |
And elsewhere in this test file
|
Fine to ignore the two failing checks. |
Vibe coded; just for discussion purposes for nowExtremely carefully reviewed by @jcheng5, except unit tests (life is too short)Add support for loading connection parameters from TOML config files (~/.snowflake/connections.toml and config.toml) following the Python Snowflake connector conventions.
New features:
connection_nameparameter to load a named connection profileconnections_file_pathparameter to specify custom config locationConnection resolution:
SNOWFLAKE_ACCOUNT env var is only used in Case 3 (programmatic mode), not when loading from config files.
Requires the toml package (Suggests) for config file parsing.
🤖 Generated with Claude Code