Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move MapProvider interfaces to config/configmapprovider package #4337

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Nov 2, 2021

Description:

Move MapProvider interfaces to configmapprovider.

This is more consistent with other packages (e.g. configunmarshaler) and (probably) reduces the chance of dependency cycles

Link to tracking Issue: #4206 (comment)

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #4337 (c1c250c) into main (3ef6d73) will not change coverage.
The diff coverage is 89.47%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4337   +/-   ##
=======================================
  Coverage   88.67%   88.67%           
=======================================
  Files         176      176           
  Lines       10377    10377           
=======================================
  Hits         9202     9202           
  Misses        947      947           
  Partials      228      228           
Impacted Files Coverage Δ
service/collector.go 69.87% <0.00%> (ø)
config/configmapprovider/default.go 100.00% <100.00%> (ø)
config/configmapprovider/expand.go 76.00% <100.00%> (ø)
config/configmapprovider/file.go 100.00% <100.00%> (ø)
config/configmapprovider/inmemory.go 70.00% <100.00%> (ø)
config/configmapprovider/merge.go 84.21% <100.00%> (ø)
config/configmapprovider/properties.go 89.65% <100.00%> (ø)
config/configtest/configtest.go 96.66% <100.00%> (ø)
service/command.go 100.00% <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 3ef6d73...c1c250c. Read the comment docs.

@mx-psi mx-psi marked this pull request as ready for review November 2, 2021 13:42
@mx-psi mx-psi requested review from a team and owais November 2, 2021 13:42
@mx-psi
Copy link
Member Author

mx-psi commented Nov 2, 2021

test-coverage failure seems unrelated to this PR.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase, and update contrib before we can merge this.

@bogdandrutu
Copy link
Member

@mx-psi same problem, after the other PR contrib is still unhappy.

@mx-psi
Copy link
Member Author

mx-psi commented Nov 3, 2021

// defined by the given configFile and overwrites fields using properties.
func NewDefaultMapProvider(configFileName string, properties []string) config.MapProvider {
func NewDefaultMapProvider(configFileName string, properties []string) Provider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/NewDefaultMapProvider/NewDefault

What do you think?

func NewExpandMapProvider(base config.MapProvider) config.MapProvider {
// NewExpandMapProvider returns a Provider, that expands all environment variables for a
// config.Map provided by the given Provider.
func NewExpandMapProvider(base Provider) Provider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/NewExpandMapProvider/NewExpand?

What do you think? Same everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewFile is the only one that may sound a bit weird, but in general it sounds better with your proposed change. Will do the change everywhere so that we see how it looks and can decide

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I think something like configmapprovider.NewFromFile and similarly configmapprovider.NewFromProperties sound better to me than configmapprovider.NewFile or configmapprovider.NewProperties (since the latter don't make it clear that you are creating a provider). What do you think?

@bogdandrutu
Copy link
Member

Please fix windows tests:

service\collector_windows.go:136:27: undefined: configmapprovider.NewDefaultMapProvider

@mx-psi
Copy link
Member Author

mx-psi commented Nov 10, 2021

@bogdandrutu ping (all tests passing and I merged main)

@bogdandrutu bogdandrutu merged commit 32274b7 into open-telemetry:main Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants