Add ReadSettings to get config settings from context #118
Conversation
pkg/awsds/sessions.go
Outdated
| defaultListMetricsPageLimit = 500 | ||
| ) | ||
|
|
||
| // ReadAuthSettingsFromEnvironmentVariables is deprecated in favor of |
pkg/awsds/sessionSettings.go
Outdated
| func ReadSettings(ctx context.Context) *AuthSettings { | ||
| settings, exists := ReadSettingsFromContext(ctx) | ||
| if !exists { | ||
| settings = ReadAuthSettingsFromEnvironmentVariables() |
There was a problem hiding this comment.
probably silly question but are there circumstances where there are settings that we'd want to read from context and from env variables? probably not???
Also Settings feels so vague to me, are there specific settings we're talking about here? I guess it's auth configuration settings from custom.ini? maybe note that in the comment above?
There was a problem hiding this comment.
No, the context and env should be the same settings (except for session duration, which I noted below). Changed to ReadAuthSettings if that's clearer? Though technically its not just auth settings, its just whatever the grafana settings are (limitMetricsPages is not auth related).
pkg/awsds/sessionSettings.go
Outdated
| if cfg == nil { | ||
| return settings, false | ||
| } | ||
| hasSettings := false |
There was a problem hiding this comment.
The azure implementation set the precedent that if the GrafanaConfig exists but doesn't set anything, it counts as not having settings, but I'm not sure if that's actually set in stone
pkg/awsds/sessionSettings.go
Outdated
| } | ||
|
|
||
| // Users set session duration directly as an environment variable | ||
| sessionDurationString := os.Getenv(SessionDurationEnvVarKeyName) |
There was a problem hiding this comment.
We can make a follow up issue to add this to the config file, but I figured that this was a big enough and time sensitive enough change without it. (this option was added in #58 but not to the config)
There was a problem hiding this comment.
added an issue for it here: grafana/grafana#81414
| @@ -15,11 +15,16 @@ import ( | |||
| // AmazonSessionProvider will return a session (perhaps cached) for given region and settings | |||
| type AmazonSessionProvider func(region string, s AWSDatasourceSettings) (*session.Session, error) | |||
|
|
|||
| // AuthSettings defines whether certain auth types and provider can be used or not | |||
| // AuthSettings stores the AWS settings from Grafana | |||
There was a problem hiding this comment.
Technically these aren't really just auth settings anymore, but for simplicity and non-breaking changes I put the additional settings here.
pkg/awsds/sessionSettings.go
Outdated
| func ReadSettings(ctx context.Context) *AuthSettings { | ||
| settings, exists := ReadSettingsFromContext(ctx) | ||
| if !exists { | ||
| settings = ReadAuthSettingsFromEnvironmentVariables() |
There was a problem hiding this comment.
No, the context and env should be the same settings (except for session duration, which I noted below). Changed to ReadAuthSettings if that's clearer? Though technically its not just auth settings, its just whatever the grafana settings are (limitMetricsPages is not auth related).
pkg/awsds/sessions.go
Outdated
| defaultListMetricsPageLimit = 500 | ||
| ) | ||
|
|
||
| // ReadAuthSettingsFromEnvironmentVariables is deprecated in favor of |
| } else { | ||
| settings.SessionDuration = &sessionDuration | ||
| } | ||
| } |
There was a problem hiding this comment.
Totally optional, but while you're in here, you could pull in the default value for SessionDuration out of sessions.go:
grafana-aws-sdk/pkg/awsds/sessions.go
Lines 259 to 262 in 68cd8f7
so that all the default values are set in the same place.
There was a problem hiding this comment.
Done in the default settings function
pkg/awsds/sessions.go
Outdated
| ) | ||
|
|
||
| // ReadAuthSettingsFromEnvironmentVariables gets the Grafana auth settings from the environment variables | ||
| // Deprecated: Use ReadAuthSettingsFromContext instead | ||
| func ReadAuthSettingsFromEnvironmentVariables() *AuthSettings { |
There was a problem hiding this comment.
It's a little funny that ReadAuthSettingsFromEnvironmentVariables is in sessions.go but ReadAuthSettingsFromConfig is in authSettings.go now. Certainly don't need to move it, especially since it's deprecated, but it's nice to tidy up :)
There was a problem hiding this comment.
done! moved this and the consts
part of grafana/grafana#79324
The grafana part of the work is at grafana/grafana#81208
As part of the decoupling work, we need to have a way to get the grafana config off the context, so that CloudWatch plugin can use that instead of the
grafana/package/settingpackage. This pr adds a helper to fetch the grafana config settings from the context. Previously, external plugins used a workaround where grafana would set the config values in the environment variables and the plugins would get the config values from those plugins. I'm adding this instead of using that existing env variable function because grafana wants to move away from setting configs in the env variables (for multitenancy reasons).(for context, heavily inspired by this commit which does the same work for azure)