Create configload.Parser struct to abstract away from Viper #2728

mx-psi · 2021-03-18T13:29:04Z

Description:

Create configload.Parser struct to encapsulate Viper.

Link to tracking Issue: First step for #2596

Testing: Amended unit tests

Documentation: Updated documentation to reflect new interface

codecov · 2021-03-18T13:36:06Z

Codecov Report

Merging #2728 (db90982) into main (34c17ea) will decrease coverage by 0.03%.
The diff coverage is 78.12%.

@@            Coverage Diff             @@
##             main    #2728      +/-   ##
==========================================
- Coverage   91.67%   91.64%   -0.04%     
==========================================
  Files         293      294       +1     
  Lines       15611    15635      +24     
==========================================
+ Hits        14312    14329      +17     
- Misses        890      896       +6     
- Partials      409      410       +1

Impacted Files	Coverage Δ
config/configload/configload.go	`72.00% <72.00%> (ø)`
config/configparser/config.go	`98.62% <100.00%> (-0.01%)`	⬇️
config/configtest/configtest.go	`100.00% <100.00%> (ø)`
service/application.go	`76.52% <100.00%> (ø)`
service/set_flag.go	`80.95% <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 34c17ea...db90982. Read the comment docs.

mx-psi · 2021-03-18T13:32:55Z

config/config.go

+// toStringMap creates a map[string]interface{} from a Viper object
+// It is equivalent to v.AllSettings() but it maps nil values
+// It is used here because of https://github.com/spf13/viper/issues/819
+func toStringMap(v *viper.Viper) map[string]interface{} {


This can be simplified if/once spf13/viper#819 gets resolved (it seems like Bogdan already stumbled on this one 😄). The code is the same as the AllSettings method except that it does not skip over nil values.

~~Not used anymore, cast.ToStringMap seems to overcome this issue~~

mx-psi · 2021-03-18T13:34:37Z

config/config.go

+
+// ToCustomUnmarshaler creates a custom unmarshaler from a Viper unmarshaled.
+// It should not be used by new code.
+func ToCustomUnmarshaler(viperUnmarshaler func(*viper.Viper, interface{}) error) component.CustomUnmarshaler {


I wrote this wrapper for convenience to fix existing tests but I don't think it should be used by new components.

Maybe not in this PR, but I would like to remove this before we declare this module stable. Maybe we can open an issue and decide how to get rid of this.

Created #2735 to track this

mx-psi · 2021-03-18T13:57:42Z

(Note that contrib tests fail because this is a breaking public API change; we can fix this once this and #2597 are dealt with)

component/component.go

bogdandrutu · 2021-03-18T16:25:27Z

config/config.go

+
+// ToCustomUnmarshaler creates a custom unmarshaler from a Viper unmarshaled.
+// It should not be used by new code.
+func ToCustomUnmarshaler(viperUnmarshaler func(*viper.Viper, interface{}) error) component.CustomUnmarshaler {


Maybe not in this PR, but I would like to remove this before we declare this module stable. Maybe we can open an issue and decide how to get rid of this.

component/component.go

This commit also moves the `ConfigUnmarshaler` and `CustomUnmarshaler` types into the config package, since otherwise an import cycle would happen.

The `config` package indirectly depends on `exporterhelper`, so `Loader` can't be on it.

For some reason my commit is not showing up on the PR

mx-psi · 2021-03-22T12:42:30Z

@bogdandrutu I gave it a try, can you have a look? Note that I had to move some things around to prevent import cycles and that I am exposing Viper() temporarily because of that (to be removed when fixing #2735, which I can do here if you don't mind it being a bigger PR)

bogdandrutu

Let's start with the initial PR to add configload package and move the viper helpers there.

config/configload/configload.go

bogdandrutu · 2021-03-22T20:28:28Z

config/configload/configload.go

+
+// ToStringMap creates a map[string]interface{} from a ConfigLoader
+func (l *Loader) ToStringMap() map[string]interface{} {
+	return cast.ToStringMap(l.v)


Not sure this will return the expected result, we need to test this.

I added some tests, looks like it doesn't so I brought back the implementation I had initially.

config/configload/configload.go

- s/instanceg/instance - Add period at end of doc comments - Reword NewViper comment - Rename ViperDelimiter to KeyDelimiter

mx-psi · 2021-03-23T14:38:24Z

I think I addressed all your comments. I brought back the initial ToStringMap definition since cast.ToStringMap doesn't work (I got confused and thought it would since it is used in some config/config.go code for unrelated stuff)

bogdandrutu · 2021-03-24T23:20:51Z

component/component.go

+type CustomUnmarshaler func(componentSection *configload.Parser, intoCfg interface{}) error
+
+// ToCustomUnmarshaler creates a custom unmarshaler from a Viper unmarshaler.
+func ToCustomUnmarshaler(viperUnmarshaler func(*viper.Viper, interface{}) error) CustomUnmarshaler {


Can we split for the moment this PR into 2:

All changes except changes in component.go (we keep the old interface for the moment), but we add the Parser and move helpers for viper.

Changes in the component.go (reason is maybe we just don't do these changes anymore and jump to add the new unmarshaler directly on the config, and deal that's it).

This commit was done by running git diff origin/main component/component.go exporter/ extension/ \ internal/testcomponents/example_exporter.go processor/ receiver/ | git apply -R

mx-psi · 2021-03-25T16:10:28Z

@bogdandrutu PTAL, I removed the component.go and related changes (check_links failure is unrelated to this PR)

bogdandrutu

We will iterate more in the following PRs.

Make ConfigUnmarshaler use map[string]interface{}

733a513

mx-psi commented Mar 18, 2021

View reviewed changes

mx-psi marked this pull request as ready for review March 18, 2021 13:52

mx-psi requested a review from a team March 18, 2021 13:52

bogdandrutu reviewed Mar 18, 2021

View reviewed changes

mx-psi mentioned this pull request Mar 19, 2021

Remove config.ToCustomUnmarshaler #2735

Closed

Remove viper references on comments

f03cabb

mx-psi mentioned this pull request Mar 19, 2021

Move custom unmarshaler to config instead of component factory #2597

Closed

mx-psi added 5 commits March 22, 2021 12:15

Encapsulate in Loader struct

3e640df

This commit also moves the `ConfigUnmarshaler` and `CustomUnmarshaler` types into the config package, since otherwise an import cycle would happen.

Missing config.NewLoader()

e52d717

Move Loader to configload package

a5ceba1

The `config` package indirectly depends on `exporterhelper`, so `Loader` can't be on it.

Empty commit to retrigger Github

08451b8

For some reason my commit is not showing up on the PR

Merge branch 'main' into mx-psi/remove-viper

8ab53d5

mx-psi added 2 commits March 22, 2021 14:07

Fix merge commit

7ec3127

More merge fixes

75302d4

bogdandrutu reviewed Mar 22, 2021

View reviewed changes

config/configload/configload.go Show resolved Hide resolved

bogdandrutu reviewed Mar 22, 2021

View reviewed changes

config/configload/configload.go Outdated Show resolved Hide resolved

mx-psi added 3 commits March 23, 2021 12:26

Address review comments

c30788a

- s/instanceg/instance - Add period at end of doc comments - Reword NewViper comment - Rename ViperDelimiter to KeyDelimiter

Rename from Loader to Parser

8c0691f

Add tests for ToStringMap and bring back old implementation

26d01c6

bogdandrutu reviewed Mar 24, 2021

View reviewed changes

mx-psi added 3 commits March 25, 2021 15:48

Merge remote-tracking branch 'origin/main' into mx-psi/remove-viper

2239eda

Revert changes on component/component.go and related changes

f749e36

This commit was done by running git diff origin/main component/component.go exporter/ extension/ \ internal/testcomponents/example_exporter.go processor/ receiver/ | git apply -R

Manual fixes after reverting component files

2e5e539

mx-psi changed the title ~~Hide Viper from unmarshaler interface~~ Create configload.Parser struct to abstract away from Viper Mar 25, 2021

mx-psi added 2 commits March 25, 2021 16:39

s/componentViperSection/componentSection/

f2dc1df

Merge remote-tracking branch 'origin/main' into mx-psi/remove-viper

db90982

bogdandrutu reviewed Mar 25, 2021

View reviewed changes

bogdandrutu approved these changes Mar 25, 2021

View reviewed changes

bogdandrutu merged commit 06ea2c6 into open-telemetry:main Mar 25, 2021

mx-psi deleted the mx-psi/remove-viper branch March 26, 2021 11:26

mx-psi mentioned this pull request Mar 26, 2021

Track @mx-psi interest and contributions for approver #2819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create configload.Parser struct to abstract away from Viper #2728

Create configload.Parser struct to abstract away from Viper #2728

mx-psi commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 18, 2021 •

edited

Loading

mx-psi Mar 18, 2021

mx-psi Mar 22, 2021 •

edited

Loading

mx-psi Mar 18, 2021

bogdandrutu Mar 18, 2021

mx-psi Mar 19, 2021

mx-psi commented Mar 18, 2021

bogdandrutu Mar 18, 2021

mx-psi commented Mar 22, 2021 •

edited

Loading

bogdandrutu left a comment

bogdandrutu Mar 22, 2021

mx-psi Mar 23, 2021

mx-psi commented Mar 23, 2021 •

edited

Loading

bogdandrutu Mar 24, 2021

mx-psi commented Mar 25, 2021 •

edited

Loading

bogdandrutu left a comment

Create configload.Parser struct to abstract away from Viper #2728

Create configload.Parser struct to abstract away from Viper #2728

Conversation

mx-psi commented Mar 18, 2021 • edited Loading

codecov bot commented Mar 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mx-psi Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Mar 18, 2021

Choose a reason for hiding this comment

mx-psi commented Mar 22, 2021 • edited Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

mx-psi commented Mar 25, 2021 • edited Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

mx-psi commented Mar 18, 2021 •

edited

Loading

codecov bot commented Mar 18, 2021 •

edited

Loading

mx-psi Mar 22, 2021 •

edited

Loading

mx-psi commented Mar 22, 2021 •

edited

Loading

mx-psi commented Mar 23, 2021 •

edited

Loading

mx-psi commented Mar 25, 2021 •

edited

Loading