Refactor ValidateConfig of all receivers to accept only what is needed #61
Conversation
| @@ -42,6 +44,10 @@ type FactoryConfig struct { | |||
| Logger logging.Logger | |||
| } | |||
|
|
|||
| func (fc *FactoryConfig) Decrypt(key, fallback string) string { | |||
| return fc.DecryptFunc(context.Background(), fc.Config.SecureSettings, key, fallback) | |||
| } | |||
There was a problem hiding this comment.
This is a wrapper to hide secure settings and context from notifier's config builder
There was a problem hiding this comment.
Figured it out - this is a nice change - not sure if that context.Background() might cause us more hard than good in the end but let's roll with it for now.
There was a problem hiding this comment.
Why?
To hide implementation details of secrets storage from config parser, and reduce the scope for functions NewConfig. The context will be enclosed upstream (in Grafana) where it makes sense because it provides a secret service that requires context.
77f76d4 to
5bde521
Compare
5bde521 to
3825c1f
Compare
| func NewFactoryConfigForValidateConfigTesting(t *testing.T, m *receivers.NotificationChannelConfig) (receivers.FactoryConfig, error) { | ||
| tmpl := templates.ForTests(t) | ||
| tmpl.ExternalURL = ParseURLUnsafe("http://localhost") | ||
| return receivers.NewFactoryConfig(m, nil, DecryptForTesting, tmpl, &images.UnavailableImageStore{}, func(ctx ...interface{}) logging.Logger { | ||
| return &logging.FakeLogger{} | ||
| }, "1.2.3") | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm assuming that all this setup (the templates, the logger, etc) was never used in the places where we setup this configuration?
There was a problem hiding this comment.
Pretty much, yes
| @@ -42,6 +44,10 @@ type FactoryConfig struct { | |||
| Logger logging.Logger | |||
| } | |||
|
|
|||
| func (fc *FactoryConfig) Decrypt(key, fallback string) string { | |||
| return fc.DecryptFunc(context.Background(), fc.Config.SecureSettings, key, fallback) | |||
| } | |||
There was a problem hiding this comment.
Figured it out - this is a nice change - not sure if that context.Background() might cause us more hard than good in the end but let's roll with it for now.
Currently, every ValidateConfig accepts entire
receivers.FactoryConfigand takes only 3 fields at best: raw settings, secure settings map, and decrypt function.This PR refactors those functions to accept only two parameters (sometimes one):
func(key string, fallback string) stringas you can see it does not accept the secure settings (and context). The reason is to hide the way secure settings are stored.Also, currently, some functions return
Configstruct and some pointer*Config. This PR refactors those that return pointer to return struct.No public API of this package are changed.