From 6ddba4f7b315b0e36c1d4cf7e67815c7762f66ef Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Wed, 24 May 2017 11:21:36 -0700 Subject: [PATCH] (#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 | 4 + plugins/commands/snapshot/command/save.rb | 22 ++++- templates/locales/en.yml | 2 + .../commands/snapshot/command/save_test.rb | 95 +++++++++++++++++++ 4 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 test/unit/plugins/commands/snapshot/command/save_test.rb diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 627038eb9..f1c2cb3e4 100644 --- a/lib/vagrant/errors.rb +++ b/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 diff --git a/plugins/commands/snapshot/command/save.rb b/plugins/commands/snapshot/command/save.rb index 496891eac..28d9afe0c 100644 --- a/plugins/commands/snapshot/command/save.rb +++ b/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] " @@ -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 diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 287079eb5..f4e182a31 100644 --- a/templates/locales/en.yml +++ b/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 diff --git a/test/unit/plugins/commands/snapshot/command/save_test.rb b/test/unit/plugins/commands/snapshot/command/save_test.rb new file mode 100644 index 000000000..2dbdf4145 --- /dev/null +++ b/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