From 87b75146033f61608b24cd5b29df97acc30bd19f Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Tue, 6 Jun 2017 14:14:55 -0700 Subject: [PATCH] (#7188) Clean up vagrant snapshot restore/delete error messages This commit adds some better handling around the snapshot restore and delete commands for the virtualbox provider. If a user attempts to restore from a vm that does not exist, instead of exiting 0 it will raise an exception saying the virtual machine has not been created yet. Addtionally, if a user attempts to restore from a snapshot id that does not exist, instead of printing a complicated exception from the virtualbox cli tool, it prints a more useful error message telling the user that the snapshot does not exist. --- lib/vagrant/errors.rb | 4 + plugins/commands/snapshot/command/delete.rb | 14 ++- plugins/commands/snapshot/command/restore.rb | 14 ++- plugins/providers/virtualbox/action.rb | 5 +- templates/locales/en.yml | 3 + .../commands/snapshot/command/delete_test.rb | 97 +++++++++++++++++++ .../commands/snapshot/command/restore_test.rb | 97 +++++++++++++++++++ 7 files changed, 229 insertions(+), 5 deletions(-) create mode 100644 test/unit/plugins/commands/snapshot/command/delete_test.rb create mode 100644 test/unit/plugins/commands/snapshot/command/restore_test.rb diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index b3255e534..42b732c42 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -656,6 +656,10 @@ module Vagrant error_key(:snapshot_force) end + class SnapshotNotFound < VagrantError + error_key(:snapshot_not_found) + end + class SnapshotNotSupported < VagrantError error_key(:snapshot_not_supported) end diff --git a/plugins/commands/snapshot/command/delete.rb b/plugins/commands/snapshot/command/delete.rb index 0d5118847..aa822b28d 100644 --- a/plugins/commands/snapshot/command/delete.rb +++ b/plugins/commands/snapshot/command/delete.rb @@ -23,7 +23,19 @@ module VagrantPlugins name = argv.pop with_target_vms(argv) do |vm| - vm.action(:snapshot_delete, snapshot_name: name) + if !vm.provider.capability?(:snapshot_list) + raise Vagrant::Errors::SnapshotNotSupported + end + + snapshot_list = vm.provider.capability(:snapshot_list) + + if snapshot_list.include? name + vm.action(:snapshot_delete, snapshot_name: name) + else + raise Vagrant::Errors::SnapshotNotFound, + snapshot_name: name, + machine: vm.name.to_s + end end # Success, exit status 0 diff --git a/plugins/commands/snapshot/command/restore.rb b/plugins/commands/snapshot/command/restore.rb index 18ca0fe04..1d8df2ca3 100644 --- a/plugins/commands/snapshot/command/restore.rb +++ b/plugins/commands/snapshot/command/restore.rb @@ -36,7 +36,19 @@ module VagrantPlugins options[:snapshot_name] = name with_target_vms(argv) do |vm| - vm.action(:snapshot_restore, options) + if !vm.provider.capability?(:snapshot_list) + raise Vagrant::Errors::SnapshotNotSupported + end + + snapshot_list = vm.provider.capability(:snapshot_list) + + if snapshot_list.include? name + vm.action(:snapshot_restore, options) + else + raise Vagrant::Errors::SnapshotNotFound, + snapshot_name: name, + machine: vm.name.to_s + end end # Success, exit status 0 diff --git a/plugins/providers/virtualbox/action.rb b/plugins/providers/virtualbox/action.rb index 1f0130791..39c4ac61b 100644 --- a/plugins/providers/virtualbox/action.rb +++ b/plugins/providers/virtualbox/action.rb @@ -245,14 +245,13 @@ module VagrantPlugins end end - # This is the action that is primarily responsible for saving a snapshot + # This is the action that is primarily responsible for restoring a snapshot def self.action_snapshot_restore Vagrant::Action::Builder.new.tap do |b| b.use CheckVirtualbox b.use Call, Created do |env, b2| if !env[:result] - b2.use MessageNotCreated - next + raise Vagrant::Errors::VMNotCreatedError end b2.use CheckAccessible diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 5e36d67a4..c173dfe70 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1167,6 +1167,9 @@ en: This may be intentional or this may be a bug. If this provider should support snapshots, then please report this as a bug to the maintainer of the provider. + snapshot_not_found: |- + The snapshot name `%{snapshot_name}` was not found for the + virtual machine `%{machine}`. ssh_authentication_failed: |- SSH authentication failed! This is typically caused by the public/private keypair for the SSH user not being properly set on the guest VM. Please diff --git a/test/unit/plugins/commands/snapshot/command/delete_test.rb b/test/unit/plugins/commands/snapshot/command/delete_test.rb new file mode 100644 index 000000000..438c9c05e --- /dev/null +++ b/test/unit/plugins/commands/snapshot/command/delete_test.rb @@ -0,0 +1,97 @@ +require File.expand_path("../../../../../base", __FILE__) + +require Vagrant.source_root.join("plugins/commands/snapshot/command/delete") + +describe VagrantPlugins::CommandSnapshot::Command::Delete do + include_context "unit" + + let(:iso_env) do + # We have to create a Vagrantfile so there is a root path + env = isolated_environment + env.vagrantfile("") + env.create_vagrant_env + end + + let(:guest) { double("guest") } + let(:host) { double("host") } + let(:machine) { iso_env.machine(iso_env.machine_names[0], :dummy) } + + let(:argv) { [] } + + subject { described_class.new(argv, iso_env) } + + before do + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return([]) + + allow(machine.provider).to receive(:capability?).with(:snapshot_list). + and_return(true) + + allow(subject).to receive(:with_target_vms) { |&block| block.call machine } + end + + describe "execute" do + context "with no arguments" do + it "shows help" do + expect { subject.execute }. + to raise_error(Vagrant::Errors::CLIInvalidUsage) + end + end + + context "with an unsupported provider" do + let(:argv) { ["test"] } + + before do + allow(machine.provider).to receive(:capability?).with(:snapshot_list). + and_return(false) + end + + it "raises an exception" do + machine.id = "foo" + expect { subject.execute }. + to raise_error(Vagrant::Errors::SnapshotNotSupported) + end + end + + context "with a snapshot name given" do + let(:argv) { ["test"] } + it "calls snapshot_delete with a snapshot name" do + machine.id = "foo" + + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return(["test"]) + + expect(machine).to receive(:action) do |name, opts| + expect(name).to eq(:snapshot_delete) + expect(opts[:snapshot_name]).to eq("test") + end + + expect(subject.execute).to eq(0) + end + + it "doesn't delete a snapshot on a non-existent machine" do + machine.id = nil + + expect(subject).to receive(:with_target_vms){} + + expect(machine).to_not receive(:action) + expect(subject.execute).to eq(0) + end + end + + context "with a snapshot name that doesn't exist" do + let(:argv) { ["nopetest"] } + + it "fails to take a snapshot and prints a warning to the user" do + machine.id = "foo" + + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return(["test"]) + + expect(machine).to_not receive(:action) + expect { subject.execute }. + to raise_error(Vagrant::Errors::SnapshotNotFound) + end + end + end +end diff --git a/test/unit/plugins/commands/snapshot/command/restore_test.rb b/test/unit/plugins/commands/snapshot/command/restore_test.rb new file mode 100644 index 000000000..8747998f1 --- /dev/null +++ b/test/unit/plugins/commands/snapshot/command/restore_test.rb @@ -0,0 +1,97 @@ +require File.expand_path("../../../../../base", __FILE__) + +require Vagrant.source_root.join("plugins/commands/snapshot/command/restore") + +describe VagrantPlugins::CommandSnapshot::Command::Restore do + include_context "unit" + + let(:iso_env) do + # We have to create a Vagrantfile so there is a root path + env = isolated_environment + env.vagrantfile("") + env.create_vagrant_env + end + + let(:guest) { double("guest") } + let(:host) { double("host") } + let(:machine) { iso_env.machine(iso_env.machine_names[0], :dummy) } + + let(:argv) { [] } + + subject { described_class.new(argv, iso_env) } + + before do + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return([]) + + allow(machine.provider).to receive(:capability?).with(:snapshot_list). + and_return(true) + + allow(subject).to receive(:with_target_vms) { |&block| block.call machine } + end + + describe "execute" do + context "with no arguments" do + it "shows help" do + expect { subject.execute }. + to raise_error(Vagrant::Errors::CLIInvalidUsage) + end + end + + context "with an unsupported provider" do + let(:argv) { ["test"] } + + before do + allow(machine.provider).to receive(:capability?).with(:snapshot_list). + and_return(false) + end + + it "raises an exception" do + machine.id = "foo" + expect { subject.execute }. + to raise_error(Vagrant::Errors::SnapshotNotSupported) + end + end + + context "with a snapshot name given" do + let(:argv) { ["test"] } + it "calls snapshot_delete with a snapshot name" do + machine.id = "foo" + + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return(["test"]) + + expect(machine).to receive(:action) do |name, opts| + expect(name).to eq(:snapshot_restore) + expect(opts[:snapshot_name]).to eq("test") + end + + expect(subject.execute).to eq(0) + end + + it "doesn't delete a snapshot on a non-existent machine" do + machine.id = nil + + expect(subject).to receive(:with_target_vms){} + + expect(machine).to_not receive(:action) + expect(subject.execute).to eq(0) + end + end + + context "with a snapshot name that doesn't exist" do + let(:argv) { ["nopetest"] } + + it "fails to take a snapshot and prints a warning to the user" do + machine.id = "foo" + + allow(machine.provider).to receive(:capability).with(:snapshot_list). + and_return(["test"]) + + expect(machine).to_not receive(:action) + expect { subject.execute }. + to raise_error(Vagrant::Errors::SnapshotNotFound) + end + end + end +end