Refactor configuration struct and add /config endpoint. #132
Conversation
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
==========================================
+ Coverage 76.18% 76.62% +0.44%
==========================================
Files 23 24 +1
Lines 865 890 +25
==========================================
+ Hits 659 682 +23
- Misses 170 172 +2
Partials 36 36
Continue to review full report at Codecov.
|
| sg.GoListenAndServe("api", conf.API.Port, routers.NewDefaultAPIRouter(optlyCache, conf.API, agentMetricsRegistry)) | ||
| sg.GoListenAndServe("webhook", conf.Webhook.Port, routers.NewWebhookRouter(optlyCache, conf.Webhook)) | ||
| sg.GoListenAndServe("admin", conf.Admin.Port, routers.NewAdminRouter(conf.Admin)) // Admin should be added last. | ||
| sg.GoListenAndServe("admin", conf.Admin.Port, routers.NewAdminRouter(*conf)) // Admin should be added last. |
There was a problem hiding this comment.
Should NewAdminRouter accept a pointer type, instead of dereferencing it here?
There was a problem hiding this comment.
I was going back and forth on this. I wanted to be consistent with all of the the other constructors which are not using a pointer.
config/config.go
Outdated
| Log LogConfig `yaml:"log" json:"log"` | ||
| Processor ProcessorConfig `yaml:"processor" json:"processor"` | ||
| Server ServerConfig `yaml:"server" json:"server"` | ||
| Webhook WebhookConfig `yaml:"webhook" json:"webhook"` | ||
| } | ||
|
|
||
| // OptlyConfig holds the set of SDK keys to bootstrap during initialization |
There was a problem hiding this comment.
Need to update this comment
config/config.go
Outdated
| BatchSize int `yaml:"batchSize" default:"10"` | ||
| QueueSize int `yaml:"queueSize" default:"1000"` | ||
| FlushInterval time.Duration `yaml:"flushInterval" default:"30s"` | ||
| BatchSize int `yaml:"batchSize" json:"batchSize" default:"10"` |
There was a problem hiding this comment.
Nit: any lint or test we can set up to ensure both yaml and json remappings on everything?
There was a problem hiding this comment.
I can see if these two can be consolidated.
There was a problem hiding this comment.
I couldn't find one, but since yaml is only used for unmarshaling then we don't technically need to map the field names. We do however need the json field mappings to yield the desired casing in the /config endpoint so I'll remove the yaml to avoid any potential discrepancies.
pkg/optimizely/cache.go
Outdated
| @@ -38,12 +38,12 @@ type OptlyCache struct { | |||
| optlyMap cmap.ConcurrentMap | |||
| ctx context.Context | |||
| wg sync.WaitGroup | |||
| conf config.OptlyConfig | |||
| conf config.ProcessorConfig | |||
There was a problem hiding this comment.
Should we rename this field to processorConf?
Summary
Exposing "optly" in the configs was gross. Moving items to the root of the application config felt more appropriate in some cases as Agent is optimizely. I also wanted to expose the configuration in the routes as this is a handy endpoint when you want to confirm that certain configs are loaded etc.