diff --git a/lib/vagrant/action/builtin/box_remove.rb b/lib/vagrant/action/builtin/box_remove.rb index 51c600297..8ee9cd7f6 100644 --- a/lib/vagrant/action/builtin/box_remove.rb +++ b/lib/vagrant/action/builtin/box_remove.rb @@ -71,6 +71,49 @@ module Vagrant box = env[:box_collection].find( box_name, box_provider, box_version) + # Verify that this box is not in use by an active machine, + # otherwise warn the user. + users = [] + env[:machine_index].each do |entry| + box_data = entry.extra_data["box"] + next if !box_data + + # If all the data matches AND the entry is a seemingly + # valid entry, then track it. + if box_data["name"] == box.name && + box_data["provider"] == box.provider.to_s && + box_data["version"] == box.version.to_s && + entry.valid?(env[:home_path]) + users << entry + end + end + + if !users.empty? + # Build up the output to show the user. + users = users.map do |entry| + "#{entry.name} (ID: #{entry.id})" + end.join("\n") + + force_key = :force_confirm_box_remove + message = I18n.t( + "vagrant.commands.box.remove_in_use_query", + name: box.name, + provider: box.provider, + version: box.version, + users: users) + " " + + # Ask the user if we should do this + stack = Builder.new.tap do |b| + b.use Confirm, message, force_key + end + + result = env[:action_runner].run(stack, env) + if !result[:result] + # They said "no", so just return + return @app.call(env) + end + end + env[:ui].info(I18n.t("vagrant.commands.box.removing", :name => box.name, :provider => box.provider, diff --git a/lib/vagrant/environment.rb b/lib/vagrant/environment.rb index 3a8509456..2c7e9eac0 100644 --- a/lib/vagrant/environment.rb +++ b/lib/vagrant/environment.rb @@ -144,7 +144,7 @@ module Vagrant hook(:environment_plugins_loaded, runner: Action::Runner.new(env: self)) # Call the environment load hooks - hook(:environment_load) + hook(:environment_load, runner: Action::Runner.new(env: self)) end # Return a human-friendly string for pretty printed or inspected @@ -165,6 +165,7 @@ module Vagrant :box_collection => boxes, :hook => method(:hook), :host => host, + :machine_index => machine_index, :gems_path => gems_path, :home_path => home_path, :root_path => root_path, diff --git a/lib/vagrant/machine.rb b/lib/vagrant/machine.rb index 9b646bba9..6c05d9966 100644 --- a/lib/vagrant/machine.rb +++ b/lib/vagrant/machine.rb @@ -260,6 +260,15 @@ module Vagrant entry.state = "preparing" entry.vagrantfile_path = @env.root_path entry.vagrantfile_name = @env.vagrantfile_name + + if @box + entry.extra_data["box"] = { + "name" => @box.name, + "provider" => @box.provider.to_s, + "version" => @box.version.to_s, + } + end + entry = @env.machine_index.set(entry) @env.machine_index.release(entry) diff --git a/lib/vagrant/machine_index.rb b/lib/vagrant/machine_index.rb index ab4015ebd..bbe99f0d7 100644 --- a/lib/vagrant/machine_index.rb +++ b/lib/vagrant/machine_index.rb @@ -378,6 +378,8 @@ module Vagrant # The parameter given should be nil if this is being created # publicly. def initialize(id=nil, raw=nil) + @extra_data = {} + # Do nothing if we aren't given a raw value. Otherwise, parse it. return if !raw @@ -401,6 +403,33 @@ module Vagrant @vagrantfile_path = Pathname.new(@vagrantfile_path) if @vagrantfile_path end + # Returns boolean true if this entry appears to be valid. + # The critera for being valid: + # + # * Vagrantfile directory exists + # * Vagrant environment contains a machine with this + # name and provider. + # + # This method is _slow_. It should be used with care. + # + # @param [Pathname] home_path The home path for the Vagrant + # environment. + # @return [Boolean] + def valid?(home_path) + return false if !vagrantfile_path + return false if !vagrantfile_path.directory? + + # Create an environment so we can determine the active + # machines... + env = vagrant_env(home_path) + env.active_machines.each do |name, provider| + return true if name.to_s == self.name.to_s && + provider.to_s == self.provider.to_s + end + + false + end + # Creates a {Vagrant::Environment} for this entry. # # @return [Vagrant::Environment] diff --git a/plugins/commands/box/command/remove.rb b/plugins/commands/box/command/remove.rb index f2a268121..28eed63f5 100644 --- a/plugins/commands/box/command/remove.rb +++ b/plugins/commands/box/command/remove.rb @@ -6,11 +6,17 @@ module VagrantPlugins class Remove < Vagrant.plugin("2", :command) def execute options = {} + options[:force] = false + opts = OptionParser.new do |o| o.banner = "Usage: vagrant box remove " o.separator "" o.separator "Options:" - o.separator "" + o.separator "" + + o.on("-f", "--force", "Destroy without confirmation.") do |f| + options[:force] = f + end o.on("--provider PROVIDER", String, "The specific provider type for the box to remove") do |p| @@ -43,6 +49,7 @@ module VagrantPlugins :box_name => argv[0], :box_provider => options[:provider], :box_version => options[:version], + :force_confirm_box_remove => options[:force], }) # Success, exit status 0 diff --git a/plugins/commands/global-status/command.rb b/plugins/commands/global-status/command.rb index 94dfd6c63..da1399c0c 100644 --- a/plugins/commands/global-status/command.rb +++ b/plugins/commands/global-status/command.rb @@ -42,7 +42,7 @@ module VagrantPlugins @env.machine_index.each do |entry| # If we're pruning and this entry is invalid, skip it # and prune it later. - if options[:prune] && invalid?(entry) + if options[:prune] && !entry.valid?(@env.home_path) prune << entry next end @@ -95,23 +95,6 @@ module VagrantPlugins # Success, exit status 0 0 end - - protected - - # Tests if a entry is invalid and should be pruned - def invalid?(entry) - return true if !entry.vagrantfile_path.directory? - - # Create an environment so we can determine the active - # machines... - env = entry.vagrant_env(@env.home_path) - env.active_machines.each do |name, provider| - return false if name.to_s == entry.name.to_s && - provider.to_s == entry.provider.to_s - end - - true - end end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index a1a521794..f10e84390 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1251,6 +1251,15 @@ en: vm_not_running: "VM is not currently running. Please, first bring it up with `vagrant up` then run this command." box: no_installed_boxes: "There are no installed boxes! Use `vagrant box add` to add some." + remove_in_use_query: |- + Box '%{name}' (v%{version}) with provider '%{provider}' appears + to still be in use by at least one Vagrant environment. Removing + the box could corrupt the environment. We recommend destroying + these environments first: + + %{users} + + Are you sure you want to remove this box? [y/N] removing: |- Removing box '%{name}' (v%{version}) with provider '%{provider}'... destroy: diff --git a/test/unit/plugins/commands/box/command/remove_test.rb b/test/unit/plugins/commands/box/command/remove_test.rb index 67c4a0131..923d920b5 100644 --- a/test/unit/plugins/commands/box/command/remove_test.rb +++ b/test/unit/plugins/commands/box/command/remove_test.rb @@ -34,11 +34,26 @@ describe VagrantPlugins::CommandBox::Command::Remove do it "invokes the action runner" do expect(action_runner).to receive(:run).with { |action, opts| expect(opts[:box_name]).to eq("foo") + expect(opts[:force_confirm_box_remove]).to be_false true } subject.execute end + + context "with --force" do + let(:argv) { super() + ["--force"] } + + it "invokes the action runner with force option" do + expect(action_runner).to receive(:run).with { |action, opts| + expect(opts[:box_name]).to eq("foo") + expect(opts[:force_confirm_box_remove]).to be_true + true + } + + subject.execute + end + end end context "with two arguments" do diff --git a/test/unit/vagrant/action/builtin/box_remove_test.rb b/test/unit/vagrant/action/builtin/box_remove_test.rb index 3a21cb820..9de3e467b 100644 --- a/test/unit/vagrant/action/builtin/box_remove_test.rb +++ b/test/unit/vagrant/action/builtin/box_remove_test.rb @@ -6,12 +6,16 @@ describe Vagrant::Action::Builtin::BoxRemove do let(:app) { lambda { |env| } } let(:env) { { box_collection: box_collection, + home_path: home_path, + machine_index: machine_index, ui: Vagrant::UI::Silent.new, } } subject { described_class.new(app, env) } let(:box_collection) { double("box_collection") } + let(:home_path) { "foo" } + let(:machine_index) { [] } let(:iso_env) { isolated_environment } let(:box) do @@ -74,6 +78,71 @@ describe Vagrant::Action::Builtin::BoxRemove do expect(env[:box_removed]).to equal(box) 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| + entry.extra_data["box"] = { + "name" => "foo", + "provider" => "virtualbox", + "version" => "1.0", + } + + entry.stub(valid?: valid) + end + end + + let(:action_runner) { double("action_runner") } + + before do + env[:action_runner] = action_runner + + box_collection.stub( + all: [ + ["foo", "1.0", :virtualbox], + ["foo", "1.1", :virtualbox], + ]) + + env[:box_name] = "foo" + env[:box_version] = "1.0" + end + + it "does delete if the box is not in use" do + expect(box_collection).to receive(:find).with( + "foo", :virtualbox, "1.0").and_return(box) + expect(box).to receive(:destroy!).once + + subject.call(env) + end + + it "does delete if the box is in use and user confirms" do + machine_index << new_entry("foo", "virtualbox", "1.0") + + result = { result: true } + expect(action_runner).to receive(:run). + with(anything, env).and_return(result) + + expect(box_collection).to receive(:find).with( + "foo", :virtualbox, "1.0").and_return(box) + expect(box).to receive(:destroy!).once + + subject.call(env) + end + + it "doesn't delete if the box is in use" do + machine_index << new_entry("foo", "virtualbox", "1.0") + + result = { result: false } + expect(action_runner).to receive(:run). + with(anything, env).and_return(result) + + expect(box_collection).to receive(:find).with( + "foo", :virtualbox, "1.0").and_return(box) + expect(box).to receive(:destroy!).never + + subject.call(env) + end + end + it "errors if the box doesn't exist" do box_collection.stub(all: []) diff --git a/test/unit/vagrant/machine_index_test.rb b/test/unit/vagrant/machine_index_test.rb index 6a7efaa72..fe4a7cd00 100644 --- a/test/unit/vagrant/machine_index_test.rb +++ b/test/unit/vagrant/machine_index_test.rb @@ -265,3 +265,74 @@ describe Vagrant::MachineIndex do end end end + +describe Vagrant::MachineIndex::Entry do + include_context "unit" + + let(:env) { + iso_env = isolated_environment + iso_env.vagrantfile(vagrantfile) + iso_env.create_vagrant_env + } + + let(:vagrantfile) { "" } + + describe "#valid?" do + let(:machine) { env.machine(:default, :dummy) } + + subject do + described_class.new.tap do |e| + e.name = "default" + e.provider = "dummy" + e.vagrantfile_path = env.root_path + end + end + + it "should be valid with a valid entry" do + machine.id = "foo" + expect(subject).to be_valid(env.home_path) + end + + it "should be invalid if no Vagrantfile path is set" do + subject.vagrantfile_path = nil + expect(subject).to_not be_valid(env.home_path) + end + + it "should be invalid if the Vagrantfile path does not exist" do + subject.vagrantfile_path = Pathname.new("/i/should/not/exist") + expect(subject).to_not be_valid(env.home_path) + end + + it "should be invalid if the machine is inactive" do + machine.id = nil + expect(subject).to_not be_valid(env.home_path) + end + + context "with another active machine" do + let(:vagrantfile) do + <<-VF + Vagrant.configure("2") do |config| + config.vm.define "web" + config.vm.define "db" + end + VF + end + + it "should be invalid if the wrong machine is active only" do + m = env.machine(:web, :dummy) + m.id = "foo" + + subject.name = "db" + expect(subject).to_not be_valid(env.home_path) + end + + it "should be valid if the correct machine is active" do + env.machine(:web, :dummy).id = "foo" + env.machine(:db, :dummy).id = "foo" + + subject.name = "db" + expect(subject).to be_valid(env.home_path) + end + end + end +end diff --git a/test/unit/vagrant/machine_test.rb b/test/unit/vagrant/machine_test.rb index e289c4028..9e8f6e5df 100644 --- a/test/unit/vagrant/machine_test.rb +++ b/test/unit/vagrant/machine_test.rb @@ -21,7 +21,14 @@ describe Vagrant::Machine do let(:provider_name) { :test } let(:provider_options) { {} } let(:base) { false } - let(:box) { Object.new } + let(:box) do + double("box").tap do |b| + b.stub(name: "foo") + b.stub(provider: :dummy) + b.stub(version: "1.0") + end + end + let(:config) { env.vagrantfile.config } let(:data_dir) { Pathname.new(Dir.mktmpdir("vagrant")) } let(:env) do @@ -403,11 +410,32 @@ describe Vagrant::Machine do end it "is set one when setting an ID" do + # Setup the box information + box = double("box") + box.stub(name: "foo") + box.stub(provider: :bar) + box.stub(version: "1.2.3") + subject.box = box + subject.id = "foo" uuid = subject.index_uuid expect(uuid).to_not be_nil expect(new_instance.index_uuid).to eq(uuid) + + # Test the entry itself + entry = env.machine_index.get(uuid) + expect(entry.name).to eq(subject.name) + expect(entry.provider).to eq(subject.provider_name.to_s) + expect(entry.state).to eq("preparing") + expect(entry.vagrantfile_path).to eq(env.root_path) + expect(entry.vagrantfile_name).to eq(env.vagrantfile_name) + expect(entry.extra_data["box"]).to eq({ + "name" => box.name, + "provider" => box.provider.to_s, + "version" => box.version, + }) + env.machine_index.release(entry) end it "deletes the UUID when setting to nil" do