Get temporary credentials from credentials file #88
Conversation
| @@ -61,8 +61,8 @@ func TestNewSession_AssumeRole(t *testing.T) { | |||
| settings := AWSDatasourceSettings{ | |||
| AssumeRoleARN: roleARN, | |||
| } | |||
| os.Setenv(AllowedAuthProvidersEnvVarKeyName, "default") | |||
| os.Setenv(AssumeRoleEnabledEnvVarKeyName, "true") | |||
| require.NoError(t, os.Setenv(AllowedAuthProvidersEnvVarKeyName, "default")) | |||
There was a problem hiding this comment.
just curious what this does, is it checking that setenv doesn't error out? seems maybe overly cautious no? Or is there an expected error that could happen?
There was a problem hiding this comment.
I think it's unlikely an error will happen, but it would be nice to know if it does instead of silently continuing in any case.
|
|
||
| assert.Equal(t, "grafanamagic", *p.ExternalID) | ||
| return credentials.NewCredentials(p) | ||
| } |
There was a problem hiding this comment.
hey i think i must be missing something obvious but when I read through this test I don't really see where we use newSTSCredentials. It seems like we make a function, but then we never call that function. I see we do this pattern in other tests so I'm assuming I'm missing something? Like does it overwrite something being used in getSession somehow?
There was a problem hiding this comment.
Yup it overwrites what's being used here.
The overwriting in the test just happens here and it's cleaned up here.
(For what it's worth: newSTSCredentials is a global variable, it was stubbed, and it effectively hides what GetSession is depending on. We could instead use dependency injection (i.e. The dependency is directly attached to the thing that depends on it. The method receiver could contain the dependency OR the signature of this function could either pass in the dependency as an argument.). In my opinion stubbing has made both the production code and test code confusing to read/understand and not explicit -- (sorry but) I think it's a bit of an anti-pattern... )
There was a problem hiding this comment.
Just to add, @fridgepoet has already taken a look at if/how we could replace the stubs in this branch and we might start replacing the tests soon. But we agreed it's probably not worth it in this PR
There was a problem hiding this comment.
(not sure if it's injected in the right places or if things are "proper" and didn't test manually to see if it all works)
Sets up to use shared credentials file for grafana_assume_role, as agreed, plus adding tests (thanks @fridgepoet for the help!) 馃檹馃徎