From 286bdf25b80eadc7036ccb1e7ff881c395b8731f Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Fri, 25 Nov 2016 15:08:33 +0000 Subject: [PATCH 1/3] Basic spec tests for config Some simple spec tests to ensure validate and finalize work, which also includes some tests showing that merging is not functioning as needed for disks/cdroms both in cdroms not being merged across configs and device ids are not handled correctly for merged configs. --- spec/unit/config_spec.rb | 112 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 spec/unit/config_spec.rb diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb new file mode 100644 index 0000000..ce2287f --- /dev/null +++ b/spec/unit/config_spec.rb @@ -0,0 +1,112 @@ +require "spec_helper" +require "support/sharedcontext" + +require "vagrant-libvirt/config" + +describe VagrantPlugins::ProviderLibvirt::Config do + include_context "unit" + + def assert_invalid + errors = subject.validate(machine) + if errors.values.any? { |v| !v.empty? } + raise "No errors: #{errors.inspect}" + end + end + + def assert_valid + errors = subject.validate(machine) + if !errors.values.all? { |v| v.empty? } + raise "Errors: #{errors.inspect}" + end + end + + describe "#validate" do + it "is valid with defaults" do + assert_valid + end + + it "is valid if relative path used for disk" do + subject.storage :file, :path => '../path/to/file.qcow2' + assert_valid + end + + it "should be invalid if absolute path used for disk" do + subject.storage :file, :path => '/absolute/path/to/file.qcow2' + assert_invalid + end + + context "with mac defined" do + let (:vm) { double("vm") } + let (:networks) { double("networks") } + before do + allow(vm).to receive(:networks).and_return(networks) + allow(machine.config).to receive(:vm).and_return(vm) + end + + it "is valid with valid mac" do + allow(networks).to receive(:each).and_return([:public, {:mac => "aa:bb:cc:dd:ee:ff"}]) + assert_valid + end + + it "should be invalid if MAC not formatted correctly" do + allow(networks).to receive(:each).and_return([:public, {:mac => "aabbccddeeff"}]) + assert_invalid + end + end + end + + describe "#merge" do + let(:one) { described_class.new } + let(:two) { described_class.new } + + subject { one.merge(two) } + + context "storage" do + context "with disks" do + context "assigned specific devices" do + it "should merge disks with specific devices" do + one.storage(:file, :device => "vdb") + two.storage(:file, :device => "vdc") + subject.finalize! + expect(subject.disks).to include(include(:device => "vdb"), + include(:device => "vdc")) + end + end + + context "without devices given" do + xit "pending device assignment in finalize " + + "should merge disks with different devices assigned automatically" do + one.storage(:file) + two.storage(:file) + subject.finalize! + expect(subject.disks).to include(include(:device => "vdb"), + include(:device => "vdc")) + end + end + end + + context "with cdroms only" do + context "assigned specific devs" do + xit "should merge disks with specific devices" do + one.storage(:file, :device => :cdrom, :dev => "hda") + two.storage(:file, :device => :cdrom, :dev => "hdb") + subject.finalize! + expect(subject.cdroms).to include(include(:dev => "hda"), + include(:dev => "hdb")) + end + end + + context "without devs given" do + xit "pending cdroms merging support and device assignment in finalize " + + "should merge cdroms with different devs assigned automatically" do + one.storage(:file, :device => :cdrom) + two.storage(:file, :device => :cdrom) + subject.finalize! + expect(subject.cdroms).to include(include(:dev => "hda"), + include(:dev => "hdb")) + end + end + end + end + end +end From 4af944f9d658bc2e23bd30bc53dde7d45cdf7814 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Fri, 25 Nov 2016 16:07:55 +0000 Subject: [PATCH 2/3] Merge CDROM storage config blocks --- lib/vagrant-libvirt/config.rb | 3 +++ spec/unit/config_spec.rb | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/vagrant-libvirt/config.rb b/lib/vagrant-libvirt/config.rb index df6f640..30acc03 100644 --- a/lib/vagrant-libvirt/config.rb +++ b/lib/vagrant-libvirt/config.rb @@ -637,6 +637,9 @@ module VagrantPlugins c = disks.dup c += other.disks result.disks = c + c = cdroms.dup + c += other.cdroms + result.cdroms = c end end end diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index ce2287f..0fb97a2 100644 --- a/spec/unit/config_spec.rb +++ b/spec/unit/config_spec.rb @@ -87,7 +87,7 @@ describe VagrantPlugins::ProviderLibvirt::Config do context "with cdroms only" do context "assigned specific devs" do - xit "should merge disks with specific devices" do + it "should merge disks with specific devices" do one.storage(:file, :device => :cdrom, :dev => "hda") two.storage(:file, :device => :cdrom, :dev => "hdb") subject.finalize! From 46e0d09ce6da760445b121031b6efde90d134c60 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Fri, 25 Nov 2016 16:08:26 +0000 Subject: [PATCH 3/3] Fix separate storage config blocks device assignment Move device assignment for CDROM's and disk storage to occur during finalize, to ensure that storage config blocks are merged before assigning devices. This ensures that defining multiple storage config blocks within the same or different Vagrantfiles that are merged, works as expected to create multiple separate storage devices without needing to explicitly define the dev name. Fixes #655 --- lib/vagrant-libvirt/config.rb | 10 ++++++++-- spec/unit/config_spec.rb | 6 ++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/vagrant-libvirt/config.rb b/lib/vagrant-libvirt/config.rb index 30acc03..325b38c 100644 --- a/lib/vagrant-libvirt/config.rb +++ b/lib/vagrant-libvirt/config.rb @@ -432,7 +432,6 @@ module VagrantPlugins # as will the address unit number (unit=0, unit=1, etc) options = { - :dev => self._get_cdrom_dev(@cdroms), :bus => "ide", :path => nil, }.merge(options) @@ -448,7 +447,6 @@ module VagrantPlugins def _handle_disk_storage(options={}) options = { - :device => _get_device(@disks), :type => 'qcow2', :size => '10G', # matches the fog default :path => nil, @@ -586,7 +584,15 @@ module VagrantPlugins # Storage @disks = [] if @disks == UNSET_VALUE + @disks.map! do |disk| + disk[:device] = _get_device(@disks) if disk[:device].nil? + disk + end @cdroms = [] if @cdroms == UNSET_VALUE + @cdroms.map! do |cdrom| + cdrom[:dev] = _get_cdrom_dev(@cdroms) if cdrom[:dev].nil? + cdrom + end # Inputs @inputs = [{:type => "mouse", :bus => "ps2"}] if @inputs == UNSET_VALUE diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index 0fb97a2..b6a677f 100644 --- a/spec/unit/config_spec.rb +++ b/spec/unit/config_spec.rb @@ -74,8 +74,7 @@ describe VagrantPlugins::ProviderLibvirt::Config do end context "without devices given" do - xit "pending device assignment in finalize " + - "should merge disks with different devices assigned automatically" do + it "should merge disks with different devices assigned automatically" do one.storage(:file) two.storage(:file) subject.finalize! @@ -97,8 +96,7 @@ describe VagrantPlugins::ProviderLibvirt::Config do end context "without devs given" do - xit "pending cdroms merging support and device assignment in finalize " + - "should merge cdroms with different devs assigned automatically" do + it "should merge cdroms with different devs assigned automatically" do one.storage(:file, :device => :cdrom) two.storage(:file, :device => :cdrom) subject.finalize!