Issue 631/Support for STS regional endpoint config #636
Conversation
…ed hard-coded region. Need to actually use sts regional endpoint config somehow to set the sts endpoint
joakibo
changed the title
| @@ -311,7 +311,7 @@ get_creds_from_sts_resp <- function(resp) { | |||
| # `mfa_serial`, and the user will be prompted interactively to provide the | |||
| # current MFA token code. | |||
| get_assumed_role_creds <- function(role_arn, role_session_name, mfa_serial, creds) { | |||
| svc <- sts(config = list(credentials = list(creds = creds), region = "us-east-1")) | |||
| svc <- sts(config = list(credentials = list(creds = creds))) | |||
There was a problem hiding this comment.
Possibly need something more here, in case users are setting region in the client configs, then how to get it in here. It's not good to hardcode at least, so need a different solution.
|
|
||
| .get_sts_endpoints <- function(profile = "") { | ||
|
|
||
| sts_regional_endpoint <- get_sts_regional_endpoint(profile) |
There was a problem hiding this comment.
Will this function from client.R be available in this context?
| @@ -102,6 +102,7 @@ NULL | |||
| #' @rdname sts | |||
| #' @export | |||
| sts <- function(config = list()) { | |||
| .sts$metadata$endpoints <- .get_sts_endpoints(config$profile) | |||
There was a problem hiding this comment.
This felt a little dirty, but since endpoints is dependent on profile this seemed like the natural way to do it.
|
I think I am understanding this issue a little better, I believe we need to create |
|
Sounds good, thanks. Having it in this custom script makes sense. I don’t have a good understanding of the code base, easier for you to deduce where things should go 👍 Changes outlined here should at least be sufficient as I see it. |
|
@joakibo I believe we are in a good place with this. Will need to create some unit tests to fully test out the functionality. But I think it is nearly done. Please feel free to test it out and let me know |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
==========================================
- Coverage 84.07% 83.73% -0.34%
==========================================
Files 63 63
Lines 4214 4305 +91
==========================================
+ Hits 3543 3605 +62
- Misses 671 700 +29 ☔ View full report in Codecov by Sentry. |
|
Thanks a lot @DyfanJones, looking forward to apply this fix on our side. Sorry for not being able to test this and assist more. |
Attempts to fix #631 . No testing done, needs input from R experts on the general strategy and placement of code.