zacdav-db / delta-sharing-r

R Delta Sharing Client
5 stars 1 forks source link

Feature request: add option to load DeltaSharing credentials/token from a list instead of a file #9

Closed januz closed 1 year ago

januz commented 1 year ago

Hey @zacdav-db, I was wondering whether you would be willing to implement an alternative way to load the DeltaSharing credentials.

Currently, the sharing_client() function expects a JSON file (typically config.share). While that works well locally, it doesn't work as well when running jobs through GithubActions where I didn't want to check in the file into version control itself but instead work with Github Sectrets / environment variables.

I worked around it by writing a wrapper function that creates the JSON from the Secrets and saves it as a temporary file that is read in using sharing_client() (see below) but I was wondering whether you could just add another parameter to the sharing_client() function, e.g., credentials_list (and rename the current parameter credentials_file), that would allow to pass it a list with the credentials (i.e., skips the jsonlite::read_json() step and just provides its output object creds directly.

Thanks for your consideration!

Here my wrapper function for reference:

create_deltasharing_client <- function() {
  temp_file <- "config-temp.share"

  list(
    shareCredentialsVersion = 1,
    bearerToken             = Sys.getenv("TOKEN_DELTASHARING"),
    endpoint                = Sys.getenv("URL_DELTASHARING"),
    expirationTime          = "2024-09-20T22:16:14.933Z"
  ) |> 
    jsonlite::toJSON(auto_unbox = TRUE) |> 
    writeLines(temp_file)

  client <- delta.sharing::sharing_client(temp_file)

  file.remove(temp_file)

  client
}

P.S. Are you still planning to release a new version that will enable the CDF functionality?

zacdav-db commented 1 year ago

Hey @januz, sorry for slow reply - been unwell.

Yeah that's a good point - I should add support for environment variables. I also was going to support reading multiple sharing credentials at once potentially - so might work on those together.

Yes still working on new version but unfortunately delayed due to being unwell.

januz commented 1 year ago

Hi @zacdav-db, thanks so much for getting back to me and I'm so sorry to hear that you aren't doing well! Thanks for considering my feature request and all the best to you.

zacdav-db commented 1 year ago

@januz I've added this functionality in latest PR (https://github.com/zacdav-db/delta-sharing-r/pull/10).

The change:

You can either opt to create your own DeltaShareCredentials (it's essentially a list) or use the new helper to build this from env vars.

januz commented 1 year ago

@zacdav-db Fantastic, thank you so much!