Merge pull request #8619 from briancain/7810/master/snapshot-names

Enforce unique snapshot names

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