diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bf0deb2e..a28e820a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,10 +109,8 @@ BUG FIXES: galaxy resources when running on a Windows host [GH-6740, GH-6757] - provisioners/ansible_local: Change the way to verify `ansible-galaxy` presence, to avoid a non-zero status code with Ansible 2.0 [GH-6793] - - provisioners/ansible_local: The configuration sanity checks now only warn - on missing files or directories, so that the requested vagrant command is - always executed (e.g. `vagrant destroy` is not aborted when the configured - playbook is not present on the guest) [GH-6763] + - provisioners/ansible(both provisioners): The Ansible configuration files + detection is only executed by the `provision` action [GH-6763, GH-6984] - provisioners/chef: Do not use double sudo when installing [GGH-6805, GH-6804] - provisioners/chef: Change the default channel to "stable" (previously it diff --git a/plugins/provisioners/ansible/config/base.rb b/plugins/provisioners/ansible/config/base.rb index 00b3f5072..67642467e 100644 --- a/plugins/provisioners/ansible/config/base.rb +++ b/plugins/provisioners/ansible/config/base.rb @@ -74,34 +74,15 @@ module VagrantPlugins @errors << I18n.t("vagrant.provisioners.ansible.errors.no_playbook") end - if playbook - check_path_is_a_file(machine, playbook, "vagrant.provisioners.ansible.errors.playbook_path_invalid") - end - - if inventory_path - check_path_exists(machine, inventory_path, "vagrant.provisioners.ansible.errors.inventory_path_invalid") - end - - if galaxy_role_file - check_path_is_a_file(machine, galaxy_role_file, "vagrant.provisioners.ansible.errors.galaxy_role_file_invalid") - end - - if vault_password_file - check_path_is_a_file(machine, vault_password_file, "vagrant.provisioners.ansible.errors.vault_password_file_invalid") - end - - # Validate that extra_vars is either a hash, or a path to an existing file + # Validate that extra_vars is either a Hash or a String (for a file path) if extra_vars extra_vars_is_valid = extra_vars.kind_of?(Hash) || extra_vars.kind_of?(String) if extra_vars.kind_of?(String) - # Accept the usage of '@' prefix in Vagrantfile (e.g. '@vars.yml' - # and 'vars.yml' are both supported) + # Accept the usage of '@' prefix in Vagrantfile + # (e.g. '@vars.yml' and 'vars.yml' are both supported) match_data = /^@?(.+)$/.match(extra_vars) extra_vars_path = match_data[1].to_s - extra_vars_is_valid = check_path_is_a_file(machine, extra_vars_path) - if extra_vars_is_valid - @extra_vars = '@' + extra_vars_path - end + @extra_vars = '@' + extra_vars_path end if !extra_vars_is_valid diff --git a/plugins/provisioners/ansible/config/guest.rb b/plugins/provisioners/ansible/config/guest.rb index 31507ad04..5f0086c4a 100644 --- a/plugins/provisioners/ansible/config/guest.rb +++ b/plugins/provisioners/ansible/config/guest.rb @@ -35,32 +35,6 @@ module VagrantPlugins { "ansible local provisioner" => @errors } end - protected - - def check_path(machine, path, test_args, error_message_key = nil) - remote_path = Helpers::expand_path_in_unix_style(path, @provisioning_path) - - if machine.communicate.ready? && !machine.communicate.test("test #{test_args} #{remote_path}") - if error_message_key - # only show warnings, as raising an error would abort the request - # vagrant action (e.g. prevent `destroy` to be executed) - machine.ui.warn(I18n.t(error_message_key, path: remote_path, system: "guest")) - end - return false - end - # when the machine is not ready for SSH communication, - # the check is "optimistically" bypassed. - true - end - - def check_path_is_a_file(machine, path, error_message_key = nil) - check_path(machine, path, "-f", error_message_key) - end - - def check_path_exists(machine, path, error_message_key = nil) - check_path(machine, path, "-e", error_message_key) - end - end end end diff --git a/plugins/provisioners/ansible/config/host.rb b/plugins/provisioners/ansible/config/host.rb index 04d2fac10..4e075ae64 100644 --- a/plugins/provisioners/ansible/config/host.rb +++ b/plugins/provisioners/ansible/config/host.rb @@ -48,27 +48,6 @@ module VagrantPlugins { "ansible remote provisioner" => @errors } end - protected - - def check_path(machine, path, path_test_method, error_message_key = nil) - expanded_path = Pathname.new(path).expand_path(machine.env.root_path) - if !expanded_path.public_send(path_test_method) - if error_message_key - @errors << I18n.t(error_message_key, path: expanded_path, system: "host") - end - return false - end - true - end - - def check_path_is_a_file(machine, path, error_message_key = nil) - check_path(machine, path, "file?", error_message_key) - end - - def check_path_exists(machine, path, error_message_key = nil) - check_path(machine, path, "exist?", error_message_key) - end - end end end diff --git a/plugins/provisioners/ansible/provisioner/base.rb b/plugins/provisioners/ansible/provisioner/base.rb index 6092ecfe3..786210474 100644 --- a/plugins/provisioners/ansible/provisioner/base.rb +++ b/plugins/provisioners/ansible/provisioner/base.rb @@ -25,6 +25,15 @@ module VagrantPlugins @inventory_path = nil end + def check_files_existence + check_path_is_a_file config.playbook, :playbook + + check_path_exists config.inventory_path, :inventory_path if config.inventory_path + check_path_is_a_file config.extra_vars[1..-1], :extra_vars if has_an_extra_vars_file_argument + check_path_is_a_file config.galaxy_role_file, :galaxy_role_file if config.galaxy_role_file + check_path_is_a_file config.vault_password_file, :vault_password if config.vault_password_file + end + def ansible_playbook_command_for_shell_execution shell_command = [] @environment_variables.each_pair do |k, v| @@ -198,8 +207,12 @@ module VagrantPlugins return inventory_groups end + def has_an_extra_vars_file_argument + config.extra_vars && config.extra_vars.kind_of?(String) && config.extra_vars =~ /^@.+$/ + end + def extra_vars_argument - if config.extra_vars.kind_of?(String) and config.extra_vars =~ /^@.+$/ + if has_an_extra_vars_file_argument # A JSON or YAML file is referenced. config.extra_vars else diff --git a/plugins/provisioners/ansible/provisioner/guest.rb b/plugins/provisioners/ansible/provisioner/guest.rb index 046806109..298f9cef3 100644 --- a/plugins/provisioners/ansible/provisioner/guest.rb +++ b/plugins/provisioners/ansible/provisioner/guest.rb @@ -14,6 +14,7 @@ module VagrantPlugins end def provision + check_files_existence check_and_install_ansible execute_ansible_galaxy_on_guest if config.galaxy_role_file execute_ansible_playbook_on_guest @@ -145,6 +146,31 @@ module VagrantPlugins end end + def check_path(path, test_args, option_name) + # Checks for the existence of given file (or directory) on the guest system, + # and error if it doesn't exist. + + remote_path = Helpers::expand_path_in_unix_style(path, config.provisioning_path) + command = "test #{test_args} #{remote_path}" + + @machine.communicate.execute( + command, + error_class: Ansible::Errors::AnsibleError, + error_key: :config_file_not_found, + config_option: option_name, + path: remote_path, + system: "guest" + ) + end + + def check_path_is_a_file(path, error_message_key) + check_path(path, "-f", error_message_key) + end + + def check_path_exists(path, error_message_key) + check_path(path, "-e", error_message_key) + end + end end end diff --git a/plugins/provisioners/ansible/provisioner/host.rb b/plugins/provisioners/ansible/provisioner/host.rb index 1eca11d45..f61024722 100644 --- a/plugins/provisioners/ansible/provisioner/host.rb +++ b/plugins/provisioners/ansible/provisioner/host.rb @@ -18,6 +18,7 @@ module VagrantPlugins # At this stage, the SSH access is guaranteed to be ready @ssh_info = @machine.ssh_info + check_files_existence warn_for_unsupported_platform execute_ansible_galaxy_from_host if config.galaxy_role_file execute_ansible_playbook_from_host @@ -257,6 +258,28 @@ module VagrantPlugins ssh_options.join(' ') end + def check_path(path, path_test_method, option_name) + # Checks for the existence of given file (or directory) on the host system, + # and error if it doesn't exist. + + expanded_path = Pathname.new(path).expand_path(@machine.env.root_path) + if !expanded_path.public_send(path_test_method) + raise Ansible::Errors::AnsibleError, + _key: :config_file_not_found, + config_option: option_name, + path: expanded_path, + system: "host" + end + end + + def check_path_is_a_file(path, option_name) + check_path(path, "file?", option_name) + end + + def check_path_exists(path, option_name) + check_path(path, "exist?", option_name) + end + end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 005c88fc5..1514c7a1f 100755 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -2128,22 +2128,16 @@ en: or adapt the `version` option of this provisioner in your Vagrantfile. See https://docs.vagrantup.com/v2/provisioning/ansible_local.html for more information. + config_file_not_found: |- + `%{config_option}` does not exist on the %{system}: %{path} extra_vars_invalid: |- `extra_vars` must be a hash or a path to an existing file. Received: %{value} (as %{type}) + no_playbook: |- + `playbook` file path must be set. raw_arguments_invalid: |- `raw_arguments` must be an array of strings. Received: %{value} (as %{type}) raw_ssh_args_invalid: |- `raw_ssh_args` must be an array of strings. Received: %{value} (as %{type}) - galaxy_role_file_invalid: |- - `galaxy_role_file` does not exist on the %{system}: %{path} - inventory_path_invalid: |- - `inventory_path` does not exist on the %{system}: %{path} - no_playbook: |- - `playbook` file path must be set. - playbook_path_invalid: |- - `playbook` does not exist on the %{system}: %{path} - vault_password_file_invalid: |- - `vault_password_file` does not exist on the %{system}: %{path} installing: "Installing Ansible..." running_galaxy: "Running ansible-galaxy..." running_playbook: "Running ansible-playbook..." diff --git a/test/unit/plugins/provisioners/ansible/config/guest_test.rb b/test/unit/plugins/provisioners/ansible/config/guest_test.rb index 56f1366c6..4368c7032 100644 --- a/test/unit/plugins/provisioners/ansible/config/guest_test.rb +++ b/test/unit/plugins/provisioners/ansible/config/guest_test.rb @@ -14,7 +14,6 @@ describe VagrantPlugins::Ansible::Config::Guest do let(:communicator) { double("communicator") } let(:existing_file) { "this/path/is/a/stub" } - let(:non_existing_file) { "this/path/does/not/exist" } it "supports a list of options" do supported_options = %w( extra_vars @@ -57,107 +56,10 @@ describe VagrantPlugins::Ansible::Config::Guest do describe "#validate" do before do - machine.stub(communicate: communicator) - end - - context "when the machine is not ready to communicate" do - before do - allow(communicator).to receive(:ready?).and_return(false) - end - - it "cannot check the existence of remote file" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible local provisioner"]).to eql([]) - # FIXME: commented out because this `communicator.ready?` stub is not working as expected - # expect(communicator).to receive(:ready?).ordered - # Note that communicator mock will fail if it receives an unexpected message, - # which is part of this spec. - end - end - - context "when the machine is ready to communicate" do - before do - allow(communicator).to receive(:ready?).and_return(true) - allow(communicator).to receive(:test).and_return(false) - - allow(communicator).to receive(:test) do |arg1| - arg1.include?("#{existing_file}") - end - - stubbed_ui = Vagrant::UI::Colored.new - machine.stub(ui: stubbed_ui) - allow(machine.ui).to receive(:warn) - - subject.playbook = existing_file - end - - it_behaves_like "an Ansible provisioner", "/vagrant", "local" - - it "only shows a warning if the playbook file does not exist" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine).to receive(:ui).with { |warning_text| - # expect(warning_text).to eq("`playbook` does not exist on the guest: /vagrant/this/path/does/not/exist") - # } - end - - it "only shows a warning if inventory_path is specified, but does not exist" do - subject.inventory_path = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine.ui).to receive(:warn).with { |warning_text| - # expect(warning_text).to eq("`inventory_path` does not exist on the guest: this/path/does/not/exist") - # } - end - - it "only shows a warning if vault_password_file is specified, but does not exist" do - subject.vault_password_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to be_nil - - # FIXME - # expect(machine.ui).to receive(:warn).with { |warning_text| - # expect(warning_text).to eq("`inventory_path` does not exist on the guest: this/path/does/not/exist") - # } - end - - it "it doesn't consider missing files on the remote system as errors" do - subject.playbook = non_existing_file - subject.inventory_path = non_existing_file - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible local provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s)) - - expect(result["ansible local provisioner"]).to_not include( - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: File.join("/vagrant", non_existing_file), system: "guest")) - - expect(result["ansible local provisioner"]).to_not include( - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: File.join("/vagrant", non_existing_file), system: "guest")) - end - + subject.playbook = existing_file end + it_behaves_like "an Ansible provisioner", "/vagrant", "local" end end diff --git a/test/unit/plugins/provisioners/ansible/config/host_test.rb b/test/unit/plugins/provisioners/ansible/config/host_test.rb index 8ebe6e35a..f6871c209 100644 --- a/test/unit/plugins/provisioners/ansible/config/host_test.rb +++ b/test/unit/plugins/provisioners/ansible/config/host_test.rb @@ -11,7 +11,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do let(:machine) { double("machine", env: Vagrant::Environment.new) } let(:existing_file) { File.expand_path(__FILE__) } - let(:non_existing_file) { "/this/does/not/exist" } it "supports a list of options" do supported_options = %w( ask_sudo_pass @@ -74,50 +73,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do it_behaves_like "an Ansible provisioner", "", "remote" - it "returns an error if the playbook file does not exist" do - subject.playbook = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if galaxy_role_file is specified, but does not exist" do - subject.galaxy_role_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.galaxy_role_file_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if inventory_path is specified, but does not exist" do - subject.inventory_path = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: non_existing_file, system: "host") - ]) - end - - it "returns an error if vault_password_file is specified, but does not exist" do - subject.vault_password_file = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.vault_password_file_invalid", - path: non_existing_file, system: "host") - ]) - end - it "returns an error if the raw_ssh_args is of the wrong data type" do subject.raw_ssh_args = { arg1: 1, arg2: "foo" } subject.finalize! @@ -138,25 +93,6 @@ describe VagrantPlugins::Ansible::Config::Host, :skip_windows => true do expect(subject.raw_arguments).to eql(["-o ControlMaster=no"]) end - it "it collects and returns all detected errors" do - subject.playbook = non_existing_file - subject.inventory_path = non_existing_file - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.playbook_path_invalid", - path: non_existing_file, system: "host")) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s)) - expect(result["ansible remote provisioner"]).to include( - I18n.t("vagrant.provisioners.ansible.errors.inventory_path_invalid", - path: non_existing_file, system: "host")) - end - end end diff --git a/test/unit/plugins/provisioners/ansible/config/shared.rb b/test/unit/plugins/provisioners/ansible/config/shared.rb index b3364e855..7ff56ee91 100644 --- a/test/unit/plugins/provisioners/ansible/config/shared.rb +++ b/test/unit/plugins/provisioners/ansible/config/shared.rb @@ -29,13 +29,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | provisioner_label = "ansible #{ansible_setup} provisioner" provisioner_system = ansible_setup == "local" ? "guest" : "host" - it "passes if the playbook option refers to an existing file" do - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) - end - it "returns an error if the playbook option is undefined" do subject.playbook = nil subject.finalize! @@ -46,14 +39,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "passes if the extra_vars option refers to an existing file" do - subject.extra_vars = existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) - end - it "passes if the extra_vars option is a hash" do subject.extra_vars = { var1: 1, var2: "foo" } subject.finalize! @@ -62,21 +47,6 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | expect(result[provisioner_label]).to eql([]) end - # FIXME: - # The guest-based config should actually NOT return this error, - # but only display a warning (similarly to GH-6764 and 4e451c6) - it "returns an error if the extra_vars option refers to a file that does not exist" do - subject.extra_vars = non_existing_file - subject.finalize! - - result = subject.validate(machine) - expect(result[provisioner_label]).to eql([ - I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", - type: subject.extra_vars.class.to_s, - value: subject.extra_vars.to_s) - ]) - end - it "returns an error if the extra_vars option is of wrong data type" do subject.extra_vars = ["var1", 3, "var2", 5] subject.finalize! @@ -89,12 +59,12 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "passes if inventory_path refers to an existing location" do - subject.inventory_path = existing_file + it "converts a raw_arguments option defined as a String into an Array" do + subject.raw_arguments = "--foo=bar" subject.finalize! result = subject.validate(machine) - expect(result[provisioner_label]).to eql([]) + expect(subject.raw_arguments).to eql(%w(--foo=bar)) end it "returns an error if the raw_arguments is of the wrong data type" do @@ -109,12 +79,25 @@ shared_examples_for 'an Ansible provisioner' do | path_prefix, ansible_setup | ]) end - it "converts a raw_arguments option defined as a String into an Array" do - subject.raw_arguments = "--foo=bar" + it "it collects and returns all detected errors" do + subject.playbook = nil + subject.extra_vars = ["var1", 3, "var2", 5] + subject.raw_arguments = { arg1: 1, arg2: "foo" } subject.finalize! result = subject.validate(machine) - expect(subject.raw_arguments).to eql(%w(--foo=bar)) + + expect(result[provisioner_label].size).to eql(3) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.no_playbook")) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.extra_vars_invalid", + type: subject.extra_vars.class.to_s, + value: subject.extra_vars.to_s)) + expect(result[provisioner_label]).to include( + I18n.t("vagrant.provisioners.ansible.errors.raw_arguments_invalid", + type: subject.raw_arguments.class.to_s, + value: subject.raw_arguments.to_s)) end describe "sudo option" do diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index e662cd959..4a916cae2 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -59,6 +59,8 @@ VF stubbed_ui.stub(detail: "") machine.env.stub(ui: stubbed_ui) + subject.stub(:check_path) + config.playbook = 'playbook.yml' end @@ -203,6 +205,43 @@ VF end end + describe 'checking existence of Ansible configuration files' do + + describe 'when the playbook file does not exist' do + it "raises an error", skip_before: true, skip_after: true do + + subject.stub(:check_path).and_raise(VagrantPlugins::Ansible::Errors::AnsibleError, + _key: :config_file_not_found, + config_option: "playbook", + path: "/home/wip/test/invalid_path.yml", + system: "host") + + config.playbook = "/home/wip/test/invalid_path.yml" + config.finalize! + + expect {subject.provision}.to raise_error(VagrantPlugins::Ansible::Errors::AnsibleError, + "`playbook` does not exist on the host: /home/wip/test/invalid_path.yml") + end + end + + describe 'when the inventory path does not exist' do + it "raises an error" + end + + describe 'when the extra_vars file does not exist' do + it "raises an error" + end + + describe 'when the galaxy_role_file does not exist' do + it "raises an error" + end + + describe 'when the vault_password_file does not exist' do + it "raises an error" + end + + end + describe 'when ansible-playbook fails' do it "raises an error", skip_before: true, skip_after: true do config.finalize!