From 87a85488d1e1a32cd98243648d2373d52a2c76c6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 22 Jan 2014 15:50:10 -0800 Subject: [PATCH] core: all box collection tests pass --- lib/vagrant/box_collection.rb | 110 ++++++++------------ templates/locales/en.yml | 3 +- test/unit/vagrant/box_collection_test.rb | 124 +++++++---------------- 3 files changed, 78 insertions(+), 159 deletions(-) diff --git a/lib/vagrant/box_collection.rb b/lib/vagrant/box_collection.rb index b85fe56d6..e1262c7ba 100644 --- a/lib/vagrant/box_collection.rb +++ b/lib/vagrant/box_collection.rb @@ -64,45 +64,48 @@ module Vagrant # # @param [Pathname] path Path to the box file on disk. # @param [String] name Logical name for the box. - # @param [Symbol] provider The provider that the box should be for. This - # will be verified with the `metadata.json` file in the box and is + # @param [String] version The version of this box. + # @param [Array] providers The providers that this box can + # be a part of. This will be verified with the `metadata.json` and is # meant as a basic check. If this isn't given, then whatever provider # the box represents will be added. # @param [Boolean] force If true, any existing box with the same name # and provider will be replaced. - def add(path, name, formats=nil, force=false) - formats = [formats] if formats && !formats.is_a?(Array) + def add(path, name, version, **opts) + providers = opts[:providers] + providers = Array(providers) if providers provider = nil - with_collection_lock do - # A helper to check if a box exists. We store this in a variable - # since we call it multiple times. - check_box_exists = lambda do |box_formats| - box = find(name, box_formats) - next if !box + # A helper to check if a box exists. We store this in a variable + # since we call it multiple times. + check_box_exists = lambda do |box_formats| + box = find(name, box_formats, version) + next if !box - if !force - @logger.error("Box already exists, can't add: #{name} #{box_formats.join(", ")}") - raise Errors::BoxAlreadyExists, :name => name, :formats => box_formats.join(", ") - end - - # We're forcing, so just delete the old box - @logger.info( - "Box already exists, but forcing so removing: #{name} #{box_formats.join(", ")}") - box.destroy! + if !opts[:force] + @logger.error( + "Box already exists, can't add: #{name} v#{version} #{box_formats.join(", ")}") + raise Errors::BoxAlreadyExists, + name: name, + providers: box_formats.join(", "), + version: version end - log_provider = formats ? formats.join(", ") : "any provider" + # We're forcing, so just delete the old box + @logger.info( + "Box already exists, but forcing so removing: " + + "#{name} v#{version} #{box_formats.join(", ")}") + box.destroy! + end + + with_collection_lock do + log_provider = providers ? providers.join(", ") : "any provider" @logger.debug("Adding box: #{name} (#{log_provider}) from #{path}") # Verify the box doesn't exist early if we're given a provider. This # can potentially speed things up considerably since we don't need # to unpack any files. - check_box_exists.call(formats) if formats - - # Verify that a V1 box doesn't exist. If it does, then we signal - # to the user that we need an upgrade. - raise Errors::BoxUpgradeRequired, :name => name if v1_box?(@directory.join(name)) + check_box_exists.call(providers) if providers # Create a temporary directory since we're not sure at this point if # the box we're unpackaging already exists (if no provider was given) @@ -111,7 +114,10 @@ module Vagrant @logger.debug("Unpacking box into temporary directory: #{temp_dir}") result = Util::Subprocess.execute( "bsdtar", "-v", "-x", "-m", "-C", temp_dir.to_s, "-f", path.to_s) - raise Errors::BoxUnpackageFailure, :output => result.stderr.to_s if result.exit_code != 0 + if result.exit_code != 0 + raise Errors::BoxUnpackageFailure, + output: result.stderr.to_s + end # If we get a V1 box, we want to update it in place if v1_box?(temp_dir) @@ -131,16 +137,8 @@ module Vagrant # to the system or check that it matches what is given to us. box_provider = box.metadata["provider"] - if formats - found = false - formats.each do |format| - # Verify that the given provider matches what the box has. - if box_provider.to_sym == format.to_sym - found = true - break - end - end - + if providers + found = providers.find { |p| p.to_sym == box_provider.to_sym } if !found @logger.error("Added box provider doesnt match expected: #{log_provider}") raise Errors::BoxProviderDoesntMatch, @@ -155,7 +153,7 @@ module Vagrant provider = box_provider.to_sym # Create the directory for this box, not including the provider - box_dir = @directory.join(name) + box_dir = @directory.join(name, version) box_dir.mkpath @logger.debug("Box directory: #{box_dir}") @@ -182,7 +180,7 @@ module Vagrant end # Return the box - find(name, provider) + find(name, provider, version) end # This returns an array of all the boxes on the system, given by @@ -266,37 +264,6 @@ module Vagrant nil end - # Upgrades a V1 box with the given name to a V2 box. If a box with the - # given name doesn't exist, then a `BoxNotFound` exception will be raised. - # If the given box is found but is not a V1 box then `true` is returned - # because this just works fine. - # - # @param [String] name Name of the box (logical name). - # @return [Boolean] `true` otherwise an exception is raised. - def upgrade(name) - with_collection_lock do - @logger.debug("Upgrade request for box: #{name}") - box_dir = @directory.join(name) - - # If the box doesn't exist at all, raise an exception - raise Errors::BoxNotFound, :name => name, :provider => "virtualbox" if !box_dir.directory? - - if v1_box?(box_dir) - @logger.debug("V1 box #{name} found. Upgrading!") - - # First we actually perform the upgrade - temp_dir = v1_upgrade(box_dir) - - # Rename the temporary directory to the provider. - FileUtils.mv(temp_dir.to_s, box_dir.join("virtualbox").to_s) - @logger.info("Box '#{name}' upgraded from V1 to V2.") - end - end - - # We did it! Or the v1 box didn't exist so it doesn't matter. - return true - end - # This upgrades a v1.1 - v1.4 box directory structure up to a v1.5 # directory structure. This will raise exceptions if it fails in any # way. @@ -311,7 +278,10 @@ module Vagrant box_name = boxdir.basename.to_s # If it is a v1 box, then we need to upgrade it first - upgrade(box_name) if v1_box?(boxdir) + if v1_box?(boxdir) + upgrade_dir = v1_upgrade(boxdir) + FileUtils.mv(upgrade_dir, boxdir.join("virtualbox")) + end # Create the directory for this box new_box_dir = temp_dir.join(box_name, "0") diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 2739a8b3f..f0803b809 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1230,7 +1230,8 @@ en: The box you're attempting to add already exists: Name: %{name} - Provider: %{formats} + Version: %{version} + Provider: %{providers} add: adding: |- Extracting box... diff --git a/test/unit/vagrant/box_collection_test.rb b/test/unit/vagrant/box_collection_test.rb index 0309753f0..b75849d83 100644 --- a/test/unit/vagrant/box_collection_test.rb +++ b/test/unit/vagrant/box_collection_test.rb @@ -8,17 +8,16 @@ describe Vagrant::BoxCollection do let(:box_class) { Vagrant::Box } let(:environment) { isolated_environment } - let(:instance) { described_class.new(environment.boxes_dir) } - subject { instance } + subject { described_class.new(environment.boxes_dir) } it "should tell us the directory it is using" do - instance.directory.should == environment.boxes_dir + subject.directory.should == environment.boxes_dir end describe "#all" do it "should return an empty array when no boxes are there" do - instance.all.should == [] + subject.all.should == [] end it "should return the boxes and their providers" do @@ -28,7 +27,7 @@ describe Vagrant::BoxCollection do environment.box3("bar", "0", :ec2) # Verify some output - results = instance.all + results = subject.all results.length.should == 3 results.include?(["foo", "1.0", :virtualbox]).should be results.include?(["foo", "1.0", :vmware]).should be @@ -37,7 +36,7 @@ describe Vagrant::BoxCollection do it 'does not raise an exception when a file appears in the boxes dir' do Tempfile.new('a_file', environment.boxes_dir) - expect { instance.all }.to_not raise_error + expect { subject.all }.to_not raise_error end end @@ -88,24 +87,23 @@ describe Vagrant::BoxCollection do box_path = environment.box2_file(:virtualbox) # Add the box - box = instance.add(box_path, "foo", :virtualbox) - box.should be_kind_of(box_class) - box.name.should == "foo" - box.provider.should == :virtualbox + box = subject.add(box_path, "foo", "1.0", providers: :virtualbox) + expect(box).to be_kind_of(box_class) + expect(box.name).to eq("foo") + expect(box.provider).to eq(:virtualbox) # Verify we can find it as well - box = instance.find("foo", :virtualbox) - box.should_not be_nil + expect(subject.find("foo", :virtualbox, "1.0")).to_not be_nil end it "should add a box without specifying a provider" do box_path = environment.box2_file(:vmware) # Add the box - box = instance.add(box_path, "foo") - box.should be_kind_of(box_class) - box.name.should == "foo" - box.provider.should == :vmware + box = subject.add(box_path, "foo", "1.0") + expect(box).to be_kind_of(box_class) + expect(box.name).to eq("foo") + expect(box.provider).to eq(:vmware) end it "should add a V1 box" do @@ -113,35 +111,39 @@ describe Vagrant::BoxCollection do box_path = environment.box1_file # Add the box - box = instance.add(box_path, "foo") - box.should be_kind_of(box_class) - box.name.should == "foo" - box.provider.should == :virtualbox + box = subject.add(box_path, "foo", "1.0") + expect(box).to be_kind_of(box_class) + expect(box.name).to eq("foo") + expect(box.provider).to eq(:virtualbox) end it "should raise an exception if the box already exists" do prev_box_name = "foo" prev_box_provider = :virtualbox + prev_box_version = "1.0" # Create the box we're adding - environment.box2(prev_box_name, prev_box_provider) + environment.box3(prev_box_name, "1.0", prev_box_provider) # Attempt to add the box with the same name box_path = environment.box2_file(prev_box_provider) - expect { instance.add(box_path, prev_box_name, prev_box_provider) }. - to raise_error(Vagrant::Errors::BoxAlreadyExists) + expect { + subject.add(box_path, prev_box_name, + prev_box_version, providers: prev_box_provider) + }.to raise_error(Vagrant::Errors::BoxAlreadyExists) end it "should replace the box if force is specified" do prev_box_name = "foo" prev_box_provider = :vmware + prev_box_version = "1.0" # Setup the environment with the box pre-added - environment.box2(prev_box_name, prev_box_provider) + environment.box3(prev_box_name, prev_box_version, prev_box_provider) # Attempt to add the box with the same name box_path = environment.box2_file(prev_box_provider, metadata: { "replaced" => "yes" }) - box = instance.add(box_path, prev_box_name, nil, true) + box = subject.add(box_path, prev_box_name, prev_box_version, force: true) box.metadata["replaced"].should == "yes" end @@ -151,25 +153,13 @@ describe Vagrant::BoxCollection do box_path = environment.box2_file(:vmware) # Add it once, successfully - expect { instance.add(box_path, box_name) }.to_not raise_error + expect { subject.add(box_path, box_name, "1.0") }.to_not raise_error # Add it again, and fail! - expect { instance.add(box_path, box_name) }. + expect { subject.add(box_path, box_name, "1.0") }. to raise_error(Vagrant::Errors::BoxAlreadyExists) end - it "should raise an exception if you're attempting to add a box that exists as a V1 box" do - prev_box_name = "foo" - - # Create the V1 box - environment.box1(prev_box_name) - - # Attempt to add some V2 box with the same name - box_path = environment.box2_file(:vmware) - expect { instance.add(box_path, prev_box_name) }. - to raise_error(Vagrant::Errors::BoxUpgradeRequired) - end - it "should raise an exception and not add the box if the provider doesn't match" do box_name = "foo" good_provider = :virtualbox @@ -180,11 +170,11 @@ describe Vagrant::BoxCollection do # Add the box but with an invalid provider, verify we get the proper # error. - expect { instance.add(box_path, box_name, bad_provider) }. + expect { subject.add(box_path, box_name, "1.0", providers: bad_provider) }. to raise_error(Vagrant::Errors::BoxProviderDoesntMatch) # Verify the box doesn't exist - instance.find(box_name, bad_provider).should be_nil + expect(subject.find(box_name, bad_provider, "1.0")).to be_nil end it "should raise an exception if you add an invalid box file" do @@ -199,7 +189,7 @@ describe Vagrant::BoxCollection do f.write("\0"*CHECKSUM_LENGTH) f.close - expect { instance.add(f.path, "foo", :virtualbox) }. + expect { subject.add(f.path, "foo", "1.0") }. to raise_error(Vagrant::Errors::BoxUnpackageFailure) ensure f.close @@ -208,57 +198,15 @@ describe Vagrant::BoxCollection do end end - describe "upgrading" do - it "should upgrade a V1 box to V2" do - # Create a V1 box - environment.box1("foo") - - # Verify that only a V1 box exists - expect { instance.find("foo", :virtualbox) }. - to raise_error(Vagrant::Errors::BoxUpgradeRequired) - - # Upgrade the box - instance.upgrade("foo").should be - - # Verify the box exists - box = instance.find("foo", :virtualbox) - box.should_not be_nil - box.name.should == "foo" - end - - it "should raise a BoxNotFound exception if a non-existent box is upgraded" do - expect { instance.upgrade("i-dont-exist") }. - to raise_error(Vagrant::Errors::BoxNotFound) - end - - it "should return true if we try to upgrade a V2 box" do - # Create a V2 box - environment.box2("foo", :vmware) - - instance.upgrade("foo").should be - end - end - describe "#upgrade_v1_1_v1_5" do let(:boxes_dir) { environment.boxes_dir } before do # Create all the various box directories - @foo_path = boxes_dir.join("foo", "virtualbox") - @vbox_path = boxes_dir.join("precise64", "virtualbox") - @vmware_path = boxes_dir.join("precise64", "vmware") - @v1_path = boxes_dir.join("v1box") - - [@foo_path, @vbox_path, @vmware_path].each do |path| - path.mkpath - path.join("name").open("w") do |f| - f.write(path.to_s) - end - end - - @v1_path.mkpath - @v1_path.join("box.ovf").open("w") { |f| f.write("yep!") } - @v1_path.join("name").open("w") { |f| f.write("v1box") } + @foo_path = environment.box2("foo", "virtualbox") + @vbox_path = environment.box2("precise64", "virtualbox") + @vmware_path = environment.box2("precise64", "vmware") + @v1_path = environment.box("v1box") end it "upgrades the boxes" do