core: all box collection tests pass

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
+    # @param [String] version The version of this box.
-    #   will be verified with the `metadata.json` file in the box and is
+    # @param [Array<String>] 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)
+    def add(path, name, version, **opts)
-      formats = [formats] if formats && !formats.is_a?(Array)
+      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)
+        box = find(name, box_formats, version)
         next if !box
 
-          if !force
+        if !opts[:force]
-            @logger.error("Box already exists, can't add: #{name} #{box_formats.join(", ")}")
+          @logger.error(
-            raise Errors::BoxAlreadyExists, :name => name, :formats => box_formats.join(", ")
+            "Box already exists, can't add: #{name} v#{version} #{box_formats.join(", ")}")
+          raise Errors::BoxAlreadyExists,
+            name: name,
+            providers: box_formats.join(", "),
+            version: version
         end
 
         # We're forcing, so just delete the old box
         @logger.info(
-            "Box already exists, but forcing so removing: #{name} #{box_formats.join(", ")}")
+          "Box already exists, but forcing so removing: " +
+          "#{name} v#{version} #{box_formats.join(", ")}")
         box.destroy!
       end
 
-        log_provider = formats ? formats.join(", ") : "any provider"
+      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
+        check_box_exists.call(providers) if providers
-
-        # 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))
 
         # 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
+            if providers
-              found = false
+              found = providers.find { |p| p.to_sym == box_provider.to_sym }
-              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 !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")

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

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 = subject.add(box_path, "foo", "1.0", providers: :virtualbox)
-      box.should be_kind_of(box_class)
+      expect(box).to be_kind_of(box_class)
-      box.name.should == "foo"
+      expect(box.name).to eq("foo")
-      box.provider.should == :virtualbox
+      expect(box.provider).to eq(:virtualbox)
 
       # Verify we can find it as well
-      box = instance.find("foo", :virtualbox)
+      expect(subject.find("foo", :virtualbox, "1.0")).to_not be_nil
-      box.should_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 = subject.add(box_path, "foo", "1.0")
-      box.should be_kind_of(box_class)
+      expect(box).to be_kind_of(box_class)
-      box.name.should == "foo"
+      expect(box.name).to eq("foo")
-      box.provider.should == :vmware
+      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 = subject.add(box_path, "foo", "1.0")
-      box.should be_kind_of(box_class)
+      expect(box).to be_kind_of(box_class)
-      box.name.should == "foo"
+      expect(box.name).to eq("foo")
-      box.provider.should == :virtualbox
+      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) }.
+      expect {
-        to raise_error(Vagrant::Errors::BoxAlreadyExists)
+        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")
+      @foo_path    = environment.box2("foo", "virtualbox")
-      @vbox_path   = boxes_dir.join("precise64", "virtualbox")
+      @vbox_path   = environment.box2("precise64", "virtualbox")
-      @vmware_path = boxes_dir.join("precise64", "vmware")
+      @vmware_path = environment.box2("precise64", "vmware")
-      @v1_path     = boxes_dir.join("v1box")
+      @v1_path     = environment.box("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") }
     end
 
     it "upgrades the boxes" do