(#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

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

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

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

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
+              raise Vagrant::Errors::VMNotCreatedError
-              next
             end
 
             b2.use CheckAccessible

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

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

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