refactor remotecfg.go #4034
refactor remotecfg.go #4034
Conversation
Signed-off-by: Erik Baranowski <[email protected]>
|
|
Signed-off-by: Erik Baranowski <[email protected]>
erikbaranowski
changed the title
Signed-off-by: Erik Baranowski <[email protected]>
Signed-off-by: Erik Baranowski <[email protected]>
Signed-off-by: Erik Baranowski <[email protected]>
| }() | ||
|
|
||
| for { | ||
| select { | ||
| case <-s.ticker.C: | ||
| err := s.fetchRemote() |
There was a problem hiding this comment.
I'm unclear why we only fetchRemote here. The new code handles fetching remote with the fallback to local included.
There was a problem hiding this comment.
Because cached local config is checked on service startup, there's no need to check for cached config every time you query the API because if it's changed from what it was on startup it only changed due to a successful fetchRemote?
| // newAPIClientWithClient creates a metrics-wrapped apiClient from an existing CollectorServiceClient. | ||
| // This is primarily used for testing with mock clients. | ||
| func newAPIClientWithClient(client collectorv1connect.CollectorServiceClient, metrics *metrics) *apiClient { | ||
| return &apiClient{ | ||
| client: client, | ||
| metrics: metrics, | ||
| } | ||
| } |
There was a problem hiding this comment.
small nit: can't we simply define a package-level mock client that tests will share? Is there an issue with reuse? Or a newMockClient()?
var mockClient = &apiClient{
client: &mockCollectorClient{},
metrics: registerMetrics(prometheus.NewRegistry())
}
| // This file contains functionality that is used by custom code external to the | ||
| // remotecfg package. These functions provide public APIs for accessing remotecfg | ||
| // service data and are intended for use by other packages and components. | ||
| package remotecfg |
There was a problem hiding this comment.
nit: I'm on the fence, but should we pull Data() here as well?
Also, let's make it consistent remotecfg on the logs as well.
PR Description
Organize the remotecfg.go for maintainability. Increase unit test coverage which was missing prior due to too much in one file.
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist