From 711270b90a9b3327beb5a48a5508d653b55a3c40 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Mon, 6 Jan 2020 11:10:28 -0800 Subject: [PATCH 1/3] Fixes #11287: Set top level provisioner name if set in provisioner config Prior to this commit, if a user had configured a provisioner that had a config with a `name` option, it would not properly set the top level provisioner classes name config option which would lead to some understanibly confusing results when trying to `--provision-with`. This commit fixes that by checking to see if the top level name isn't set, look to see if that provisioners config defines a name, and use that instead. --- .../action/builtin/mixin_provisioners.rb | 19 +++++- plugins/kernel_v2/config/vm_provisioner.rb | 5 +- .../action/builtin/mixin_provisioners_test.rb | 63 ++++++++++++------- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/lib/vagrant/action/builtin/mixin_provisioners.rb b/lib/vagrant/action/builtin/mixin_provisioners.rb index ac5c89cab..e343a0455 100644 --- a/lib/vagrant/action/builtin/mixin_provisioners.rb +++ b/lib/vagrant/action/builtin/mixin_provisioners.rb @@ -25,9 +25,26 @@ module Vagrant # Store in the type map so that --provision-with works properly @_provisioner_types[result] = provisioner.type + # Set top level provisioner name to provisioner configs name if top level name not set. + # This is mostly for handling the shell provisioner, if a user has set its name like: + # + # config.vm.provision "shell", name: "my_provisioner" + # + # Where `name` is a shell config option, not a top level provisioner class option + # + # Note: `name` is set to a symbol, since it is converted to one via #Config::VM.provision + provisioner_name = provisioner.name + if !provisioner_name + if provisioner.config.name + provisioner_name = provisioner.config.name.to_sym + end + else + provisioner_name = provisioner_name.to_sym + end + # Build up the options options = { - name: provisioner.name, + name: provisioner_name, run: provisioner.run, before: provisioner.before, after: provisioner.after, diff --git a/plugins/kernel_v2/config/vm_provisioner.rb b/plugins/kernel_v2/config/vm_provisioner.rb index d0c545ba9..97f838e6e 100644 --- a/plugins/kernel_v2/config/vm_provisioner.rb +++ b/plugins/kernel_v2/config/vm_provisioner.rb @@ -9,7 +9,10 @@ module VagrantPlugins # Unique name for this provisioner # - # @return [String] + # Accepts a string, but is ultimately forced into a symbol in the top level method inside + # #Config::VM.provision method while being parsed from a Vagrantfile + # + # @return [Symbol] attr_reader :name # Internal unique name for this provisioner diff --git a/test/unit/vagrant/action/builtin/mixin_provisioners_test.rb b/test/unit/vagrant/action/builtin/mixin_provisioners_test.rb index 63cb4def3..ec3726d65 100644 --- a/test/unit/vagrant/action/builtin/mixin_provisioners_test.rb +++ b/test/unit/vagrant/action/builtin/mixin_provisioners_test.rb @@ -13,7 +13,7 @@ describe Vagrant::Action::Builtin::MixinProvisioners do sandbox.create_vagrant_env end - let(:provisioner_config){ {} } + let(:provisioner_config){ double("provisioner_config", name: nil) } let(:provisioner_one) do prov = VagrantPlugins::Kernel_V2::VagrantConfigProvisioner.new("spec-test", :shell) prov.config = provisioner_config @@ -24,8 +24,14 @@ describe Vagrant::Action::Builtin::MixinProvisioners do prov.config = provisioner_config prov end + let(:provisioner_three) do + prov = VagrantPlugins::Kernel_V2::VagrantConfigProvisioner.new(nil, :shell) + provisioner_config = double("provisioner_config", name: "my_shell") + prov.config = provisioner_config + prov + end - let(:provisioner_instances) { [provisioner_one,provisioner_two] } + let(:provisioner_instances) { [provisioner_one,provisioner_two,provisioner_three] } let(:ui) { double("ui") } let(:vm) { double("vm", provisioners: provisioner_instances) } @@ -54,6 +60,17 @@ describe Vagrant::Action::Builtin::MixinProvisioners do shell_two = result[1] expect(shell_two.first).to be_a(VagrantPlugins::Shell::Provisioner) end + + it "returns all the instances of configured provisioners" do + result = subject.provisioner_instances(env) + expect(result.size).to eq(provisioner_instances.size) + shell_one = result.first + expect(shell_one[1][:name]).to eq(:"spec-test") + shell_two = result[1] + expect(shell_two[1][:name]).to eq(:"spec-test") + shell_three = result[2] + expect(shell_three[1][:name]).to eq(:"my_shell") + end end context "#sort_provisioner_instances" do @@ -91,9 +108,9 @@ describe Vagrant::Action::Builtin::MixinProvisioners do it "returns the array in the correct order" do result = subject.provisioner_instances(env) - expect(result[0].last[:name]).to eq("before-test") - expect(result[1].last[:name]).to eq("root-test") - expect(result[2].last[:name]).to eq("after-test") + expect(result[0].last[:name]).to eq(:"before-test") + expect(result[1].last[:name]).to eq(:"root-test") + expect(result[2].last[:name]).to eq(:"after-test") end end @@ -121,10 +138,10 @@ describe Vagrant::Action::Builtin::MixinProvisioners do it "puts the each shortcut provisioners in place" do result = subject.provisioner_instances(env) - expect(result[0].last[:name]).to eq("before-test") - expect(result[1].last[:name]).to eq("root-test") - expect(result[2].last[:name]).to eq("before-test") - expect(result[3].last[:name]).to eq("root2-test") + expect(result[0].last[:name]).to eq(:"before-test") + expect(result[1].last[:name]).to eq(:"root-test") + expect(result[2].last[:name]).to eq(:"before-test") + expect(result[3].last[:name]).to eq(:"root2-test") end end @@ -152,10 +169,10 @@ describe Vagrant::Action::Builtin::MixinProvisioners do it "puts the each shortcut provisioners in place" do result = subject.provisioner_instances(env) - expect(result[0].last[:name]).to eq("root-test") - expect(result[1].last[:name]).to eq("after-test") - expect(result[2].last[:name]).to eq("root2-test") - expect(result[3].last[:name]).to eq("after-test") + expect(result[0].last[:name]).to eq(:"root-test") + expect(result[1].last[:name]).to eq(:"after-test") + expect(result[2].last[:name]).to eq(:"root2-test") + expect(result[3].last[:name]).to eq(:"after-test") end end @@ -189,12 +206,12 @@ describe Vagrant::Action::Builtin::MixinProvisioners do it "puts the each shortcut provisioners in place" do result = subject.provisioner_instances(env) - expect(result[0].last[:name]).to eq("before-test") - expect(result[1].last[:name]).to eq("root-test") - expect(result[2].last[:name]).to eq("after-test") - expect(result[3].last[:name]).to eq("before-test") - expect(result[4].last[:name]).to eq("root2-test") - expect(result[5].last[:name]).to eq("after-test") + expect(result[0].last[:name]).to eq(:"before-test") + expect(result[1].last[:name]).to eq(:"root-test") + expect(result[2].last[:name]).to eq(:"after-test") + expect(result[3].last[:name]).to eq(:"before-test") + expect(result[4].last[:name]).to eq(:"root2-test") + expect(result[5].last[:name]).to eq(:"after-test") end end @@ -228,10 +245,10 @@ describe Vagrant::Action::Builtin::MixinProvisioners do it "puts the each shortcut provisioners in place" do result = subject.provisioner_instances(env) - expect(result[0].last[:name]).to eq("before-test") - expect(result[1].last[:name]).to eq("root-test") - expect(result[2].last[:name]).to eq("root2-test") - expect(result[3].last[:name]).to eq("after-test") + expect(result[0].last[:name]).to eq(:"before-test") + expect(result[1].last[:name]).to eq(:"root-test") + expect(result[2].last[:name]).to eq(:"root2-test") + expect(result[3].last[:name]).to eq(:"after-test") end end end From b4c302a74c95acd83b4c2526552748aa686bb5c6 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Mon, 6 Jan 2020 12:48:33 -0800 Subject: [PATCH 2/3] Set shell provisioner name in provision_test --- test/unit/vagrant/action/builtin/provision_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/vagrant/action/builtin/provision_test.rb b/test/unit/vagrant/action/builtin/provision_test.rb index b6dca80f5..89c09f553 100644 --- a/test/unit/vagrant/action/builtin/provision_test.rb +++ b/test/unit/vagrant/action/builtin/provision_test.rb @@ -71,7 +71,7 @@ describe Vagrant::Action::Builtin::Provision do prov.config = provisioner_config prov end - let(:provisioner_config){ {} } + let(:provisioner_config){ double("provisioner_config", name: "spec-test") } before{ expect(vm_config).to receive(:provisioners).and_return([provisioner]) } From ee8b38d47aa93992ef8bd516fe85cc89e5ca79ea Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Mon, 6 Jan 2020 13:25:31 -0800 Subject: [PATCH 3/3] Set provisioner names to symbol Because Vagrant is handling provisioner names to be symbols more uniformly now, update the mocked tests to reflect this change. Otherwise these provisioners will be ignored and not run. --- test/unit/vagrant/action/builtin/provision_test.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/unit/vagrant/action/builtin/provision_test.rb b/test/unit/vagrant/action/builtin/provision_test.rb index 89c09f553..4952582af 100644 --- a/test/unit/vagrant/action/builtin/provision_test.rb +++ b/test/unit/vagrant/action/builtin/provision_test.rb @@ -107,13 +107,13 @@ describe Vagrant::Action::Builtin::Provision do end it "should not run if provision types are set and provisioner is not included" do - env[:provision_types] = ["other-provisioner", "other-test"] + env[:provision_types] = [:"other-provisioner", :"other-test"] expect(hook).not_to receive(:call).with(:provisioner_run, anything) instance.call(env) end it "should run if provision types are set and include provisioner name" do - env[:provision_types] = ["spec-test"] + env[:provision_types] = [:"spec-test"] expect(hook).to receive(:call).with(:provisioner_run, anything) instance.call(env) end @@ -142,13 +142,13 @@ describe Vagrant::Action::Builtin::Provision do end it "should not run if provision types are set and provisioner is not included" do - env[:provision_types] = ["other-provisioner", "other-test"] + env[:provision_types] = [:"other-provisioner", :"other-test"] expect(hook).not_to receive(:call).with(:provisioner_run, anything) instance.call(env) end it "should run if provision types are set and include provisioner name" do - env[:provision_types] = ["spec-test"] + env[:provision_types] = [:"spec-test"] expect(hook).to receive(:call).with(:provisioner_run, anything) instance.call(env) end @@ -177,13 +177,13 @@ describe Vagrant::Action::Builtin::Provision do end it "should not run if provision types are set and provisioner is not included" do - env[:provision_types] = ["other-provisioner", "other-test"] + env[:provision_types] = [:"other-provisioner", :"other-test"] expect(hook).not_to receive(:call).with(:provisioner_run, anything) instance.call(env) end it "should run if provision types are set and include provisioner name" do - env[:provision_types] = ["spec-test"] + env[:provision_types] = [:"spec-test"] expect(hook).to receive(:call).with(:provisioner_run, anything) instance.call(env) end @@ -192,7 +192,7 @@ describe Vagrant::Action::Builtin::Provision do File.open(File.join(data_dir.to_s, "action_provision"), "w") do |file| file.write("1.5:machine-id") end - env[:provision_types] = ["spec-test"] + env[:provision_types] = [:"spec-test"] expect(hook).to receive(:call).with(:provisioner_run, anything) instance.call(env) end