MergedConfig: Use Chef::Mash as storage

[elthariel]

When you have a MergedConfig with two config defining the 'same' key, but one as a string and one as a symbol, you end up having this very confusing behavior:

config['bootstrap_options']
=> {some hash}
config[:bootstrap_options]
=> {another hash}

This PR address this behavior. I hope but doubt this is an appropriate fix, so let me know what you think about it

Signed-off-by: Julien 'Lta' BALLET [email protected]

[rhass]

Is your intention to set value to a string regardless of the original object type of name?

[rhass]

I also am concerned as I think this will cause the value to never be equal to nil, since it would convert the value of nil to a empty string.

Eg:

[1] pry(main)> (nil).nil?
=> true
[2] pry(main)> (nil.to_s).nil?
=> false

[elthariel]

@rhass I'm not sure I understand your comment. I'm only casting the key to a string or symbol. The value will still be nil if the hash has neither the key 'key' nor :key

[lamont-granquist]

kind of 👎 on this one since i'm failure sure this just needs to be a Chef::Mash or needs to use mixlib-config or Chef::Config or something like that... this is already a solved problem, and we should definitely re-use the solution instead of inventing a new problem.

[elthariel]

@lamont-granquist: Yeah, I'd rather switch the underlying implementation of MergedConfig to a Mash if it's ok with you

[elthariel]

To address @lamont-granquist and @rhass comments, I've switched the underlying storage to Chef::Mash, which address the initial issue, and reuse existing code with a known semantic.

[elthariel]

I've also added a few specs for the MergedConfig

[lamont-granquist]

ah i was hoping we could nuke the whole class, now i sort of see what else its trying to do.

but 👍 for fixing the issue at hand this way. i'll have to dig into it to see why it seems to be trying to reimplement attribute precedence levels and if we can avoid that...

@thommay i'm 👍 on it the way it is.

we need to deprecate and kill off them method_missing here as well...

[tyler-ball]

Merging this looks to have broken Chef Provisioning in the latest ChefDK. The issue arises when calling .to_h on the MergedConfig - https://github.com/chef/chef-provisioning-aws/blob/17e249b4b8b01caf30e016eac6f5ed74b43ab01f/lib/chef/provisioning/aws_driver/driver.rb#L852

In cheffish < 4.1 the bootstrap_options there are returned with keys as symbols. In cheffish >= 4.1 the keys are returned as strings. This messes up our hash merging further on in that method because we start storing both :image_id and "image_id" as keys.

[tyler-ball]

It seems to me like this is breaking behavior. Calling to_h on a Mash always returns the keys as strings, not what they were inserted as - http://www.rubydoc.info/gems/chef/Mash#to_hash-instance_method

[lamont-granquist]

This is the correct behavior. We can do the dance of bumping cheffish to 5.0 but there's no going back.

[tyler-ball]

I'm okay with that. But can we revert this, release 4.1.1 (or 4.2) and then re-release this as 5.0? That will prevent breaking all existing Chef Provisioning users

[tyler-ball]

@lamont-granquist 🤗 Thank you!