-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
Merging environment variables should not override existing settings #144
Conversation
end | ||
end | ||
result | ||
__convert_to_hash(marshal_dump) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the point of moving this to a separate method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to move it back if you want. I initially thought we could use __convert_to_hash
directly, but ran into the symbol vs string problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this change brings no value and only makes the code more complicated. so yes, I would like it to be gone :) By the way, are you running this code in Rails? I have a suspicion it might be connected with #120
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 , removed.
Yes, I am running this code in Rails, but I'm not sure about the connection to DeepMerge. If I take the example from this spec and drop into a debugger at https://github.com/railsconfig/config/blob/master/lib/config/options.rb#L110:
[106, 115] in /Users/cabeer/Projects/3rd-party/config/lib/config/options.rb
106: end
107:
108: def merge!(hash)
109: require 'byebug'; byebug
110: current = to_hash
=> 111: DeepMerge.deep_merge!(hash, current)
112: marshal_load(__convert(current).marshal_dump)
113: self
114: end
115:
The two hashes have different keys (strings vs symbols):
(byebug) hash
{
[...]
"size"=>3
"inner"=>{
"something1"=>"new blah"
}
}
(byebug) current
{
:size=>1,
:inner=>{
:something1=>"blah1",
:something2=>"blah2"
},
[...]
...
}
And the merge (dutifully) merges them into an even bigger hash:
(byebug) current
{
:size=>1,
:inner=>{:something1=>"blah1", :something2=>"blah2"},
[...]
"size"=>"3",
"inner"=>{"something1"=>"new blah"}
}
With simple config structures (like size
above), this isn't a problem when we load the hash back into OpenStruct; the last key (which happens to contain the data we want) wins.
With a complex structure (like inner
), the last element replaces the value entirely, removing the previous settings.
I tested your branch and your fix did't really worked for me. So I spent quite some time last night and rebuild how ENV variables are loaded to correctly support multilevel settings. I just published the new version of the gem (1.2.1). Let me know if that fixes your problem? |
Will do, thanks. |
@pkuczynski 1.2.1 fixes the problem for me, thanks. |
Great to hear that :) |
We're having a problem with the new environment loading code and our configuration hierarchy.
With a setting like:
If I have an environment variable (and config configuration to support..) e.g.
SETTINGS_FEATURE_GROUP=y
, in the application I see:But, other keys in the configuration are removed:
It looks like there's a problem with string + symbol hashes coming out of
OpenStruct
. I don't know if we ought to assume OpenStruct will always dump symbolized hashes, or, as I do here, push the hash throughConfig::Options
before dumping it.