Merging environment variables should not override existing settings

[cbeer]

We're having a problem with the new environment loading code and our configuration hierarchy.

With a setting like:

feature:
  enabled: true
  group: x

If I have an environment variable (and config configuration to support..) e.g. SETTINGS_FEATURE_GROUP=y, in the application I see:

Settings.feature.group # => 'y'

But, other keys in the configuration are removed:

Settings.feature.enabled # => nil

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 through Config::Options before dumping it.

[pkuczynski]

What would be the point of moving this to a separate method?

[cbeer]

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.

[pkuczynski]

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

[cbeer]

👌 , 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.

[pkuczynski]

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?

[cbeer]

Will do, thanks.

[cbeer]

@pkuczynski 1.2.1 fixes the problem for me, thanks.

[pkuczynski]

Great to hear that :)