From e47deb7fd055dc871235d14bbae4c90b3bdf26c7 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Thu, 9 Nov 2017 18:17:04 +0100 Subject: [PATCH 1/2] ansible: Fix broken 'ask_sudo_pass' option This bug (invalid method call) hasn't been caught by unit tests because Vagrant::Plugin::V2::Config catches all invalid/bad configuration calls and save them for generating error messages during the "validate" stage. This way, the `ask_sudo_pass=(value)` method was not interrupted and the `@ask_become_pass` attribute was (surprisingly) correctly set (allowing the related unit tests to pass). In order to avoid similar problem to happen again, the deprecation message output is now fully verified. --- plugins/provisioners/ansible/config/host.rb | 2 +- .../provisioners/ansible/config/host_test.rb | 1 + .../provisioners/ansible/config/shared.rb | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/plugins/provisioners/ansible/config/host.rb b/plugins/provisioners/ansible/config/host.rb index 06df0d1c4..8197d8e7b 100644 --- a/plugins/provisioners/ansible/config/host.rb +++ b/plugins/provisioners/ansible/config/host.rb @@ -16,7 +16,7 @@ module VagrantPlugins # alias :ask_sudo_pass :ask_become_pass def ask_sudo_pass=(value) - show_deprecation_warning 'ask_sudo_pass', 'ask_become_pass' + show_deprecation_info 'ask_sudo_pass', 'ask_become_pass' @ask_become_pass = value end diff --git a/test/unit/plugins/provisioners/ansible/config/host_test.rb b/test/unit/plugins/provisioners/ansible/config/host_test.rb index b868d2648..e89bedbd3 100644 --- a/test/unit/plugins/provisioners/ansible/config/host_test.rb +++ b/test/unit/plugins/provisioners/ansible/config/host_test.rb @@ -78,6 +78,7 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do allow($stdout).to receive(:puts) end it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :ask_sudo_pass, false + it_behaves_like "any deprecated option", :ask_sudo_pass, :ask_become_pass, true end describe "ask_vault_pass option" do it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :ask_vault_pass, false diff --git a/test/unit/plugins/provisioners/ansible/config/shared.rb b/test/unit/plugins/provisioners/ansible/config/shared.rb index 16902a138..a6495e72c 100644 --- a/test/unit/plugins/provisioners/ansible/config/shared.rb +++ b/test/unit/plugins/provisioners/ansible/config/shared.rb @@ -30,6 +30,17 @@ shared_examples_for 'options shared by both Ansible provisioners' do end +shared_examples_for 'any deprecated option' do |deprecated_option, new_option, option_value| + it "shows the deprecation message" do + expect($stdout).to receive(:puts).with("DEPRECATION: The '#{deprecated_option}' option for the Ansible provisioner is deprecated.").and_return(nil) + expect($stdout).to receive(:puts).with("Please use the '#{new_option}' option instead.").and_return(nil) + expect($stdout).to receive(:puts).with("The '#{deprecated_option}' option will be removed in a future release of Vagrant.\n\n").and_return(nil) + + subject.send("#{deprecated_option}=", option_value) + subject.finalize! + end +end + shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | provisioner_label = "ansible #{ansible_setup} provisioner" @@ -158,6 +169,15 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | allow($stdout).to receive(:puts) end it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :sudo, false + it_behaves_like "any deprecated option", :sudo, :become, true + end + + describe "sudo_user option" do + before do + # Filter the deprecation notice + allow($stdout).to receive(:puts) + end + it_behaves_like "any deprecated option", :sudo_user, :become_user, "foo" end end From 8333090d1d24578bd33f4913f5ae2c0f8b2cf899 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Fri, 10 Nov 2017 20:40:13 +0100 Subject: [PATCH 2/2] ansible: Refuse to run unit tests with an invalid config With this change, a bug like #9173 can be detected. --- .../provisioners/ansible/provisioner_test.rb | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 3b552f323..082645fb2 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -202,6 +202,14 @@ VF end end + def ensure_that_config_is_valid + # Abort the test when an invalid configuration is detected + config.validate(machine) + if config._detected_errors.length > 0 + raise "Invalid Provisioner Configuration! Detected Errors:\n#{config._detected_errors.to_s}" + end + end + describe "#provision" do before do @@ -216,6 +224,8 @@ VF after do unless RSpec.current_example.metadata[:skip_after] + ensure_that_config_is_valid + subject.provision end end @@ -233,6 +243,7 @@ VF config.playbook = STUBBED_INVALID_PATH config.finalize! + ensure_that_config_is_valid expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleError, "`playbook` does not exist on the host: #{STUBBED_INVALID_PATH}") @@ -245,11 +256,8 @@ VF config.playbook = existing_file config.send(option_name + '=', STUBBED_INVALID_PATH) - if option_name == 'extra_vars' - # little trick to auto-append the '@' prefix, which is a duty of the config validator... - config.validate(machine) - end config.finalize! + ensure_that_config_is_valid expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleError, "`#{option_name}` does not exist on the host: #{STUBBED_INVALID_PATH}") @@ -261,6 +269,7 @@ VF describe 'when ansible-playbook fails' do it "raises an error", skip_before: true, skip_after: true do config.finalize! + ensure_that_config_is_valid allow(subject).to receive(:check_path) allow(Vagrant::Util::Subprocess).to receive(:execute) @@ -398,6 +407,7 @@ VF end it "raises a compatibility conflict error", skip_before: false, skip_after: true do + ensure_that_config_is_valid expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleCompatibilityModeConflict) end end @@ -986,6 +996,7 @@ VF end it "raises an error about the ansible version mismatch", skip_before: false, skip_after: true do + ensure_that_config_is_valid expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleVersionMismatch) end end @@ -1020,6 +1031,7 @@ VF it "raises an error when ansible-galaxy command fails", skip_before: true, skip_after: true do config.finalize! + ensure_that_config_is_valid allow(subject).to receive(:check_path) allow(Vagrant::Util::Subprocess).to receive(:execute)