-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added Databricks CLI & Azure CLI authentication and a basic integration test #37
base: main
Are you sure you want to change the base?
Conversation
if (!is_azure()) { | ||
return(NULL) | ||
} | ||
token_source <- .azure_cli_token_source("2ff814a6-3304-4ab8-85cb-cd0e6f879c1d") |
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.
should this be removed?
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.
Nope. It holds private state used for token refreshing internally when it's about to expire
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.
Need to fix is_<cloud>
functions to handle NULL
:
databricks-sdk-r/R/api_client.R
Lines 123 to 135 in bb96aac
is_azure <- function() { | |
grepl(".azuredatabricks.net", cfg$host) | |
} | |
# is_gcp returns true if client is configured for GCP | |
is_gcp <- function() { | |
grepl(".gcp.databricks.com", cfg$host) | |
} | |
# is_aws returns true if client is configured for AWS | |
is_aws <- function() { | |
!is_azure() & !is_gcp() | |
} |
e.g.
is_azure <- function() {
if (!is.null(cfg$host)) {
grepl(".azuredatabricks.net", cfg$host)
} else {
FALSE
}
}
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.
That's a bit irrelevant to this PR, can you please send one?
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.
also, in this case, we won't have null there, per se
No description provided.