Skip to content
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

machine() add_machine_options fails to merge bootstrap_options #455

Open
keen99 opened this issue Sep 30, 2015 · 2 comments
Open

machine() add_machine_options fails to merge bootstrap_options #455

keen99 opened this issue Sep 30, 2015 · 2 comments
Labels
Aspect: Documentation How do we use this project? Triage: Confirmed Indicates and issue has been confirmed as described. Type: Bug Doesn't work as expected.

Comments

@keen99
Copy link

keen99 commented Sep 30, 2015

Originally mentioned this in #383 (comment)

Spent more time digging and confirmed that machine()'s add_machine_options uses a different merge strategy than add_machine_options() does.

https://github.com/chef/chef-provisioning/blob/master/lib/chef/provisioning/chef_run_data.rb#L79

    def add_machine_options(options, &block)
      with_machine_options(Chef::Mixin::DeepMerge.hash_only_merge(current_machine_options, options), &block)
    end

vs

https://github.com/chef/chef-provisioning/blob/master/lib/chef/resource/machine.rb#L100

  def add_machine_options(options)
    @machine_options = Cheffish::MergedConfig.new(options, @machine_options)
  end

The end result is if you try to merge bootstrap_options at the machine level, you get:

TypeError
---------
can't dup NilClass

Simple to test:

machine "test" do
    machine_options :bootstrap_options => { :Boption_four => 'four' }
    add_machine_options :bootstrap_options => { :Boption_five => 'five' }
    action :allocate
end

or

  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",
     },
  })
machine "test" do
    add_machine_options :bootstrap_options => { :Boption_five => 'five' }
    action :allocate
end

Seems like the fix would be to either consume add_machine_options() for the merge, or switch to using the same DeepMerge strategy. I might be able to get a chance to submit a PR on it...

@keen99
Copy link
Author

keen99 commented Sep 30, 2015

Note: I'm testing against chef-provisioning (1.3.0) at the moment, but this doesn't seem to have changed since 2014 with fd2effc

@tyler-ball
Copy link
Contributor

We just need to delete add_machine_options everywhere. See #457 (comment)

@tas50 tas50 added Type: Bug Doesn't work as expected. Type: Documentation and removed Bug labels Jul 31, 2018
@tas50 tas50 added Aspect: Documentation How do we use this project? Triage: Confirmed Indicates and issue has been confirmed as described. and removed Type: Documentation labels Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aspect: Documentation How do we use this project? Triage: Confirmed Indicates and issue has been confirmed as described. Type: Bug Doesn't work as expected.
Development

No branches or pull requests

3 participants