From 64ff69c64b4500ac16b7896afff01d12de3b9bd5 Mon Sep 17 00:00:00 2001
From: Seth Vargo <sethvargo@gmail.com>
Date: Thu, 26 Nov 2015 13:11:51 -0500
Subject: [PATCH] Only run cleanup tasks when they are defined on the
 provisioner

This helps with some confusion caused in GH-2538, since the output says:

> Running cleanup tasks for 'shell' provisioner...

But that's actually not true. It is running the cleanup tasks iff the
provisioner defined a cleanup task. This commit changes the
provisioner_cleanup middleware to only run cleanup tasks if the subclass
defines a cleanup task.

The reason we can't just check if the provisioner `respond_to?` the
`cleanup` method is because the parent provisioner base class (which
all provisioners inherit from) defines a blank cleanup method. This is
important because it means we never risk calling an unimplemented
cleanup function, and it also helps define the public API for a
provisioner.
---
 .../action/builtin/provisioner_cleanup.rb     | 18 ++++++++++---
 .../builtin/provisioner_cleanup_test.rb       | 26 +++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/lib/vagrant/action/builtin/provisioner_cleanup.rb b/lib/vagrant/action/builtin/provisioner_cleanup.rb
index c76b88f39..ead72268c 100644
--- a/lib/vagrant/action/builtin/provisioner_cleanup.rb
+++ b/lib/vagrant/action/builtin/provisioner_cleanup.rb
@@ -32,10 +32,20 @@ module Vagrant
 
           # Ask the provisioners to modify the configuration if needed
           provisioner_instances(env).each do |p, _|
-            env[:ui].info(I18n.t(
-              "vagrant.provisioner_cleanup",
-              name: type_map[p].to_s))
-            p.cleanup
+            name = type_map[p].to_s
+
+            # Check if the subclass defined a cleanup method. The parent
+            # provisioning class defines a `cleanup` method, so we cannot use
+            # `respond_to?` here. Instead, we have to check if _this_ instance
+            # defines a cleanup task.
+            if p.public_methods(false).include?(:cleanup)
+              env[:ui].info(I18n.t("vagrant.provisioner_cleanup",
+                name: name,
+              ))
+              p.cleanup
+            else
+              @logger.debug("Skipping cleanup tasks for `#{name}' - not defined")
+            end
           end
         end
       end
diff --git a/test/unit/vagrant/action/builtin/provisioner_cleanup_test.rb b/test/unit/vagrant/action/builtin/provisioner_cleanup_test.rb
index 00132f616..2d3b907c7 100644
--- a/test/unit/vagrant/action/builtin/provisioner_cleanup_test.rb
+++ b/test/unit/vagrant/action/builtin/provisioner_cleanup_test.rb
@@ -55,4 +55,30 @@ describe Vagrant::Action::Builtin::ProvisionerCleanup do
       instance.call(env)
     end
   end
+
+  it "only runs cleanup tasks if the subclass defines it" do
+    parent = Class.new do
+      class_variable_set(:@@cleanup, false)
+
+      def self.called?
+        class_variable_get(:@@cleanup)
+      end
+
+      def cleanup
+        self.class.class_variable_set(:@@cleanup)
+      end
+    end
+
+    child = Class.new(parent)
+
+    allow_any_instance_of(described_class).to receive(:provisioner_type_map)
+      .and_return(child => :test_provisioner)
+    allow_any_instance_of(described_class).to receive(:provisioner_instances)
+      .and_return([child])
+
+    expect(parent.called?).to be(false)
+    instance = described_class.new(app, env)
+    instance.call(env)
+    expect(parent.called?).to be(false)
+  end
 end