-
Notifications
You must be signed in to change notification settings - Fork 23
Create GetSessionWithAuthSettings #144
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
Conversation
pkg/awsds/sessions.go
Outdated
} | ||
|
||
// When calling, be sure that the context passed is the datasource instance's context which contains auth settings | ||
func (sc *SessionCache) GetSessionWithContext(ctx context.Context, c GetSessionWithContextConfig) (*session.Session, error) { |
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.
I know that I'm the one that suggested this approach, but if we need to use the instance context every time we get a session, it may be better to make this something like GetSessionWithAuthSettings(authSettings AuthSettings, c SessionConfig)
and remove the AuthSettings from the SessionConfig to make it a clearer separation (cloudwatch is the only one using SessionConfig.AuthSettings
so it's fine). This implementation is going to end up requiring us to store the context, which is discouraged in Go, while if we take the AuthSettings
as an argument we can store those on the instance instead.
@njvrzm do you have any ideas/preferences for how to organize this?
Ok updated as you suggested @iwysiu so that auth settings are now explicitly required to be passed in (with instructions on how to do so), rather than read from the env (or pulled from a stored context) |
return credentials.NewCredentials(defaults.RemoteCredProvider(*sess.Config, sess.Handlers)) | ||
} | ||
|
||
type GetSessionConfig struct { |
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.
We could probably just use SessionConfig for GetSessionWithAuthSettings
but this works too!
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.
yeah i guess my thinking is that sessionConfig has authsettings in there which seems confusing to have a property in there we never would fill out idk.
When working on grafana/timestream-datasource#285 we noticed that in order to not rely on env variables we have to grab auth settings from the datasource instance's context and pass them along to GetSession.
However this is easy to forget/miss. This pr will create a new method for getting session which is passed in the datasource instance's context with auth settings and sets it as appropriate.