From baf0649dcfa1db203b8fe4c0404d1428d8c2a537 Mon Sep 17 00:00:00 2001 From: Gilles Cornu Date: Sat, 29 Mar 2014 09:19:26 +0100 Subject: [PATCH] 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