diff --git a/lib/vagrant/action/builtin/box_remove.rb b/lib/vagrant/action/builtin/box_remove.rb index 47dedd1fa..cf80542cb 100644 --- a/lib/vagrant/action/builtin/box_remove.rb +++ b/lib/vagrant/action/builtin/box_remove.rb @@ -111,7 +111,7 @@ module Vagrant provider: box.provider, version: box.version)) box.destroy! - env[:box_collection].clean_up(box) + env[:box_collection].clean(box.name) # Passes on the removed box to the rest of the middleware chain env[:box_removed] = box diff --git a/lib/vagrant/box_collection.rb b/lib/vagrant/box_collection.rb index 17c2f3927..6e83ad4ee 100644 --- a/lib/vagrant/box_collection.rb +++ b/lib/vagrant/box_collection.rb @@ -12,9 +12,9 @@ module Vagrant # for accessing/finding individual boxes, adding new boxes, or deleting # boxes. class BoxCollection - TEMP_PREFIX = "vagrant-box-add-temp-" - VAGRANT_SLASH = "-VAGRANTSLASH-" - VAGRANT_COLON = "-VAGRANTCOLON-" + TEMP_PREFIX = "vagrant-box-add-temp-".freeze + VAGRANT_SLASH = "-VAGRANTSLASH-".freeze + VAGRANT_COLON = "-VAGRANTCOLON-".freeze # The directory where the boxes in this collection are stored. # @@ -348,17 +348,12 @@ module Vagrant end end - # Removes the whole directory of a given box if there are no - # other versions nor providers of the box exist. - def clean_up(box) - return false if exists?(box.name) - - box_directory = box.name - .gsub('/', VAGRANT_SLASH) - .gsub(':', VAGRANT_COLON) - - path = File.join(directory, box_directory) - FileUtils.rm_r(path) + # Cleans the directory for a box by removing the folders that are + # empty. + def clean(name) + return false if exists?(name) + path = File.join(directory, dir_name(name)) + FileUtils.rm_rf(path) end protected diff --git a/test/unit/vagrant/action/builtin/box_remove_test.rb b/test/unit/vagrant/action/builtin/box_remove_test.rb index 016fec243..f1a740218 100644 --- a/test/unit/vagrant/action/builtin/box_remove_test.rb +++ b/test/unit/vagrant/action/builtin/box_remove_test.rb @@ -30,7 +30,7 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(box_collection).to receive(:find).with( "foo", :virtualbox, "1.0").and_return(box) - expect(box_collection).to receive(:clean_up).with(box) + expect(box_collection).to receive(:clean).with(box.name) .and_return(true) expect(box).to receive(:destroy!).once expect(app).to receive(:call).with(env).once @@ -52,7 +52,7 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(box_collection).to receive(:find).with( "foo", :virtualbox, "1.0").and_return(box) - expect(box_collection).to receive(:clean_up).with(box) + expect(box_collection).to receive(:clean).with(box.name) .and_return(false) expect(box).to receive(:destroy!).once expect(app).to receive(:call).with(env).once @@ -74,7 +74,7 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(box_collection).to receive(:find).with( "foo", :virtualbox, "1.0").and_return(box) - expect(box_collection).to receive(:clean_up).with(box) + expect(box_collection).to receive(:clean).with(box.name) .and_return(false) expect(box).to receive(:destroy!).once expect(app).to receive(:call).with(env).once @@ -84,22 +84,6 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(env[:box_removed]).to equal(box) end - it "deletes the whole directory of the box if it's the last box on the system" do - box_collection.stub( - all: [ - ["foo", "1.0", :virtualbox], - ]) - - env[:box_name] = "foo" - - expect(box_collection).to receive(:find).with( - "foo", :virtualbox, "1.0").and_return(box) - expect(box_collection).to receive(:clean_up).with(box) - .and_return(true) - - subject.call(env) - end - context "checking if a box is in use" do def new_entry(name, provider, version, valid=true) Vagrant::MachineIndex::Entry.new.tap do |entry| @@ -132,7 +116,7 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(box_collection).to receive(:find).with( "foo", :virtualbox, "1.0").and_return(box) expect(box).to receive(:destroy!).once - expect(box_collection).to receive(:clean_up).with(box) + expect(box_collection).to receive(:clean).with(box.name) .and_return(true) subject.call(env) @@ -147,7 +131,7 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(box_collection).to receive(:find).with( "foo", :virtualbox, "1.0").and_return(box) - expect(box_collection).to receive(:clean_up).with(box) + expect(box_collection).to receive(:clean).with(box.name) .and_return(true) expect(box).to receive(:destroy!).once diff --git a/test/unit/vagrant/box_collection_test.rb b/test/unit/vagrant/box_collection_test.rb index a09d23559..4a22b5b72 100644 --- a/test/unit/vagrant/box_collection_test.rb +++ b/test/unit/vagrant/box_collection_test.rb @@ -44,6 +44,65 @@ describe Vagrant::BoxCollection do end end + describe "#clean" do + it "removes the directory if no other versions of the box exists" do + # Create a few boxes, immediately destroy them + environment.box3("foo", "1.0", :virtualbox) + environment.box3("foo", "1.0", :vmware) + + # Delete them all + subject.all.each do |parts| + subject.find(parts[0], parts[2], ">= 0").destroy! + end + + # Cleanup + subject.clean("foo") + + # Make sure the whole directory is empty + expect(environment.boxes_dir.children).to be_empty + end + + it "doesn't remove the directory if a provider exists" do + # Create a few boxes, immediately destroy them + environment.box3("foo", "1.0", :virtualbox) + environment.box3("foo", "1.0", :vmware) + + # Delete them all + subject.find("foo", :virtualbox, ">= 0").destroy! + + # Cleanup + subject.clean("foo") + + # Make sure the whole directory is not empty + expect(environment.boxes_dir.children).to_not be_empty + + # Make sure the results still exist + results = subject.all + expect(results.length).to eq(1) + expect(results.include?(["foo", "1.0", :vmware])).to be + end + + it "doesn't remove the directory if a version exists" do + # Create a few boxes, immediately destroy them + environment.box3("foo", "1.0", :virtualbox) + environment.box3("foo", "1.2", :virtualbox) + + # Delete them all + subject.find("foo", :virtualbox, ">= 1.1").destroy! + + # Cleanup + subject.clean("foo") + + # Make sure the whole directory is not empty + expect(environment.boxes_dir.children).to_not be_empty + + # Make sure the results still exist + results = subject.all + expect(results.length).to eq(1) + expect(results.include?(["foo", "1.0", :virtualbox])).to be + end + end + describe "#find" do it "returns nil if the box does not exist" do expect(subject.find("foo", :i_dont_exist, ">= 0")).to be_nil