From 87ec540994c6dd6c2f51d18367ba483e56ebe481 Mon Sep 17 00:00:00 2001
From: Darragh Bailey <daragh.bailey@gmail.com>
Date: Sat, 4 Dec 2021 21:03:53 +0000
Subject: [PATCH] Rework prevent destroy if provider completed (#1422)

Adjust handling to use a method reference instead of attempting to
update the env value as that only appears to be available to actions
further down the chain and any changes are not visible during recovery
when exercised for real.

By passing a function reference can limit the actions that can be taken
to only allow a single call that will set a flag internal to the clean
up action.

Fixes: #1328
---
 .../action/cleanup_on_failure.rb              | 23 ++++++++++++++-----
 spec/unit/action/cleanup_on_failure_spec.rb   |  3 ++-
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/vagrant-libvirt/action/cleanup_on_failure.rb b/lib/vagrant-libvirt/action/cleanup_on_failure.rb
index 1baf741..c95484c 100644
--- a/lib/vagrant-libvirt/action/cleanup_on_failure.rb
+++ b/lib/vagrant-libvirt/action/cleanup_on_failure.rb
@@ -8,10 +8,16 @@ module VagrantPlugins
         def initialize(app, _env)
           @logger = Log4r::Logger.new('vagrant_libvirt::action::cleanup_on_failure')
           @app = app
+          @cleanup = true
         end
 
         def call(env)
-          env['vagrant-libvirt.provider'] = :starting
+          # passing a value doesn't work as the env that is updated may be dupped from
+          # the original meaning the latter action's update is discarded. Instead pass
+          # a reference to the method on this class that will toggle the instance
+          # variable indicating whether cleanup is needed or not.
+          env['vagrant-libvirt.complete'] = method(:completed)
+
           @app.call(env)
         end
 
@@ -19,8 +25,8 @@ module VagrantPlugins
           return unless env[:machine] && env[:machine].state.id != :not_created
 
           # only destroy if failed to complete bring up
-          if env['vagrant-libvirt.provider'] == :finished
-            @logger.info("VM completed provider setup, no need to teardown")
+          unless @cleanup
+            @logger.debug('VM provider setup was completed, no need to halt/destroy')
             return
           end
 
@@ -40,22 +46,27 @@ module VagrantPlugins
             env[:action_runner].run(Action.action_destroy, destroy_env)
           end
         end
+
+        def completed
+          @cleanup = false
+        end
       end
 
       class SetupComplete
         def initialize(app, _env)
-          @logger = Log4r::Logger.new('vagrant_libvirt::action::cleanup_on_failure')
+          @logger = Log4r::Logger.new('vagrant_libvirt::action::setup_complete')
           @app = app
         end
 
         def call(env)
-          if env['vagrant-libvirt.provider'].nil?
+          if env['vagrant-libvirt.complete'].nil? or !env['vagrant-libvirt.complete'].respond_to? :call
             raise Errors::CallChainError, require_action: CleanupOnFailure.name, current_action: SetupComplete.name
           end
 
+          @logger.debug('Marking provider setup as completed')
           # mark provider as finished setup so that any failure after this
           # point doesn't result in destroying or shutting down the VM
-          env['vagrant-libvirt.provider'] = :finished
+          env['vagrant-libvirt.complete'].call
 
           @app.call(env)
         end
diff --git a/spec/unit/action/cleanup_on_failure_spec.rb b/spec/unit/action/cleanup_on_failure_spec.rb
index 7fdcca5..abb0167 100644
--- a/spec/unit/action/cleanup_on_failure_spec.rb
+++ b/spec/unit/action/cleanup_on_failure_spec.rb
@@ -40,6 +40,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::CleanupOnFailure do
     allow(machine).to receive(:state).and_return(state)
 
     allow(logger).to receive(:info)
+    allow(logger).to receive(:debug)
     allow(logger).to receive(:error)
 
     allow(runner).to receive(:run).and_call_original
@@ -120,7 +121,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::CleanupOnFailure do
         it 'should not perform halt or destroy' do
           expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_halt)
           expect(VagrantPlugins::ProviderLibvirt::Action).to_not receive(:action_destroy)
-          expect(logger).to receive(:info).with('VM completed provider setup, no need to teardown')
+          expect(logger).to receive(:debug).with('VM provider setup was completed, no need to halt/destroy')
 
           expect { runner.run(action_chain) }.to raise_error(Exception, "some action failed")
         end