deep merge machine options, #455

[keen99]

use deep merge for machine() add_machine_options, addresses bootstrap_options merge failure in #455

[chef-supermarket]

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

• @keen99

Please sign the CLA here.

[chef-supermarket]

Hi. Your friendly Curry bot here. Just letting you know that all commit authors have become authorized to contribute. I have added the "Signed CLA" label to this issue so it can easily be found in the future.

[keen99]

mm, this doesn't quite work -

  with_machine_options({
    :Moption_one => "one",
    :Moption_two => "one",
    :bootstrap_options => {
      :Boption_one => "one",
      :Boption_two => "one",
     },
  })
  add_machine_options({
    :Moption_two => "two",
    :bootstrap_options => {
      :Boption_two => "two",
      :Boption_three => "two",

     },
  })
m = machine "bootstrapper-test" do
    add_machine_options :bootstrap_options => { :Boption_three => 'three', :Boption_four => 'four' }
    action :allocate
end

puts "DSR: m.machine_options = #{m.machine_options}"

DSR: m.machine_options = {:bootstrap_options=>{:Boption_three=>"two", :Boption_four=>"four", :Boption_one=>"one", :Boption_two=>"two"}, :Moption_one=>"one", :Moption_two=>"two"}

Boption_three didn't get replaced....so back to the merge drawing board tomorrow.

[keen99]

ok, there we go, now it's merging in the right direction:

DSR: m.machine_options = {:Moption_one=>"one", :Moption_two=>"two", :bootstrap_options=>{:Boption_one=>"one", :Boption_two=>"two", :Boption_three=>"three", :Boption_four=>"four"}}

[keen99]

tests should probably be updated to validate this, but I dont even know where to begin on this. :)

[tyler-ball]

@keen99 You should not use add_machine_options inside the machine resource - there you should be specifying machine_options (the attribute). How do your tests handle this?

[keen99]

er...if we shouldn't use it, why is it there? (it being the "add_machine_options" attribute to the machine resource)

and without it, how do I override my "global" option state for a specific machine without contaminating global, at a per-machine level?

for example, normally I create a t2.micro by default, but want to create a t1.micro for this machine. I need to switch AMIs and instance types.

On Tue, Oct 6, 2015 at 4:36 PM, Tyler Ball [email protected] wrote:

@keen99 https://github.com/keen99 You should not use add_machine_options inside the machine resource - there you should be specifying machine_options (the attribute). How do your tests handle this?

— Reply to this email directly or view it on GitHub https://github.com/chef/chef-provisioning/pull/457#issuecomment-145992733 .

[keen99]

this pattern is certainly mentioned in #383 - the problem is that the existing version doesn't support a deep merge, so bootstrap_options is just replaced (or worse, blows up in many variations)

[tyler-ball]

@keen99 add_machine_options isn't actually an attribute, it is just a method on the resource. I think the error you are seeing is because Cheffish::MergedConfig is an incomplete class. We should make it compatible with the Hash API but it isn't right now.

The reason so many people use this is because the machine_option attribute completely replaces whatever was set by with_machine_options and add_machine_options does not. The many ways these attributes can be set is super confusing.

This is my vision for the future:

1. A with_machine_options method that accepts an optional block where the machine_options only exist in there. If you have an initial with_machine_options and you call with_machine_options again, it will replace the existing one. If you have an initial with_machine_options and you call with_machine_options again but you pass the optional block, then it will only replace the existing one within that block.
2. Delete add_machine_options - it is too confusing/unecessary.
3. Make the machine_options attribute by default merge (with the attribute taking precedence) with any with_machine_options currently set.
4. Optionally, add another attribute ignore_with_machine_options that defaults to false. If true, it does not merge with_machine_options and machine_options. But I don't think this is necessary if users always specify the optional block to with_machine_options.

There isn't a huge gap between the present and this vision. I think #1 is already done, but this needs testing. People can stop using add_machine_options until we get it removed. And #3 can be implemented by using the following for the attribute:

