(#7810) Enforce unique snapshot names

Prior to this commit, the vagrant snapshot plugin would save snapshots
with existing names which lead to duplicate snapshot names being saved.
This commit fixes that by checking to see if the given snapshot name
already exists and if so, fails telling the user the given snapshot name
already exists. If a user passes a --force flag, vagrant will first
delete the existing snapshot, and take a new one with the given name.

lib/vagrant/errors.rb

@@ -652,6 +652,10 @@ module Vagrant
       error_key(:shell_expand_failed)
     end
 
+    class SnapshotConflictFailed < VagrantError
+      error_key(:snapshot_force)
+    end
+
     class SSHAuthenticationFailed < VagrantError
       error_key(:ssh_authentication_failed)
     end

plugins/commands/snapshot/command/save.rb

@@ -6,6 +6,7 @@ module VagrantPlugins
       class Save < Vagrant.plugin("2", :command)
         def execute
           options = {}
+          options[:force] = false
 
           opts = OptionParser.new do |o|
             o.banner = "Usage: vagrant snapshot save [options] [vm-name] <name>"
@@ -16,6 +17,10 @@ module VagrantPlugins
             o.separator ""
             o.separator "Snapshots are useful for experimenting in a machine and being able"
             o.separator "to rollback quickly."
+
+            o.on("-f", "--force", "Replace snapshot without confirmation") do |f|
+              options[:force] = f
+            end
           end
 
           # Parse the options
@@ -28,7 +33,22 @@ module VagrantPlugins
 
           name = argv.pop
           with_target_vms(argv) do |vm|
-            vm.action(:snapshot_save, snapshot_name: name)
+            if !vm.provider.capability?(:snapshot_list)
+              vm.ui.info(I18n.t("vagrant.commands.snapshot.not_supported"))
+              next
+            end
+
+            snapshot_list = vm.provider.capability(:snapshot_list)
+
+            if !snapshot_list.include? name
+              vm.action(:snapshot_save, snapshot_name: name)
+            elsif options[:force]
+              # not a unique snapshot name
+              vm.action(:snapshot_delete, snapshot_name: name)
+              vm.action(:snapshot_save, snapshot_name: name)
+            else
+              raise Vagrant::Errors::SnapshotConflictFailed
+            end
           end
 
           # Success, exit status 0

templates/locales/en.yml

@@ -1156,6 +1156,8 @@ en:
         (probably for one of your shared folders). This is an extremely rare
         error case and most likely indicates an unusual configuration of the
         guest system. Please report a bug with your Vagrantfile and debug log.
+      snapshot_force: |-
+        You must include the `--force` option to replace an existing snapshot.
       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/save_test.rb

@@ -0,0 +1,95 @@
+require File.expand_path("../../../../../base", __FILE__)
+
+require Vagrant.source_root.join("plugins/commands/snapshot/command/save")
+
+describe VagrantPlugins::CommandSnapshot::Command::Save 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 a snapshot name given" do
+      let(:argv)     { ["test"] }
+      it "calls snapshot_save with a snapshot name" do
+        machine.id = "foo"
+
+        expect(machine).to receive(:action) do |name, opts|
+          expect(name).to eq(:snapshot_save)
+          expect(opts[:snapshot_name]).to eq("test")
+        end
+
+        expect(subject.execute).to eq(0)
+      end
+
+      it "doesn't snapshot 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 duplicate snapshot name given and no force flag" do
+      let(:argv)     { ["test"] }
+
+      it "fails to take a snapshot and prints a warning to the user" do
+        machine.id = "fool"
+
+        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::SnapshotConflictFailed)
+      end
+    end
+
+    context "with a duplicate snapshot name given and a force flag" do
+      let(:argv)     { ["test", "--force"] }
+
+      it "deletes the existing snapshot and takes a new one" do
+        machine.id = "foo"
+
+        allow(machine.provider).to receive(:capability).with(:snapshot_list).
+          and_return(["test"])
+
+        expect(machine).to receive(:action).with(:snapshot_delete, snapshot_name: "test")
+        expect(machine).to receive(:action).with(:snapshot_save, snapshot_name: "test")
+
+        expect(subject.execute).to eq(0)
+      end
+    end
+  end
+end