Return an error on config.Map.Set

[mx-psi]

seems there's at least a couple of calls that could trigger an error in this func with Load just above this line, I would be in favour of returning an error from Set

Originally posted by @codeboten in #4462 (comment)

[jpkrohling]

Looks like a good first issue?

[mx-psi]

@jpkrohling yes, I think this should also be required for GA since it's a breaking change

[shree007]

@jpkrohling I will take a look into it.

[bogdandrutu]

@mx-psi I understand the current implementation of "Set" may do this, but I am not sure conceptually when a "Set" func would return an error if you were implementing this?

[mx-psi]

@bogdandrutu I gave some thought to this and I don't have a strong view on it either way. One conceptual argument in favor of returning an error on Set is that we return an error on Merge: it feels inconsistent to return an error on one method but not on the other (internally they are the same koanf error after all).

[bogdandrutu]

The error on Merge is if for one key we change the type, maybe we should document the same. Then it may make sense :)

[mx-psi]

That would make sense to me, yes. @shree007, what do you think?

[shree007]

As I see there is following two things 1. Create proper documentation to prevent passing the heterogenous type parameter in map. 2. Since issue is also producible so i would say better to handle it as well, in case we skip the fact that "map type parameter should not be different" while reviewing the PR in future. This is what I am thinking. Thanks

…

On Wed, 12 Jan, 2022, 2:56 pm Pablo Baeyens, ***@***.***> wrote: That would make sense to me, yes. @shree007 <https://github.com/shree007>, what do you think? — Reply to this email directly, view it on GitHub <#4467 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADG76LOFKKIDQUNDV7JSJNDUVVCNHANCNFSM5IQVXU4Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[mx-psi]

Sounds reasonable to me

[bogdandrutu]

@mx-psi @shree007 I think nobody uses "Set" anymore, should we better remove the API?

[shree007]

@bogdandrutu If nobody is using then I can proceed for code cleanup, I mean remove the API
cc: @mx-psi @jpkrohling

[bogdandrutu]

@shree007 let's craft a PR.

[shree007]

Ok, I will do that.

…

On Wed, Mar 23, 2022 at 6:45 PM Bogdan Drutu ***@***.***> wrote: @shree007 <https://github.com/shree007> let's craft a PR. — Reply to this email directly, view it on GitHub <#4467 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADG76LMI74XSPZ22ICNDY2TVBMKQRANCNFSM5IQVXU4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mx-psi]

We make use of Set on the Datadog Agent OTLP endpoint, to transform a Viper map into a config.Map. I think it's safe to remove and we could take a different approach and avoid using Set.

[shree007]

@bogdandrutu Before removing I am checking and found that there are two places where its being used
https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configmapprovider/expand.go#L30
https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/internal/configsource/manager.go#L212
Should I still proceed to craft the PR for removing ?
Please let me know thanks