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

Enhancements to service construction? #421

Closed
hadley opened this issue May 19, 2021 · 6 comments
Closed

Enhancements to service construction? #421

hadley opened this issue May 19, 2021 · 6 comments
Labels
enhancement 💡 New feature or request

Comments

@hadley
Copy link

hadley commented May 19, 2021

Instead of

svc <- acm(
  config = list(
    credentials = list(
      creds = list(
        access_key_id = "string",
        secret_access_key = "string",
        session_token = "string"
      ),
      profile = "string"
    ),
    endpoint = "string",
    region = "string"
  )
)

Could we have this?

svc <- acm(
  credentials = list(
    creds = list(
      access_key_id = "string",
      secret_access_key = "string",
      session_token = "string"
    ),
    profile = "string"
  ),
  endpoint = "string",
  region = "string"
)

This makes life easier because auto-complete works, and you could document the individual parameters with a little more detail. You could make the change backward compatible by continuing to provide the config parameter and then combining it with the list generated by the other parameters.

It would also be useful if NULL values were automatically dropped, so that if you're wrapping in a function, you can default those arguments to NULL in your function.

@davidkretch
Copy link
Member

davidkretch commented May 20, 2021

Yeah, that sounds good. I think that should be pretty straightforward.

@davidkretch davidkretch added the enhancement 💡 New feature or request label Dec 4, 2021
@DyfanJones
Copy link
Member

DyfanJones commented Jun 28, 2023

@hadley @davidkretch sorry for taking so long to address this issue.

Original

svc <- acm(
  config = list(
    credentials = list(
      creds = list(
        access_key_id = "string",
        secret_access_key = "string",
        session_token = "string"
      ),
      profile = "string"
    ),
    endpoint = "string",
    region = "string"
  )
)

Proposal 1

As suggested by @hadley

svc <- acm(
  credentials = list(
    creds = list(
      access_key_id = "string",
      secret_access_key = "string",
      session_token = "string"
    ),
    profile = "string"
  ),
  endpoint = "string",
  region = "string"
)

My concerns with this approach would be:

However we could get around this by keeping the config parameter plus it would prevent breaking dependent packages.

# modified to keep the config parameter
svc <- acm(
  credentials = list(
    creds = list(
      access_key_id = "string",
      secret_access_key = "string",
      session_token = "string"
    ),
    profile = "string"
  ),
  endpoint = "string",
  region = "string",
  config = list()
)

Proposal 2

svc <- acm(config = list(), ...)

This allows the config parameters to be called at "root" for example:

svc <- acm(
  access_key_id = "string",
  secret_access_key = "string",
  session_token = "string",
  profile = "string",
  endpoint = "string",
  region = "string"
)

The down side to this it doesn't address the auto-complete functionality, but it does make initialising the service a little easier. I have a PR #639 toying with this idea.

What's everyone view on this?

@hadley
Copy link
Author

hadley commented Jun 29, 2023

I think you should keep the config argument, and just combine with credentials, endpoint, etc. I don't think there's much benefit to using ... in this way, as it's hard to document.

DyfanJones added a commit to DyfanJones/paws that referenced this issue Jun 29, 2023
@DyfanJones
Copy link
Member

Ok the only issue we have now is credentials is a list parameter and is doesn't support auto-compelete. After chatting with @davidkretch paws.common has a nifty class called struct that does help with auto-complete

get_service_parameter <- function(param) {
  firstup <- function(x) {
    substr(x, 1, 1) <- toupper(substr(x, 1, 1))
    x
  }
  fn <- get(param, envir = getNamespace("paws.common"))
  elements <- fn()
  elements <- elements[grep("provider", names(elements), perl = T, invert = T)]
  
  for (nms in names(elements)) {
    if (is.list(elements[[nms]])) {
      elements[[nms]] <- get_service_parameter(firstup(nms))
    }
  }
  return(elements)
}

build_service_parameter <- function(param) {
  return(
    do.call(
      struct,
      get_service_parameter(param)
    )
  )
}

SetConfig <- build_service_parameter("Config")
SetCredentials <- build_service_parameter("Credentials")
SetCreds <- build_service_parameter("Creds")

Screenshot 2023-06-30 at 15 57 06

It will also allow for mix lists and structs together

Screenshot 2023-06-30 at 16 05 03

Also it is handy when they are nested together.

Screenshot 2023-06-30 at 16 07 54

I believe these could be possible helper functions.

What is everyones thoughts on this?

@DyfanJones
Copy link
Member

DyfanJones commented Jul 3, 2023

Update of progress:

The helper functions will snake case to be consistent with the rest of paws exported functions and methods.

Short term the helper functions can be accessed by loading paws.common into session.

library(paws.common)
s3 <- paws::s3(config(credentials(profile = "paws")))
s3$list_buckets()

Long term we would want to re-export the helper functions to all paws service packages so that paws.common doesn't need to loaded explicitly. Plus in combination with the extra service parameters.

s3 <- paws::s3(config(credentials(profile = "paws")))
s3$list_buckets()

# or

s3 <- paws::s3(credentials = credentials(profile = "paws"))
s3$list_buckets()

# or

s3 <- paws::s3(credentials = list(profile = "paws"))
s3$list_buckets()

@DyfanJones
Copy link
Member

paws v-0.4.0 has now been released to the cran. I will close this ticket for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 💡 New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants