Return an error in `service.NewConfigProvider` if there is no configLocations (v0.45) #4762

noisersup · 2022-01-28T20:11:55Z

Description:
As was said in #4734, this commit:

changes service.NewConfigProvider to return an error when provided configLocations length is equals to 0,
deprecates service.MustNewConfigProvider and makes it panic under the same condition as above,
changes back all references in code from "Must" version to normal and handles error.

When the version 0.44 will come out I will undraft this PR.
Also I'm not sure is it possible to apply only third commit (the previous 2 are from my last PR) but I will remove them when my last PR will be merged.

Link to tracking Issue: #4730

Testing: make all

service/config_provider.go

bogdandrutu · 2022-02-01T02:02:28Z

Plese rebase.

codecov · 2022-02-01T08:50:52Z

Codecov Report

Merging #4762 (e55d31a) into main (89e0d42) will decrease coverage by 0.11%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #4762      +/-   ##
==========================================
- Coverage   90.66%   90.54%   -0.12%     
==========================================
  Files         182      182              
  Lines       10621    10656      +35     
==========================================
+ Hits         9630     9649      +19     
- Misses        774      788      +14     
- Partials      217      219       +2

Impacted Files	Coverage Δ
service/command.go	`73.91% <87.50%> (-8.44%)`	⬇️
service/config_provider.go	`90.44% <87.50%> (-3.96%)`	⬇️
service/collector.go	`71.14% <0.00%> (-4.03%)`	⬇️

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 89e0d42...e55d31a. Read the comment docs.

noisersup · 2022-02-06T08:44:46Z

Should I add entry to changelog?

In my last PR Skip Changelog flag was added.

bogdandrutu · 2022-02-14T17:13:31Z

@noisersup yes add a changelog entry, and rebase so we can merge this.

noisersup · 2022-02-15T21:22:47Z

done

bogdandrutu · 2022-02-16T23:56:25Z

@noisersup I am sorry, I missed this release, but I think that is good, because I think we can do even better now. What about we start using an Option pattern here, and keep only the locations (as required) field?

cc @codeboten @tigrannajaryan @jpkrohling

func NewConfigProvider(locations []string, opts ...ConfigProviderOption) (ConfigProvider, error) {
}

bogdandrutu · 2022-02-18T19:30:58Z

@noisersup looks like we have an agreement here, do you mind implementing that change?

noisersup · 2022-02-18T19:37:24Z

@bogdandrutu Sure, I'll implement that.

…rovider`

noisersup · 2022-02-20T10:11:48Z

I hope I understood the requirements

bogdandrutu

I documented couple of options. Probably we can extract issues out of these and move forward with the current PR which does improve lots of the things.

bogdandrutu · 2022-02-20T17:08:29Z

service/config_provider.go

+func WithConfigMapProviders(c map[string]configmapprovider.Provider) ConfigProviderOption {
+	return func(opts *ConfigProviderOptions) {
+		opts.configMapProviders = c
+	}
+}


Not sure if we want a map as argument or one of the next two options:

func WithConfigMapProvider(scheme string, p configmapprovider.Provider) ConfigProviderOption { return func(opts *ConfigProviderOptions) { opts.configMapProviders[scheme] = p } }

// Change configmapprovider.Provider to have a `Scheme()` func on it. func WithConfigMapProvider(p configmapprovider.Provider) ConfigProviderOption { return func(opts *ConfigProviderOptions) { opts.configMapProviders[p.Scheme()] = p } }

I wanted to implement both approaches in seperate commits but I have an issue with the first.

Due to the fact that 2 of the functions (service.MustNewConfigProvider and service.TestConfigProvider_Errors) that use WithConfigMapProviders are using map[string]configmapprovider.Provider approach I need to translate these maps to []ConfigProviderOption and after that append this array to other Options and pass them to NewConfigProvider function.

example:

// Convert map[string]configmapprovider.Provider to []ConfigProviderOption configMapProvidersOpts := []ConfigProviderOption{} for scheme, provider := range configMapProviders { configMapProvidersOpts = append(configMapProvidersOpts, WithConfigMapProvider(scheme, provider)) } configProvider, err := NewConfigProvider(locations, append(configMapProvidersOpts, WithCfgMapConverters(cfgMapConverters), WithConfigUnmarshaler(configUnmarshaler))...)

should I create a function that converts config maps to []ConfigProviderOption and appends them to provided options
to avoid reused code or it's not that big deal?

func AppendConfigMapProvider( opts []ConfigProviderOption, configMapProviders map[string]configmapprovider.Provider) []ConfigProviderOption { // Convert map[string]configmapprovider.Provider to []ConfigProviderOption for scheme, provider := range configMapProviders { opts = append(opts, WithConfigMapProvider(scheme, provider)) } return opts }

// Change configmapprovider.Provider to have a Scheme() func on it.

I think I prefer this approach. We require that providers have a scheme anyways, so why not a way to determine what they think their scheme is?

should I create a function that converts config maps to []ConfigProviderOption and appends them to provided options to avoid reused code or it's not that big deal?

An unexported helper function to do this might be useful, but at least service.MustNewConfigProvider() is expected to go away in the near future, so I'm not sure how big a deal it is.

I think I prefer this approach. We require that providers have a scheme anyways, so why not a way to determine what they think their scheme is?

After more thoughts on this, I think is the right thing to do, since this will allow us to use the same pattern in the builder to support more providers - builder will look for NewProvider in the given import (similar to NewFactory for components.

Per @Aneurysm9 comment the service.MustNewConfigProvider() is going away, and the other one is a test. I would rather look into an example how a distribution with extra providers will use this. Or an example written for this case.

service/config_provider.go

Aneurysm9 · 2022-02-21T18:33:57Z

service/config_provider.go

+	}
+}
+
+func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {


Suggested change

func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {

func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {

Consistency is important.

service/config_provider.go

Aneurysm9 · 2022-02-21T18:44:49Z

service/config_provider.go

+func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
+	return func(opts *configProvider) {
+		opts.cfgMapConverters = c
+	}
+}


Suggested change

func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {

return func(opts *configProvider) {

opts.cfgMapConverters = c

}

}

func WithConfigMapConverters(c ...config.MapConverterFunc) ConfigProviderOption {

return func(opts *configProvider) {

opts.cfgMapConverters = append(opts.cfgMapConverters, c...)

}

}

I think I would prefer to see these append to the config, allowing for multiple invocations of the options, possibly provided by separate packages. The invocation of NewConfigProvider() ultimately controls the order in which they're applied. A small change would be required there to set the default values after applying options if the list is empty, rather than setting it first and allowing it to be overwritten.

The same applies to WithConfigMapProviders(). There order is not important, so multiple maps can be merged.

I am confused about your comment: I think I would prefer to see these append to the config, allowing for multiple invocations of the options, possibly provided by separate packages. are you suggesting a global?

Even without the "default" problem, your suggestion does not trivially allow "no converters" case.

Also these append thing is unclear to me, I would rather provide only one "Converter" and a "MultiConverter" implementation if the list is a problem.

Aneurysm9 · 2022-02-21T18:49:22Z

The title of this PR is no longer adequately descriptive, can you please update it?

@open-telemetry/collector-approvers please review this PR as it has changed in scope and may have more significant impact than initially proposed.

github-actions · 2022-03-09T03:16:04Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

noisersup changed the title ~~Return an error in service.NewConfigProvider if there is no configLocations~~ Return an error in service.NewConfigProvider if there is no configLocations (v0.45) Jan 28, 2022

bogdandrutu reviewed Jan 31, 2022

View reviewed changes

service/config_provider.go Outdated Show resolved Hide resolved

bogdandrutu added this to the v0.45.0 milestone Feb 1, 2022

noisersup force-pushed the GH-4730-0.45 branch from d159eac to 2fedc6e Compare February 1, 2022 08:47

noisersup marked this pull request as ready for review February 6, 2022 08:41

noisersup requested review from a team and bogdandrutu February 6, 2022 08:41

noisersup force-pushed the GH-4730-0.45 branch from 2fedc6e to 9b9d76e Compare February 15, 2022 21:22

noisersup added 2 commits February 20, 2022 11:10

Return an error in service.NewConfigProvider and `NewDefaultConfigP…

