provisioners/ansible(both): fix ansible config files presence checks

With this change, the presence of Ansible configuration files (like
playbook file, inventory path, galaxy role file, etc.) is no longer
performed by the `config` classes, but by the `provisioner` classes
(at the beginning of the provision command).

This change fixes several issues:

- Resolve #6984 as `provision` method are only executed when remote
  (ssh) communication with the guest machine is possible.
- Resolve #6763 in a better way than 4e451c6 initially did.
- Improve the general provisioner speed since the `config` checks are
  actually triggered by many vagrant actions (e.g. `destroy`,...), and
  can also be triggered multiple times during a vagrant run (e.g. on
  callback request made by the machine provider).

Unlike the former `config`-based checks, the provision action won't
collect all the invalid options, but only report the first invalid
option found and abort the execution.

Some unit tests were not implemented yet to save my scarce "open source
contribution time" for other important issues, but they should be done
at last via GH-6633.

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
+  - provisioners/ansible(both provisioners): The Ansible configuration files
-      on missing files or directories, so that the requested vagrant command is
+      detection is only executed by the `provision` action [GH-6763, GH-6984]
-      always executed (e.g. `vagrant destroy` is not aborted when the configured
-      playbook is not present on the guest) [GH-6763]
   - provisioners/chef: Do not use double sudo when installing
       [GGH-6805, GH-6804]
   - provisioners/chef: Change the default channel to "stable" (previously it

plugins/provisioners/ansible/config/base.rb

@@ -74,35 +74,16 @@ module VagrantPlugins
             @errors << I18n.t("vagrant.provisioners.ansible.errors.no_playbook")
           end
 
-          if playbook
+          # Validate that extra_vars is either a Hash or a String (for a file path)
-            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
           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'
+              # Accept the usage of '@' prefix in Vagrantfile
-              # and 'vars.yml' are both supported)
+              # (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
-            end
 
             if !extra_vars_is_valid
               @errors << I18n.t(

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

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

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

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

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

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..."

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
-
-    end
-
   end
 
 end

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

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
+  it "converts a raw_arguments option defined as a String into an Array" do
-    subject.inventory_path = existing_file
+    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
+  it "it collects and returns all detected errors" do
-    subject.raw_arguments = "--foo=bar"
+    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

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!