Embed merge support into the default ConfigProvider. #4637
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4637 +/- ##
==========================================
- Coverage 90.45% 90.44% -0.01%
==========================================
Files 181 179 -2
Lines 10598 10594 -4
==========================================
- Hits 9586 9582 -4
Misses 788 788
Partials 224 224
Continue to review full report at Codecov.
|
service/config_provider.go
Outdated
| // * Then applies all the ConfigMapConverterFunc in the given order. | ||
| // * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler. | ||
| // | ||
| // This is a temporary API. |
There was a problem hiding this comment.
Can we add few words what to expect instead going forward?
There was a problem hiding this comment.
Seconded. I think I would prefer to see this take functional options as it would allow for more flexibility in how the set of providers and converters are constructed.
There was a problem hiding this comment.
Even using "options" would not help me, since I will have to do the split of the ConfigUnmarshaler which will cause some breaking changes. Until that happens I prefer to mark this experimental to ensure the right expectations.
service/config_provider.go
Outdated
| // * Then applies all the ConfigMapConverterFunc in the given order. | ||
| // * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler. | ||
| // | ||
| // This is a temporary API. |
There was a problem hiding this comment.
Seconded. I think I would prefer to see this take functional options as it would allow for more flexibility in how the set of providers and converters are constructed.
bogdandrutu
force-pushed
the
rmmerge
branch
2 times, most recently
from
4106128
to
911a7de
Compare
There was a problem hiding this comment.
The code looks good to me.
I would still ask to put some info in Changelog, maybe mention these functions as replacement for removed configmapprovider.NewDefault, configmapprovider.NewExpand, configmapprovider.NewMerge in the same Breaking changes line.
Or at least mention this PR in #4600 's description
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
|
@dmitryax done. |
Depends on #4636