Move service/parserprovider package to config/configmapprovider

[mx-psi]

Description:

Second part of #4178.

Link to tracking Issue: Updates #4177

[codecov]

Codecov Report

Merging #4206 (5671d5b) into main (2981b3c) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #4206   +/-   ##
=======================================
  Coverage   87.29%   87.29%           
=======================================
  Files         176      176           
  Lines       10622    10622           
=======================================
  Hits         9273     9273           
  Misses       1108     1108           
  Partials      241      241           

Impacted Files	Coverage Δ
config/configmapprovider/default.go	100.00% <ø> (ø)
config/configmapprovider/expand.go	76.00% <ø> (ø)
config/configmapprovider/file.go	100.00% <ø> (ø)
config/configmapprovider/inmemory.go	70.00% <ø> (ø)
config/configmapprovider/merge.go	84.21% <ø> (ø)
config/configmapprovider/properties.go	89.65% <ø> (ø)
config/configmapprovider/simple.go	100.00% <ø> (ø)
service/command.go	0.00% <0.00%> (ø)
config/configtest/configtest.go	96.66% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2981b3c...5671d5b. Read the comment docs.

[codeboten]

Based on what I'm reading in #4178 i would have expected the contents of parserprovider to end up in config along with the config.MapProvider, it's possible I'm missing context though.

Please resolve the conflict

[mx-psi]

Based on what I’m reading in #4178 i would have expected the contents of parserprovider to end up in config along with the config.MapProvider, it’s possible I’m missing context though.

I was moving them to mapprovider based on #4178 (comment). Happy to move them to config though

[codeboten]

I was moving them to mapprovider based on #4178 (comment). Happy to move them to config though

Ah I see, thanks for clarifying. Personally I would expect the functions to initialize the map provider to live in the same place as the interface, but I don't feel strongly about this. @bogdandrutu @tigrannajaryan any thoughts?

[bogdandrutu]

Maybe a different package but under "config/configmapprovider"?

[mx-psi]

@codeboten how does Bogdan's proposal sound to you?

[codeboten]

@codeboten how does Bogdan's proposal sound to you?

config/mapprovider works for me, no need to repeat the config in the package name.

[bogdandrutu]

@codeboten I think we need to repeat config because right now users will use this as "mapprovider.File" with the repeat the use will be "configmapprovider.File" I think the second is better and clear what map provides.

[mx-psi]

After thinking a bit more about this I agree with Bogdan here, but want to hear your thoughts before changing this again @codeboten

[codeboten]

After thinking a bit more about this I agree with Bogdan here, but want to hear your thoughts before changing this again @codeboten

@bogdandrutu's proposal works for me.

[mx-psi]

@bogdandrutu @codeboten PTAL

[bogdandrutu]

Needs a rebase. Last question: if we go with this package, should the provider interface be in the same place then? Can be done after in a separate PR.

[mx-psi]

Rebased.

If we go with this package, should the provider interface be in the same place then?

I hate to not have thought about this before doing #4178 but yes, I think it should live in the same place, both for consistency with other packages (e.g. configunmarshaler) and to reduce the chance of uncomfortable dependency cycles. I can do a follow up PR for this.

[mx-psi]

See #4337 for the interface move