Move service.ConfigMapConverterFunc to config.MapConverterFunc #4673
Conversation
The drop of the Config prefix from the func name was forced by go lint, which does not like to repeat the package name as the prefix for types. Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
mvcfgmapconv
branch
from
cc3708f
to
c71b003
Compare
Codecov Report
@@ Coverage Diff @@
## main #4673 +/- ##
=======================================
Coverage 90.72% 90.72%
=======================================
Files 179 179
Lines 10627 10627
=======================================
Hits 9641 9641
Misses 765 765
Partials 221 221
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM.
@Aneurysm9 this is a breaking change, but should be an easy fix if you depend on it in your distro, so I assume not a problem, right?
There was a problem hiding this comment.
this is a breaking change, but should be an easy fix if you depend on it in your distro, so I assume not a problem, right?
Yes, I believe we can work with this. #4672 is more important to address breakage that we could not fix without code duplication.
I think the addition of a context.Context to this function signature is a valuable addition and was done in #4668 so could be done here, but as long as it is done before the next release there wouldn't need to be two breaking changes here to deal with.
| // TODO: Consider to add a context as the first argument. | ||
| type MapConverterFunc func(*Map) error |
There was a problem hiding this comment.
I believe that this is a valuable addition and should be done now.
The drop of the Config prefix from the func name was forced by go lint, which does not like to repeat the package name as the prefix for types.
Signed-off-by: Bogdan Drutu [email protected]