machine "my_machine" do
  machine_options Cheffish::MergedConfig.new({...}, run_context.chef_provisioning.current_machine_options)

So I think the correct fix, instead of this PR, is to fix the problem with the MergedConfig class. Would you like to tackle that?

[keen99]

fixing MergedConfig is probably outside of my time and scope to tackle

but your vision plan seems like it would introduce breaking and non-obvious behavior changes for existing users. (ie machine()'s machine_options becoming a merge instead of a replace) as a user, I'd prefer a path that continues to support the existing mechanisms, even if they're "wrong"

• and only change the existing behavior on purpose. (ie ignore_with_machine_options would default to true in that vision, instead of false)

personally I only find current state -

with_machine_options()
add_machine_options()

machine() do
 machine_options
 add_machine_options
end

confusing simply because the two add_'s dont behave the same way. I haven't seen (though I haven't read every issue.... :P ) any indication of it being confusing, sort of a lack of docu. that one issue seems to be the extent of the docu on it.

on 1) - with_machine_options() definitely does overwrite the global state from what I saw in testing, each time you use it (taking into account the various convergence layers..)

with_machine_options()
machine() do
end


with_machine_options()
machine() do
end

does what logic would expect - the second machine gets the second set.

all that said - I'm far from being a developer. at best I'm a poor hack. I do seem to spend a lot of my time fixing things in ruby, though. :)

On Tue, Oct 6, 2015 at 6:08 PM, Tyler Ball [email protected] wrote:

@keen99 https://github.com/keen99 add_machine_options isn't actually an attribute, it is just a method on the resource. I think the error you are seeing is because Cheffish::MergedConfig is an incomplete class. We should make it compatible with the Hash API but it isn't right now.

The reason so many people use this is because the machine_option attribute completely replaces whatever was set by with_machine_options and add_machine_options does not. The many ways these attributes can be set is super confusing.

This is my vision for the future:

1. A with_machine_options method that accepts an optional block where the machine_options only exist in there. If you have an initial with_machine_options and you call with_machine_options again, it will replace the existing one. If you have an initial with_machine_options and you call with_machine_options again but you pass the optional block, then it will only replace the existing one within that block.
2. Delete add_machine_options - it is too confusing/unecessary.
3. Make the machine_options attribute by default merge (with the attribute taking precedence) with any with_machine_options currently set.
4. Optionally, add another attribute ignore_with_machine_options that defaults to false. If true, it does not merge with_machine_options and machine_options. But I don't think this is necessary if users always specify the optional block to with_machine_options.

There isn't a huge gap between the present and this vision. I think #1 is already done, but this needs testing. People can stop using add_machine_options until we get it removed. And #3 can be implemented by using the following for the attribute:

machine "my_machine" do machine_options Cheffish::MergedConfig.new({...}, run_context.chef_provisioning.current_machine_options)

So I think the correct fix, instead of this PR, is to fix the problem with the MergedConfig class. Would you like to tackle that?

— Reply to this email directly or view it on GitHub https://github.com/chef/chef-provisioning/pull/457#issuecomment-146016529 .

[keen99]

I'll definite say, though, that as a chef user, having to do this:

machine "my_machine" do
  machine_options Cheffish::MergedConfig.new({...}, run_context.chef_provisioning.current_machine_options)

is a complete non-starter - a) as we've discovered cheffish::mergedconfig doesn't actually do a deep merge, and b) that's just not something a normal user should be doing! that's pure hackery in my book. I might -choose- to do it, but I'd never suggest it to a user as a correct solution. :(

[poliva83]

@tyler-ball I and others in my company have been using add_machine_options inside machine resource blocks. This was actually presented to us as proper path by CSE. I agree with @keen99 that chef-provisioning should be at level of maturity that breaking and non-obvious behavior changes for existing users should only be done in major version releases.

[tyler-ball]

I also agree about breaking changes - what I proposed would only be done in a major version release, with documentation explaining the break and necessary migration steps.