search

uber-go / config

Configuration for Go applications
https://godoc.org/go.uber.org/config
MIT License
442 stars 41 forks source link

Fix deep map merging for missing keys in dst #89

Closed dgladkov closed 6 years ago

dgladkov commented 6 years ago

This change is based on 1.1.0 release. This fixes a case when map from src gets assigned to a key in dst and then mutated in subsequent mergeMaps call.

CLAassistant commented 6 years ago

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

akshayjshah commented 6 years ago

Hi! Thanks so much for this fix. Can you see if this issue is still reproducible on the head of the master branch? There were a bunch of bugs like this, so we ended up completely rewriting and simplifying the package internals.

I hope that we've fixed your bug. If we haven't, we'll need to start working on a bugfix from scratch.

dgladkov commented 6 years ago

The issueI had is fixed in master, however my project specific use case prevents from upgrading past 1.1.0 as we have a custom Provider with dynamic key lookup, so we're relying on the fact that merge happens on lookup, not on init. I couldn't reimplement that behavior using the new project design.

akshayjshah commented 6 years ago

I see. We're not planning to keep dynamic providers - the code on master is the direction we're taking with this project.

If your environment relies on dynamic providers, this library won't be a good fit for you going forward. You're better off forking the v1.1.0 code, trimming it down to just the bits you use, and treating it as a standalone project.

dgladkov commented 6 years ago

I see, thanks! Given that this PR fixes a known issue, do think it's worth cutting a 1.1.1 bugfix release? Otherwise feel free to close.

akshayjshah commented 6 years ago

I'd prefer not to patch this in, mostly because this is only one of a very large number of known bugs in the v1.1.0 release. If you end up forking this package, I'll happily direct anyone using a dynamic provider to use your fork.