From f0a596b47cea33db66035c6922a11c3386538e77 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Fri, 21 Mar 2014 12:02:23 +0100 Subject: [PATCH 1/5] provisioners/ansible: add first unit tests --- .../provisioners/ansible/config_test.rb | 42 ++++ .../provisioners/ansible/provisioner_test.rb | 180 ++++++++++++++++++ 2 files changed, 222 insertions(+) create mode 100644 test/unit/plugins/provisioners/ansible/config_test.rb create mode 100644 test/unit/plugins/provisioners/ansible/provisioner_test.rb diff --git a/test/unit/plugins/provisioners/ansible/config_test.rb b/test/unit/plugins/provisioners/ansible/config_test.rb new file mode 100644 index 000000000..5d00b255f --- /dev/null +++ b/test/unit/plugins/provisioners/ansible/config_test.rb @@ -0,0 +1,42 @@ +require_relative "../../../base" + +require Vagrant.source_root.join("plugins/provisioners/ansible/config") + +describe VagrantPlugins::Ansible::Config do + include_context "unit" + + subject { described_class.new } + + let(:machine) { double("machine", env: Vagrant::Environment.new) } + let(:file_that_exists) { File.expand_path(__FILE__) } + + describe "#validate" do + it "returns an error if playbook file does not exist" do + non_existing_file = "/this/does/not/exist" + + subject.playbook = non_existing_file + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([ + I18n.t("vagrant.provisioners.ansible.playbook_path_invalid", + :path => non_existing_file) + ]) + end + + it "returns an error if inventory_path is specified, but does not exist" do + non_existing_file = "/this/does/not/exist" + + subject.playbook = file_that_exists + subject.inventory_path = non_existing_file + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([ + I18n.t("vagrant.provisioners.ansible.inventory_path_invalid", + :path => non_existing_file) + ]) + end + + end +end diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb new file mode 100644 index 000000000..6ec3eba31 --- /dev/null +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -0,0 +1,180 @@ +require_relative "../../../base" + +require Vagrant.source_root.join("plugins/provisioners/ansible/provisioner") + +describe VagrantPlugins::Ansible::Provisioner do + include_context "unit" + + subject { described_class.new(machine, config) } + + let(:iso_env) do + # We have to create a Vagrantfile so there is a Vagrant Environment + env = isolated_environment + env.vagrantfile("") + env.create_vagrant_env + end + + let(:machine) { iso_env.machine(iso_env.machine_names[0], :dummy) } + let(:config) { double("config") } + + let(:ssh_info) {{ + private_key_path: ['/path/to/my/key'], + username: 'testuser' + }} + + before do + machine.stub(ssh_info: ssh_info) + + config.stub(playbook: 'playbook.yml') + config.stub(extra_vars: nil) + config.stub(inventory_path: nil) + config.stub(ask_sudo_pass: nil) + config.stub(limit: nil) + config.stub(sudo: nil) + config.stub(sudo_user: nil) + config.stub(verbose: nil) + config.stub(tags: nil) + config.stub(skip_tags: nil) + config.stub(start_at_task: nil) + config.stub(groups: {}) + config.stub(host_key_checking: 'false') + config.stub(raw_arguments: nil) + config.stub(raw_ssh_args: nil) + end + + shared_examples_for "an ansible-playbook subprocess" do + it "sets default arguments" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + + index = args.find_index("ansible-playbook") + expect(index).to eql(0) + index = args.find_index("--private-key=/path/to/my/key") + expect(index).to eql(1) + index = args.find_index("--user=testuser") + expect(index).to eql(2) + + index = args.find_index("--limit=#{machine.name}") + expect(index).to be > 0 + + index = args.find_index("playbook.yml") + expect(index).to eql(args.length-2) + } + subject.provision + end + + it "exports default environment variables" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + expect(cmd_opts[:env]['ANSIBLE_FORCE_COLOR']).to eql("true") + expect(cmd_opts[:env]['ANSIBLE_HOST_KEY_CHECKING']).to eql("false") + expect(cmd_opts[:env]['PYTHONUNBUFFERED']).to eql(1) + } + subject.provision + end + end + + shared_examples_for "SSH transport mode is not forced" do + it "does not export ANSIBLE_SSH_ARGS" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to be_nil + } + subject.provision + end + it "does not force SSH transport mode" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + index = args.find_index("--connection=ssh") + expect(index).to be_nil + } + subject.provision + end + end + + shared_examples_for "SSH transport mode is forced" do + it "sets --connection=ssh argument" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + index = args.find_index("--connection=ssh") + expect(index).to be > 0 + } + subject.provision + end + it "enables ControlPersist (like Ansible defaults) via ANSIBLE_SSH_ARGS" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ControlMaster=auto") + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ControlPersist=60s") + } + subject.provision + end + end + + describe "#provision" do + + let(:result) { Vagrant::Util::Subprocess::Result.new(0, "", "") } + + before do + Vagrant::Util::Subprocess.stub(execute: result) + end + + it "doesn't raise an error if it succeeds" do + subject.provision + end + + it "raises an error if the exit code is non-zero" do + Vagrant::Util::Subprocess.stub( + execute: Vagrant::Util::Subprocess::Result.new(1, "", "")) + + expect {subject.provision}. + to raise_error(Vagrant::Errors::AnsibleFailed) + end + + describe "with default options" do + it_behaves_like 'an ansible-playbook subprocess' + it_behaves_like 'SSH transport mode is not forced' + + it "generates the inventory" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + index = args.find_index("--inventory-file=#{File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory))}") + expect(index).to be > 0 + } + subject.provision + end + end + + describe "with multiple SSH identities" do + before do + ssh_info[:private_key_path] = ['/path/to/my/key', '/an/other/identity', '/yet/an/other/key'] + end + + it_behaves_like 'an ansible-playbook subprocess' + it_behaves_like 'SSH transport mode is forced' + + it "passes additional Identity Files via ANSIBLE_SSH_ARGS" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o IdentityFile=/an/other/identity") + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o IdentityFile=/yet/an/other/key") + } + subject.provision + end + end + + describe "with ssh forwarding enabled" do + before do + ssh_info[:forward_agent] = true + end + + it_behaves_like 'an ansible-playbook subprocess' + it_behaves_like 'SSH transport mode is forced' + + it "enables SSH-Forwarding via ANSIBLE_SSH_ARGS" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ForwardAgent=yes") + } + subject.provision + end + end + + end +end From e32783312ba801ce72ea23b2f7e1019c14d71765 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Sat, 22 Mar 2014 17:18:00 +0100 Subject: [PATCH 2/5] provisioners/ansible: improve unit tests (wip) - Don't mock the config object, but use a true instance - When possible, take advantage of Rpsec before/after hooks to reduce code repetitions - Add an example ("with inventory_path option") - Use a global variable to store the path of the generated inventory - Miscellaneous changes in existing examples (style, fixes) --- .../provisioners/ansible/provisioner_test.rb | 95 +++++++++---------- 1 file changed, 47 insertions(+), 48 deletions(-) diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 6ec3eba31..746efe709 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -1,5 +1,6 @@ require_relative "../../../base" +require Vagrant.source_root.join("plugins/provisioners/ansible/config") require Vagrant.source_root.join("plugins/provisioners/ansible/provisioner") describe VagrantPlugins::Ansible::Provisioner do @@ -15,7 +16,8 @@ describe VagrantPlugins::Ansible::Provisioner do end let(:machine) { iso_env.machine(iso_env.machine_names[0], :dummy) } - let(:config) { double("config") } + let(:config) { VagrantPlugins::Ansible::Config.new } + let(:file_that_exists) { File.expand_path(__FILE__) } let(:ssh_info) {{ private_key_path: ['/path/to/my/key'], @@ -25,41 +27,24 @@ describe VagrantPlugins::Ansible::Provisioner do before do machine.stub(ssh_info: ssh_info) - config.stub(playbook: 'playbook.yml') - config.stub(extra_vars: nil) - config.stub(inventory_path: nil) - config.stub(ask_sudo_pass: nil) - config.stub(limit: nil) - config.stub(sudo: nil) - config.stub(sudo_user: nil) - config.stub(verbose: nil) - config.stub(tags: nil) - config.stub(skip_tags: nil) - config.stub(start_at_task: nil) - config.stub(groups: {}) - config.stub(host_key_checking: 'false') - config.stub(raw_arguments: nil) - config.stub(raw_ssh_args: nil) + config.playbook = 'playbook.yml' + + $generated_inventory_file = File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory)) end shared_examples_for "an ansible-playbook subprocess" do it "sets default arguments" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.find_index("ansible-playbook") - expect(index).to eql(0) - index = args.find_index("--private-key=/path/to/my/key") - expect(index).to eql(1) - index = args.find_index("--user=testuser") - expect(index).to eql(2) + expect(args[0]).to eq("ansible-playbook") + expect(args[1]).to eq("--private-key=/path/to/my/key") + expect(args[2]).to eq("--user=testuser") - index = args.find_index("--limit=#{machine.name}") + index = args.index("--limit=#{machine.name}") expect(index).to be > 0 - index = args.find_index("playbook.yml") - expect(index).to eql(args.length-2) + expect(args[args.length-2]).to eq("playbook.yml") } - subject.provision end it "exports default environment variables" do @@ -69,7 +54,6 @@ describe VagrantPlugins::Ansible::Provisioner do expect(cmd_opts[:env]['ANSIBLE_HOST_KEY_CHECKING']).to eql("false") expect(cmd_opts[:env]['PYTHONUNBUFFERED']).to eql(1) } - subject.provision end end @@ -79,24 +63,21 @@ describe VagrantPlugins::Ansible::Provisioner do cmd_opts = args.last expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to be_nil } - subject.provision end it "does not force SSH transport mode" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.find_index("--connection=ssh") + index = args.index("--connection=ssh") expect(index).to be_nil } - subject.provision end end shared_examples_for "SSH transport mode is forced" do it "sets --connection=ssh argument" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.find_index("--connection=ssh") + index = args.index("--connection=ssh") expect(index).to be > 0 } - subject.provision end it "enables ControlPersist (like Ansible defaults) via ANSIBLE_SSH_ARGS" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| @@ -104,40 +85,60 @@ describe VagrantPlugins::Ansible::Provisioner do expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ControlMaster=auto") expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ControlPersist=60s") } - subject.provision end end describe "#provision" do - let(:result) { Vagrant::Util::Subprocess::Result.new(0, "", "") } - before do - Vagrant::Util::Subprocess.stub(execute: result) + unless example.metadata[:skip_before] + config.finalize! + Vagrant::Util::Subprocess.stub(execute: Vagrant::Util::Subprocess::Result.new(0, "", "")) + end + end + + after do + unless example.metadata[:skip_after] + subject.provision + end end it "doesn't raise an error if it succeeds" do - subject.provision end - it "raises an error if the exit code is non-zero" do - Vagrant::Util::Subprocess.stub( - execute: Vagrant::Util::Subprocess::Result.new(1, "", "")) + it "raises an error if the exit code is non-zero", skip_before: true, skip_after: true do + config.finalize! + Vagrant::Util::Subprocess.stub(execute: Vagrant::Util::Subprocess::Result.new(1, "", "")) - expect {subject.provision}. - to raise_error(Vagrant::Errors::AnsibleFailed) + expect {subject.provision}.to raise_error(Vagrant::Errors::AnsibleFailed) end describe "with default options" do + it_behaves_like 'an ansible-playbook subprocess' it_behaves_like 'SSH transport mode is not forced' - it "generates the inventory" do + it "generates an inventory file and uses it" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.find_index("--inventory-file=#{File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory))}") - expect(index).to be > 0 + expect(File.exists?($generated_inventory_file)).to be_true + expect(args.include?("--inventory-file=#{$generated_inventory_file}")).to be_true + } + end + end + + describe "with inventory_path option" do + before do + config.inventory_path = file_that_exists + end + + it_behaves_like 'an ansible-playbook subprocess' + it_behaves_like 'SSH transport mode is not forced' + + it "uses given inventory path" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args.include?("--inventory-file=#{file_that_exists}")).to be_true + expect(args.include?("--inventory-file=#{$generated_inventory_file}")).to be_false } - subject.provision end end @@ -155,7 +156,6 @@ describe VagrantPlugins::Ansible::Provisioner do expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o IdentityFile=/an/other/identity") expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o IdentityFile=/yet/an/other/key") } - subject.provision end end @@ -172,7 +172,6 @@ describe VagrantPlugins::Ansible::Provisioner do cmd_opts = args.last expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ForwardAgent=yes") } - subject.provision end end From baf0649dcfa1db203b8fe4c0404d1428d8c2a537 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Sat, 29 Mar 2014 09:19:26 +0100 Subject: [PATCH 3/5] provisioners/ansible: add more unit tests Remove wrong usage of shared examples and introduce embedded class methods (as kind of simple "RSpec macros") to reduce code duplication. --- .../provisioners/ansible/config_test.rb | 138 ++++++- .../provisioners/ansible/provisioner_test.rb | 360 +++++++++++++++--- 2 files changed, 446 insertions(+), 52 deletions(-) diff --git a/test/unit/plugins/provisioners/ansible/config_test.rb b/test/unit/plugins/provisioners/ansible/config_test.rb index 5d00b255f..2a9f25fad 100644 --- a/test/unit/plugins/provisioners/ansible/config_test.rb +++ b/test/unit/plugins/provisioners/ansible/config_test.rb @@ -8,12 +8,73 @@ describe VagrantPlugins::Ansible::Config do subject { described_class.new } let(:machine) { double("machine", env: Vagrant::Environment.new) } - let(:file_that_exists) { File.expand_path(__FILE__) } + let(:existing_file) { File.expand_path(__FILE__) } + let(:non_existing_file) { "/this/does/not/exist" } + + it "supports a list of options" do + config_options = subject.public_methods(false).find_all { |i| i.to_s.end_with?('=') } + config_options.map! { |i| i.to_s.sub('=', '') } + supported_options = %w( ask_sudo_pass + extra_vars + groups + host_key_checking + inventory_path + limit + playbook + raw_arguments + raw_ssh_args + skip_tags + start_at_task + sudo + sudo_user + tags + verbose ) + + expect(config_options.sort).to eql(supported_options) + end + + it "assigns default values to unset options" do + subject.finalize! + + expect(subject.playbook).to be_nil + expect(subject.extra_vars).to be_nil + expect(subject.ask_sudo_pass).to be_nil + expect(subject.limit).to be_nil + expect(subject.sudo).to be_nil + expect(subject.sudo_user).to be_nil + expect(subject.verbose).to be_nil + expect(subject.tags).to be_nil + expect(subject.skip_tags).to be_nil + expect(subject.start_at_task).to be_nil + expect(subject.groups).to eq({}) + expect(subject.host_key_checking).to eq('false') + expect(subject.raw_arguments).to be_nil + expect(subject.raw_ssh_args).to be_nil + end describe "#validate" do - it "returns an error if playbook file does not exist" do - non_existing_file = "/this/does/not/exist" + before do + subject.playbook = existing_file + end + it "passes if the playbook option refers to an existing file" do + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([]) + end + + it "returns an error if the playbook option is undefined" do + subject.playbook = nil + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([ + I18n.t("vagrant.provisioners.ansible.no_playbook") + ]) + end + + it "returns an error if the playbook file does not exist" do subject.playbook = non_existing_file subject.finalize! @@ -24,10 +85,55 @@ describe VagrantPlugins::Ansible::Config do ]) end - it "returns an error if inventory_path is specified, but does not exist" do - non_existing_file = "/this/does/not/exist" + it "passes if the extra_vars option refers to an existing file" do + subject.extra_vars = existing_file + subject.finalize! - subject.playbook = file_that_exists + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([]) + end + + it "passes if the extra_vars option is a hash" do + subject.extra_vars = { var1: 1, var2: "foo" } + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([]) + end + + 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["ansible provisioner"]).to eql([ + I18n.t("vagrant.provisioners.ansible.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! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([ + I18n.t("vagrant.provisioners.ansible.extra_vars_invalid", + :type => subject.extra_vars.class.to_s, + :value => subject.extra_vars.to_s) + ]) + end + + it "passes if inventory_path refers to an existing location" do + subject.inventory_path = existing_file + subject.finalize! + + result = subject.validate(machine) + expect(result["ansible provisioner"]).to eql([]) + end + + it "returns an error if inventory_path is specified, but does not exist" do subject.inventory_path = non_existing_file subject.finalize! @@ -38,5 +144,25 @@ describe VagrantPlugins::Ansible::Config do ]) 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 provisioner"]).to include( + I18n.t("vagrant.provisioners.ansible.playbook_path_invalid", + :path => non_existing_file)) + expect(result["ansible provisioner"]).to include( + I18n.t("vagrant.provisioners.ansible.extra_vars_invalid", + :type => subject.extra_vars.class.to_s, + :value => subject.extra_vars.to_s)) + expect(result["ansible provisioner"]).to include( + I18n.t("vagrant.provisioners.ansible.inventory_path_invalid", + :path => non_existing_file)) + end + end + end diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 746efe709..692b4e0b3 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -3,83 +3,147 @@ require_relative "../../../base" require Vagrant.source_root.join("plugins/provisioners/ansible/config") require Vagrant.source_root.join("plugins/provisioners/ansible/provisioner") +# +# Helper Functions +# + +def find_last_argument_after(ref_index, ansible_playbook_args, arg_pattern) + subset = ansible_playbook_args[(ref_index + 1)..(ansible_playbook_args.length-2)].reverse + subset.each do |i| + return true if i =~ arg_pattern + end + return false +end + describe VagrantPlugins::Ansible::Provisioner do include_context "unit" subject { described_class.new(machine, config) } let(:iso_env) do - # We have to create a Vagrantfile so there is a Vagrant Environment + # We have to create a Vagrantfile so there is a Vagrant Environment to provide: + # - a location for the generated inventory + # - multi-machines configuration + env = isolated_environment - env.vagrantfile("") + env.vagrantfile(<<-VF) +Vagrant.configure("2") do |config| + config.vm.box = "base" + config.vm.define :machine1 + config.vm.define :machine2 +end +VF env.create_vagrant_env end let(:machine) { iso_env.machine(iso_env.machine_names[0], :dummy) } let(:config) { VagrantPlugins::Ansible::Config.new } - let(:file_that_exists) { File.expand_path(__FILE__) } - let(:ssh_info) {{ private_key_path: ['/path/to/my/key'], - username: 'testuser' + username: 'testuser', + host: '127.0.0.1', + port: 2223 }} + let(:existing_file) { File.expand_path(__FILE__) } + let(:generated_inventory_file) { File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory)) } + before do machine.stub(ssh_info: ssh_info) + machine.env.stub(active_machines: [[iso_env.machine_names[0], :dummy], [iso_env.machine_names[1], :dummy]]) config.playbook = 'playbook.yml' - - $generated_inventory_file = File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory)) end - shared_examples_for "an ansible-playbook subprocess" do - it "sets default arguments" do + # + # Class methods for code reuse across examples + # + + def self.it_should_set_arguments_and_environment_variables(expected_args_count = 5, expected_vars_count = 3, expected_host_key_checking = false) + it "sets implicit arguments in a specific order" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| expect(args[0]).to eq("ansible-playbook") - expect(args[1]).to eq("--private-key=/path/to/my/key") - expect(args[2]).to eq("--user=testuser") + expect(args[1]).to eq("--private-key=#{machine.ssh_info[:private_key_path][0]}") + expect(args[2]).to eq("--user=#{machine.ssh_info[:username]}") - index = args.index("--limit=#{machine.name}") - expect(index).to be > 0 + limit_index = args.index("--limit=#{machine.name}") + expect(limit_index).to be > 2 + expect(limit_index).to be < 5 + if (limit_index == 4) + expect(args[3]).to match("--connection=ssh") + end + + inventory_count = args.count { |x| x =~ /--inventory-file=.+/ } + expect(inventory_count).to be > 0 expect(args[args.length-2]).to eq("playbook.yml") } end - it "exports default environment variables" do + it "exports environment variables" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| cmd_opts = args.last expect(cmd_opts[:env]['ANSIBLE_FORCE_COLOR']).to eql("true") - expect(cmd_opts[:env]['ANSIBLE_HOST_KEY_CHECKING']).to eql("false") + expect(cmd_opts[:env]['ANSIBLE_HOST_KEY_CHECKING']).to eql(expected_host_key_checking.to_s) expect(cmd_opts[:env]['PYTHONUNBUFFERED']).to eql(1) } end + + # "roughly" verify that only expected args/vars have been defined by the provisioner + it "sets the expected number of arguments and environment variables" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args.length-2).to eq(expected_args_count) + expect(args.last[:env].length).to eq(expected_vars_count) + } + end end - shared_examples_for "SSH transport mode is not forced" do + def self.it_should_set_optional_arguments(arg_map) + it "sets optional arguments" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + arg_map.each_pair do |vagrant_option, ansible_argument| + index = args.index(ansible_argument) + if config.send(vagrant_option) + expect(index).to be > 0 + else + expect(index).to be_nil + end + end + } + end + end + + def self.it_should_use_smart_transport_mode it "does not export ANSIBLE_SSH_ARGS" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| cmd_opts = args.last expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to be_nil } end - it "does not force SSH transport mode" do + + it "does not force any transport mode" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.index("--connection=ssh") - expect(index).to be_nil + total = args.count { |x| x =~ /--connection=\w+/ } + expect(total).to eql(0) } end end - shared_examples_for "SSH transport mode is forced" do - it "sets --connection=ssh argument" do + def self.it_should_use_transport_mode(transport_mode) + it "it enables '#{transport_mode}' transport mode" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - index = args.index("--connection=ssh") + index = args.rindex("--connection=#{transport_mode}") expect(index).to be > 0 + expect(find_last_argument_after(index, args, /--connection=\w+/)).to be_false } end - it "enables ControlPersist (like Ansible defaults) via ANSIBLE_SSH_ARGS" do + end + + def self.it_should_force_ssh_transport_mode + it_should_use_transport_mode('ssh') + + it "configures ControlPersist (like Ansible defaults) via ANSIBLE_SSH_ARGS" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| cmd_opts = args.last expect(cmd_opts[:env]['ANSIBLE_SSH_ARGS']).to include("-o ControlMaster=auto") @@ -88,6 +152,26 @@ describe VagrantPlugins::Ansible::Provisioner do end end + def self.it_should_create_and_use_generated_inventory + it "generates an inventory with all active machines" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(config.inventory_path).to be_nil + expect(File.exists?(generated_inventory_file)).to be_true + inventory_content = File.read(generated_inventory_file) + expect(inventory_content).to include("#{machine.name} ansible_ssh_host=#{machine.ssh_info[:host]} ansible_ssh_port=#{machine.ssh_info[:port]}\n") + expect(inventory_content).to include("# MISSING: '#{iso_env.machine_names[1]}' machine was probably removed without using Vagrant. This machine should be recreated.\n") + } + end + + it "uses the auto-generated inventory file" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + inventory_index = args.rindex("--inventory-file=#{generated_inventory_file}") + expect(inventory_index).to be > 0 + expect(find_last_argument_after(inventory_index, args, /--inventory-file=\w+/)).to be_false + } + end + end + describe "#provision" do before do @@ -103,52 +187,201 @@ describe VagrantPlugins::Ansible::Provisioner do end end - it "doesn't raise an error if it succeeds" do - end + describe 'when ansible-playbook fails' do + it "raises an error", skip_before: true, skip_after: true do + config.finalize! + Vagrant::Util::Subprocess.stub(execute: Vagrant::Util::Subprocess::Result.new(1, "", "")) - it "raises an error if the exit code is non-zero", skip_before: true, skip_after: true do - config.finalize! - Vagrant::Util::Subprocess.stub(execute: Vagrant::Util::Subprocess::Result.new(1, "", "")) - - expect {subject.provision}.to raise_error(Vagrant::Errors::AnsibleFailed) + expect {subject.provision}.to raise_error(Vagrant::Errors::AnsibleFailed) + end end describe "with default options" do + it_should_set_arguments_and_environment_variables + it_should_use_smart_transport_mode + it_should_create_and_use_generated_inventory - it_behaves_like 'an ansible-playbook subprocess' - it_behaves_like 'SSH transport mode is not forced' - - it "generates an inventory file and uses it" do + it "does not add any group section to the generated inventory" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - expect(File.exists?($generated_inventory_file)).to be_true - expect(args.include?("--inventory-file=#{$generated_inventory_file}")).to be_true + inventory_content = File.read(generated_inventory_file) + expect(inventory_content).to_not match(/^\s*\[^\\+\]\s*$/) + + # Note: + # The expectation below is a workaround to a possible misuse (or bug) in RSpec/Ruby stack. + # If 'args' variable is not required by in this block, the "Vagrant::Util::Subprocess).to receive" + # surprisingly expects to receive "no args". + # This problem can be "solved" by using args the "unnecessary" (but harmless) expectation below: + expect(args.length).to be > 0 + } + end + end + + describe "with groups option" do + it_should_create_and_use_generated_inventory + + it "adds group sections to the generated inventory" do + config.groups = { + "group1" => "#{machine.name}", + "group1:children" => 'bar', + "group2" => ["iso_env.machine_names[1]"], + "group3" => ["unknown", "#{machine.name}"], + "bar" => ["#{machine.name}", "group3"], + "bar:children" => ["group1", "group2", "group3", "group4"], + "bar:vars" => ["myvar=foo"], + } + + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + inventory_content = File.read(generated_inventory_file) + + # Group variables are intentionally not supported in generated inventory + expect(inventory_content).not_to match(/^\[.*:vars\]$/) + + # Accept String instead of Array for group that contains a single item + expect(inventory_content).to include("[group1]\n#{machine.name}\n") + expect(inventory_content).to include("[group1:children]\nbar\n") + + # Skip "lost" machines + expect(inventory_content).to include("[group2]\n\n") + + # Skip "unknown" machines + expect(inventory_content).to include("[group3]\n#{machine.name}\n") + + # Don't mix group names and host names + expect(inventory_content).to include("[bar]\n#{machine.name}\n") + + # A group of groups only includes declared groups + expect(inventory_content).not_to match(/^group4$/) + expect(inventory_content).to include("[bar:children]\ngroup1\ngroup2\ngroup3\n") + } + end + end + + describe "with host_key_checking option enabled" do + before do + config.host_key_checking = "true" + end + + it_should_set_arguments_and_environment_variables 5, 3, true + it_should_use_smart_transport_mode + end + + describe "with boolean (flag) options disabled" do + before do + config.sudo = false + config.ask_sudo_pass = false + + config.sudo_user = 'root' + end + + it_should_set_arguments_and_environment_variables 6 + it_should_set_optional_arguments({ "sudo_user" => "--sudo-user=root" }) + + it "it does not set boolean flag when corresponding option is set to false" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args.index("--sudo")).to be_nil + expect(args.index("--ask-sudo-pass")).to be_nil + } + end + end + + describe "with raw_arguments option" do + before do + config.sudo = false + config.skip_tags = %w(foo bar) + config.raw_arguments = ["--connection=paramiko", + "--skip-tags=ignored", + "--module-path=/other/modules", + "--sudo", + "--inventory-file=/forget/it/my/friend", + "--new-arg=yeah"] + end + + it_should_set_arguments_and_environment_variables 12 + it_should_create_and_use_generated_inventory + it_should_use_transport_mode('paramiko') + + it "sets all raw arguments" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + config.raw_arguments.each do |raw_arg| + expect(args).to include(raw_arg) + end + } + end + + it "sets raw arguments before arguments related to supported options" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args.index("--skip-tags=foo,bar")).to be > args.index("--skip-tags=ignored") + } + end + + it "sets boolean flag (e.g. --sudo) defined in raw_arguments, even if corresponding option is set to false" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args).to include('--sudo') + } + end + + end + + describe "with limit option" do + before do + config.limit = %w(foo !bar) + end + + it_should_set_arguments_and_environment_variables 6 + it_should_set_optional_arguments({ "limit" => "--limit=foo,!bar" }) + + it "sets custom limit argument after implicit default limit" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + expect(args.index("--limit=foo,!bar")).to be > args.index("--limit=#{machine.name}") } end end describe "with inventory_path option" do before do - config.inventory_path = file_that_exists + config.inventory_path = existing_file end - it_behaves_like 'an ansible-playbook subprocess' - it_behaves_like 'SSH transport mode is not forced' + it_should_set_arguments_and_environment_variables + it_should_use_smart_transport_mode - it "uses given inventory path" do + it "does not generate the inventory and uses given inventory path instead" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - expect(args.include?("--inventory-file=#{file_that_exists}")).to be_true - expect(args.include?("--inventory-file=#{$generated_inventory_file}")).to be_false + expect(args).to include("--inventory-file=#{existing_file}") + expect(args).not_to include("--inventory-file=#{generated_inventory_file}") + expect(File.exists?(generated_inventory_file)).to be_false } end end + describe "with raw_ssh_args" do + before do + config.raw_ssh_args = ['-o ControlMaster=no'] + end + + it_should_set_arguments_and_environment_variables 6, 4 + it_should_force_ssh_transport_mode + + it "passes custom SSH options via ANSIBLE_SSH_ARGS with the highest priority" do + expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| + cmd_opts = args.last + raw_opt_index = cmd_opts[:env]['ANSIBLE_SSH_ARGS'].index("-o ControlMaster=no") + default_opt_index = cmd_opts[:env]['ANSIBLE_SSH_ARGS'].index("-o ControlMaster=auto") + expect(raw_opt_index).not_to be_nil + expect(default_opt_index).not_to be_nil + expect(raw_opt_index).to be < default_opt_index + } + end + + end + describe "with multiple SSH identities" do before do ssh_info[:private_key_path] = ['/path/to/my/key', '/an/other/identity', '/yet/an/other/key'] end - it_behaves_like 'an ansible-playbook subprocess' - it_behaves_like 'SSH transport mode is forced' + it_should_set_arguments_and_environment_variables 6, 4 + it_should_force_ssh_transport_mode it "passes additional Identity Files via ANSIBLE_SSH_ARGS" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| @@ -164,8 +397,8 @@ describe VagrantPlugins::Ansible::Provisioner do ssh_info[:forward_agent] = true end - it_behaves_like 'an ansible-playbook subprocess' - it_behaves_like 'SSH transport mode is forced' + it_should_set_arguments_and_environment_variables 6, 4 + it_should_force_ssh_transport_mode it "enables SSH-Forwarding via ANSIBLE_SSH_ARGS" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| @@ -175,5 +408,40 @@ describe VagrantPlugins::Ansible::Provisioner do end end + # Note: + # The Vagrant Ansible provisioner does not validate the coherency of arguments combination, + # and let ansible-playbook complaign. + describe "with a maximum of options" do + before do + # command line arguments + config.extra_vars = "@#{existing_file}" + config.sudo = true + config.sudo_user = 'deployer' + config.verbose = "vvv" + config.ask_sudo_pass = true + config.tags = %w(db www) + config.skip_tags = %w(foo bar) + config.limit = 'machine*:&vagrant:!that_one' + config.start_at_task = 'an awesome task' + + # environment variables + config.host_key_checking = true + config.raw_ssh_args = ['-o ControlMaster=no'] + end + + it_should_set_arguments_and_environment_variables 15, 4, true + it_should_force_ssh_transport_mode + it_should_set_optional_arguments({ "extra_vars" => "--extra-vars=@#{File.expand_path(__FILE__)}", + "sudo" => "--sudo", + "sudo_user" => "--sudo-user=deployer", + "verbose" => "-vvv", + "ask_sudo_pass" => "--ask-sudo-pass", + "tags" => "--tags=db,www", + "skip_tags" => "--skip-tags=foo,bar", + "limit" => "--limit=machine*:&vagrant:!that_one", + "start_at_task" => '--start-at-task=an awesome task', + }) + end + end end From ad038890bb3a9d4077e650b40c927c40f00af680 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Sat, 12 Apr 2014 08:29:40 +0200 Subject: [PATCH 4/5] provisioners/ansible: update specs after [GH-3436] --- test/unit/plugins/provisioners/ansible/provisioner_test.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 692b4e0b3..9a4d3695f 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -46,7 +46,8 @@ VF }} let(:existing_file) { File.expand_path(__FILE__) } - let(:generated_inventory_file) { File.join(machine.env.local_data_path, %w(provisioners ansible inventory vagrant_ansible_inventory)) } + let(:generated_inventory_dir) { File.join(machine.env.local_data_path, %w(provisioners ansible inventory)) } + let(:generated_inventory_file) { File.join(generated_inventory_dir, 'vagrant_ansible_inventory') } before do machine.stub(ssh_info: ssh_info) @@ -163,9 +164,9 @@ VF } end - it "uses the auto-generated inventory file" do + it "sets as ansible inventory the directory containing the auto-generated inventory file" do expect(Vagrant::Util::Subprocess).to receive(:execute).with { |*args| - inventory_index = args.rindex("--inventory-file=#{generated_inventory_file}") + inventory_index = args.rindex("--inventory-file=#{generated_inventory_dir}") expect(inventory_index).to be > 0 expect(find_last_argument_after(inventory_index, args, /--inventory-file=\w+/)).to be_false } From 7ed17ae9ed792340d020e783a7506401f5d9afc1 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Sat, 12 Apr 2014 11:00:34 +0200 Subject: [PATCH 5/5] provisioners/ansible: use strict boolean options With this change, the ansible provisioner fully complies with the current user documentation. --- plugins/provisioners/ansible/config.rb | 8 +++--- .../provisioners/ansible/config_test.rb | 17 +++++++++-- .../provisioners/ansible/provisioner_test.rb | 2 +- .../provisioners/support/shared/config.rb | 28 +++++++++++++++++++ 4 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 test/unit/plugins/provisioners/support/shared/config.rb diff --git a/plugins/provisioners/ansible/config.rb b/plugins/provisioners/ansible/config.rb index f702cf827..79ddcce30 100644 --- a/plugins/provisioners/ansible/config.rb +++ b/plugins/provisioners/ansible/config.rb @@ -33,7 +33,7 @@ module VagrantPlugins @skip_tags = UNSET_VALUE @start_at_task = UNSET_VALUE @groups = UNSET_VALUE - @host_key_checking = "false" + @host_key_checking = UNSET_VALUE @raw_arguments = UNSET_VALUE @raw_ssh_args = UNSET_VALUE end @@ -42,16 +42,16 @@ module VagrantPlugins @playbook = nil if @playbook == UNSET_VALUE @extra_vars = nil if @extra_vars == UNSET_VALUE @inventory_path = nil if @inventory_path == UNSET_VALUE - @ask_sudo_pass = nil if @ask_sudo_pass == UNSET_VALUE + @ask_sudo_pass = false unless @ask_sudo_pass == true @limit = nil if @limit == UNSET_VALUE - @sudo = nil if @sudo == UNSET_VALUE + @sudo = false unless @sudo == true @sudo_user = nil if @sudo_user == UNSET_VALUE @verbose = nil if @verbose == UNSET_VALUE @tags = nil if @tags == UNSET_VALUE @skip_tags = nil if @skip_tags == UNSET_VALUE @start_at_task = nil if @start_at_task == UNSET_VALUE @groups = {} if @groups == UNSET_VALUE - @host_key_checking = nil if @host_key_checking == UNSET_VALUE + @host_key_checking = false unless @host_key_checking == true @raw_arguments = nil if @raw_arguments == UNSET_VALUE @raw_ssh_args = nil if @raw_ssh_args == UNSET_VALUE end diff --git a/test/unit/plugins/provisioners/ansible/config_test.rb b/test/unit/plugins/provisioners/ansible/config_test.rb index 2a9f25fad..6e0ec3301 100644 --- a/test/unit/plugins/provisioners/ansible/config_test.rb +++ b/test/unit/plugins/provisioners/ansible/config_test.rb @@ -1,4 +1,5 @@ require_relative "../../../base" +require_relative "../support/shared/config" require Vagrant.source_root.join("plugins/provisioners/ansible/config") @@ -38,20 +39,30 @@ describe VagrantPlugins::Ansible::Config do expect(subject.playbook).to be_nil expect(subject.extra_vars).to be_nil - expect(subject.ask_sudo_pass).to be_nil + expect(subject.ask_sudo_pass).to be_false expect(subject.limit).to be_nil - expect(subject.sudo).to be_nil + expect(subject.sudo).to be_false expect(subject.sudo_user).to be_nil expect(subject.verbose).to be_nil expect(subject.tags).to be_nil expect(subject.skip_tags).to be_nil expect(subject.start_at_task).to be_nil expect(subject.groups).to eq({}) - expect(subject.host_key_checking).to eq('false') + expect(subject.host_key_checking).to be_false expect(subject.raw_arguments).to be_nil expect(subject.raw_ssh_args).to be_nil end + describe "host_key_checking option" do + it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :host_key_checking, false + end + describe "ask_sudo_pass option" do + it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :ask_sudo_pass, false + end + describe "sudo option" do + it_behaves_like "any VagrantConfigProvisioner strict boolean attribute", :sudo, false + end + describe "#validate" do before do subject.playbook = existing_file diff --git a/test/unit/plugins/provisioners/ansible/provisioner_test.rb b/test/unit/plugins/provisioners/ansible/provisioner_test.rb index 9a4d3695f..007d3b98a 100644 --- a/test/unit/plugins/provisioners/ansible/provisioner_test.rb +++ b/test/unit/plugins/provisioners/ansible/provisioner_test.rb @@ -259,7 +259,7 @@ VF describe "with host_key_checking option enabled" do before do - config.host_key_checking = "true" + config.host_key_checking = true end it_should_set_arguments_and_environment_variables 5, 3, true diff --git a/test/unit/plugins/provisioners/support/shared/config.rb b/test/unit/plugins/provisioners/support/shared/config.rb new file mode 100644 index 000000000..052a10b4a --- /dev/null +++ b/test/unit/plugins/provisioners/support/shared/config.rb @@ -0,0 +1,28 @@ +shared_examples_for 'any VagrantConfigProvisioner strict boolean attribute' do |attr_name, attr_default_value| + + [true, false].each do |bool| + it "returns the assigned boolean value (#{bool})" do + subject.send("#{attr_name}=", bool) + subject.finalize! + + expect(subject.send(attr_name)).to eql(bool) + end + end + + it "returns the default value (#{attr_default_value}) if undefined" do + subject.finalize! + + expect(subject.send(attr_name)).to eql(attr_default_value) + end + + [nil, 'true', 'false', 1, 0, 'this is not a boolean'].each do |nobool| + it "returns the default value when assigned value is invalid (#{nobool.class}: #{nobool})" do + subject.send("#{attr_name}=", nobool) + subject.finalize! + + expect(subject.send(attr_name)).to eql(attr_default_value) + end + end + +end +