Improve the internal consistency of credential handling #282
Conversation
There was a problem hiding this comment.
Just to confirm that I understand what's going on, there's currently no cache invalidation if the request fails due to the token expiring, right? It's only invalidated if the token is out of date (based on the expiration date given when it was first received)?
| @@ -59,11 +59,16 @@ chat_databricks <- function(workspace = databricks_workspace(), | |||
| model <- set_default(model, "databricks-dbrx-instruct") | |||
| turns <- normalize_turns(turns, system_prompt) | |||
| echo <- check_echo(echo) | |||
| if (!is.null(token)) { | |||
| credentials <- function() list(Authorization = paste("Bearer", token)) | |||
There was a problem hiding this comment.
Do you want to allow manual credentials here too?
There was a problem hiding this comment.
No, I really want to encourage folks to use authentication setups that are compatible with other Databricks tooling, which use environment variables. If anyone starts asking for an "escape hatch", though we could consider it then.
|
If we wanted to make this easier to do in httr2, I think we'd just need to export (This might already be obvious to you, but it's been a while since I looked at this code) |
This commit adopts the pattern used in `chat_azure()` to `chat_databricks()` and `chat_snowflake()`. That is: these providers now have a single `credentials` property that returns a list of headers when called. In addition, there is now consistent internal usage of a `default_x_credentials()` function that itself returns a `credentials` function. (Through experimenting with various designs, I decided that a simple function factory approach was preferable to the heavyweight S7 classes and generics approach I thought we could use initially.) Finally, I've added several new unit tests for Databricks and Snowflake authentication. Closes #262. Signed-off-by: Aaron Jacobs <[email protected]>
|
I think if we did that, I could rewrite these |
atheriel
force-pushed
the
internal-credentials-consistency
branch
from
9188021 to
82cd70c
Compare
|
I realize that maybe I should use that pattern anyway so that we handle the OAuth token flows more correctly. Right now we get |
|
My current feeling is that figuring out what to do about |
|
Yeah, agreed. Feel free to merge. |
|
Hmmm, maybe the |
This commit adopts the pattern used in
chat_azure()tochat_databricks()andchat_snowflake(). That is: these providers now have a singlecredentialsproperty that returns a list of headers when called.In addition, there is now consistent internal usage of a
default_x_credentials()function that itself returns acredentialsfunction.(Through experimenting with various designs, I decided that a simple function factory approach was preferable to the heavyweight S7 classes and generics approach I thought we could use initially.)
Finally, I've added several new unit tests for Databricks and Snowflake authentication.
Closes #262.