697f4bb

…rovider`

Add option pattern in service.NewConfigProvider

0c8c405

noisersup force-pushed the GH-4730-0.45 branch from 9b9d76e to 0c8c405 Compare February 20, 2022 10:10

bogdandrutu reviewed Feb 20, 2022

View reviewed changes

service/config_provider.go Outdated Show resolved Hide resolved

service/config_provider.go Outdated Show resolved Hide resolved

service/config_provider.go Outdated Show resolved Hide resolved

Remove ConfigProviderOptions and NewDefaultConfigProvider

e55d31a

bogdandrutu approved these changes Feb 21, 2022

View reviewed changes

Aneurysm9 reviewed Feb 21, 2022

View reviewed changes

bogdandrutu mentioned this pull request Mar 1, 2022

Add a NewConfigProvider that uses options for settings, and does input validation #4936

Merged

github-actions bot added the Stale label Mar 9, 2022

bogdandrutu removed the Stale label Mar 13, 2022

bogdandrutu closed this Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return an error in `service.NewConfigProvider` if there is no configLocations (v0.45) #4762

Return an error in `service.NewConfigProvider` if there is no configLocations (v0.45) #4762

noisersup commented Jan 28, 2022

bogdandrutu commented Feb 1, 2022

codecov bot commented Feb 1, 2022 •

edited

Loading

noisersup commented Feb 6, 2022

bogdandrutu commented Feb 14, 2022

noisersup commented Feb 15, 2022

bogdandrutu commented Feb 16, 2022

bogdandrutu commented Feb 18, 2022

noisersup commented Feb 18, 2022

noisersup commented Feb 20, 2022

bogdandrutu left a comment

bogdandrutu Feb 20, 2022

noisersup Feb 21, 2022 •

edited

Loading

Aneurysm9 Feb 21, 2022

bogdandrutu Feb 22, 2022

Aneurysm9 Feb 21, 2022

bogdandrutu Feb 21, 2022

Aneurysm9 Feb 21, 2022

bogdandrutu Feb 22, 2022

Aneurysm9 commented Feb 21, 2022

github-actions bot commented Mar 9, 2022

	func WithCfgMapConverters(c []config.MapConverterFunc) ConfigProviderOption {
	func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption {

Return an error in service.NewConfigProvider if there is no configLocations (v0.45) #4762

Return an error in service.NewConfigProvider if there is no configLocations (v0.45) #4762

Conversation

noisersup commented Jan 28, 2022

bogdandrutu commented Feb 1, 2022

codecov bot commented Feb 1, 2022 • edited Loading

Codecov Report

noisersup commented Feb 6, 2022

bogdandrutu commented Feb 14, 2022

noisersup commented Feb 15, 2022

bogdandrutu commented Feb 16, 2022

bogdandrutu commented Feb 18, 2022

noisersup commented Feb 18, 2022

noisersup commented Feb 20, 2022

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Feb 20, 2022

Choose a reason for hiding this comment

noisersup Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Aneurysm9 Feb 21, 2022

Choose a reason for hiding this comment

bogdandrutu Feb 22, 2022

Choose a reason for hiding this comment

Aneurysm9 Feb 21, 2022

Choose a reason for hiding this comment

bogdandrutu Feb 21, 2022

Choose a reason for hiding this comment

Aneurysm9 Feb 21, 2022

Choose a reason for hiding this comment

bogdandrutu Feb 22, 2022

Choose a reason for hiding this comment

Aneurysm9 commented Feb 21, 2022

github-actions bot commented Mar 9, 2022

Return an error in `service.NewConfigProvider` if there is no configLocations (v0.45) #4762

Return an error in `service.NewConfigProvider` if there is no configLocations (v0.45) #4762

codecov bot commented Feb 1, 2022 •

edited

Loading

noisersup Feb 21, 2022 •

edited

Loading