Make action read-only, set at instance creation time.mcquin/Issue-1862

2 files changed, 88 insertions, 66 deletions

diff --git a/lib/chef/provider.rb b/lib/chef/provider.rb
index bdfe826944..f6afa939f0 100644
--- a/lib/chef/provider.rb
+++ b/lib/chef/provider.rb
@@ -35,20 +35,24 @@ class Chef
     attr_reader :recipe_name
     attr_reader :cookbook_name
 
-    #--
-    # TODO: this should be a reader, and the action should be passed in the
-    # constructor; however, many/most subclasses override the constructor so
-    # changing the arity would be a breaking change. Change this at the next
-    # break, e.g., Chef 11.
-    attr_accessor :action
-
-    def initialize(new_resource, run_context)
+    attr_reader :action
+
+    # @TODO Chef 13: Make action a required parameter.
+    # action is an optional parameter for Chef 12 so custom provider
+    # classes can be updated.
+    def initialize(new_resource, run_context, action=nil)
       @new_resource = new_resource
-      @action = action
       @current_resource = nil
       @run_context = run_context
       @converge_actions = nil
 
+      if action.nil?
+        Chef::Log.warn("Initializing a provider without an action parameter is deprecated" +
+          " in Chef 12 and will be removed in Chef 13. It is recommended that you update" +
+          " custom providers to reflect these changes.")
+      end
+      @action = action
+
       @recipe_name = nil
       @cookbook_name = nil
     end
@@ -93,11 +97,14 @@ class Chef
       run_context.events
     end
 
+    # @TODO Chef 13: Remove optional parameter action.
     def run_action(action=nil)
-      @action = action unless action.nil?
-
-      # TODO: it would be preferable to get the action to be executed in the
-      # constructor...
+      unless action.nil?
+        Chef::Log.warn("Specifying an action to #run_action is deprecated in" +
+          " Chef 12 and will be removed in Chef 13. An action should be supplied" +
+          " to the provider constructor.")
+        @action = action
+      end
 
       # user-defined LWRPs may include unsafe load_current_resource methods that cannot be run in whyrun mode
       if !whyrun_mode? || whyrun_supported?
diff --git a/spec/unit/provider_spec.rb b/spec/unit/provider_spec.rb
index 9b89fc1888..8fc6195403 100644
--- a/spec/unit/provider_spec.rb
+++ b/spec/unit/provider_spec.rb
@@ -50,109 +50,124 @@ class ConvergeActionDemonstrator < Chef::Provider
 end
 
 describe Chef::Provider do
-  before(:each) do
-    @cookbook_collection = Chef::CookbookCollection.new([])
-    @node = Chef::Node.new
-    @node.name "latte"
-    @events = Chef::EventDispatch::Dispatcher.new
-    @run_context = Chef::RunContext.new(@node, @cookbook_collection, @events)
-    @resource = Chef::Resource.new("funk", @run_context)
-    @resource.cookbook_name = "a_delicious_pie"
-    @provider = Chef::Provider.new(@resource, @run_context)
+  let(:cookbook_collection) { Chef::CookbookCollection.new([]) }
+
+  let(:node) do
+    n = Chef::Node.new
+    n.name "latte"
+    n
+  end
+
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+
+  let(:run_context) { Chef::RunContext.new(node, cookbook_collection, events) }
+
+  let(:resource) do
+    res = Chef::Resource.new("funk", run_context)
+    res.cookbook_name = "a_delicious_pie"
+    res
   end
 
+  let(:provider) { Chef::Provider.new(resource, run_context, :nothing) }
+
   it "should mixin shell_out" do
-    expect(@provider.respond_to?(:shell_out)).to be true
+    expect(provider.respond_to?(:shell_out)).to be true
   end
 
   it "should mixin shell_out!" do
-    expect(@provider.respond_to?(:shell_out!)).to be true
+    expect(provider.respond_to?(:shell_out!)).to be true
   end
 
   it "should mixin shell_out_with_systems_locale" do
-    expect(@provider.respond_to?(:shell_out_with_systems_locale)).to be true
+    expect(provider.respond_to?(:shell_out_with_systems_locale)).to be true
   end
 
   it "should store the resource passed to new as new_resource" do
-    @provider.new_resource.should eql(@resource)
+    provider.new_resource.should eql(resource)
   end
 
   it "should store the node passed to new as node" do
-    @provider.node.should eql(@node)
+    provider.node.should eql(node)
   end
 
   it "should have nil for current_resource by default" do
-    @provider.current_resource.should eql(nil)
+    provider.current_resource.should eql(nil)
   end
 
   it "should not support whyrun by default" do
-    @provider.send(:whyrun_supported?).should eql(false)
+    provider.send(:whyrun_supported?).should eql(false)
   end
 
   it "should return true for action_nothing" do
-    @provider.action_nothing.should eql(true)
+    provider.action_nothing.should eql(true)
   end
 
   it "evals embedded recipes with a pristine resource collection" do
-    @provider.run_context.instance_variable_set(:@resource_collection, "doesn't matter what this is")
+    provider.run_context.instance_variable_set(:@resource_collection, "doesn't matter what this is")
     temporary_collection = nil
-    snitch = Proc.new {temporary_collection = @run_context.resource_collection}
-    @provider.send(:recipe_eval, &snitch)
-    temporary_collection.should be_an_instance_of(Chef::ResourceCollection)
-    @provider.run_context.instance_variable_get(:@resource_collection).should == "doesn't matter what this is"
-  end
+    snitch = Proc.new {temporary_collection = run_context.resource_collection}
+    provider.send(:recipe_eval, &snitch)
+    expect(temporary_collection).to be_an_instance_of(Chef::ResourceCollection)
+    expect(provider.run_context.instance_variable_get(:@resource_collection)).to eq "doesn't matter what this is"
+  end
+
+  describe "when creating the temporary run context" do
+    let(:run_context) do
+      rc = Chef::RunContext.new(node, cookbook_collection, events)
+      # we actually want to test that RunContext#load is never called, but we
+      # can't stub all instances of an object with rspec's mocks. :/
+      allow(Chef::RunContext).to receive(:new).and_raise("not supposed to happen again")
+      rc
+    end
+    let(:snitch) { Proc.new { temporary_collection = run_context.resource_collection } }
 
-  it "does not re-load recipes when creating the temporary run context" do
-    # we actually want to test that RunContext#load is never called, but we
-    # can't stub all instances of an object with rspec's mocks. :/
-    Chef::RunContext.stub(:new).and_raise("not supposed to happen")
-    snitch = Proc.new {temporary_collection = @run_context.resource_collection}
-    @provider.send(:recipe_eval, &snitch)
+    it "does not re-load recipes" do
+      provider.send(:recipe_eval, &snitch)
+    end
   end
 
   context "when no converge actions are queued" do
     before do
-      @provider.stub(:whyrun_supported?).and_return(true)
-      @provider.stub(:load_current_resource)
+      allow(provider).to receive(:whyrun_supported?).and_return(true)
+      allow(provider).to receive(:load_current_resource)
     end
 
     it "does not mark the new resource as updated" do
-      @resource.should_not be_updated
-      @resource.should_not be_updated_by_last_action
+      expect(resource).to_not be_updated
+      expect(resource).to_not be_updated_by_last_action
     end
   end
 
   context "when converge actions have been added to the queue" do
     describe "and provider supports whyrun mode" do
-      before do
-        @provider = ConvergeActionDemonstrator.new(@resource, @run_context)
-      end
+      let(:provider) { ConvergeActionDemonstrator.new(resource, run_context, :foo) }
 
       it "should tell us that it does support whyrun" do
-        @provider.should be_whyrun_supported
+        expect(provider).to be_whyrun_supported
       end
 
       it "queues up converge actions" do
-        @provider.action_foo
-        @provider.send(:converge_actions).should have(1).actions
+        provider.action_foo
+        expect(provider.send(:converge_actions)).to have(1).actions
       end
 
       it "executes pending converge actions to converge the system" do
-        @provider.run_action(:foo)
-        @provider.instance_variable_get(:@system_state_altered).should be_true
+        provider.run_action
+        expect(provider.instance_variable_get(:@system_state_altered)).to be_true
       end
 
       it "marks the resource as updated" do
-        @provider.run_action(:foo)
-        @resource.should be_updated
-        @resource.should be_updated_by_last_action
+        provider.run_action
+        expect(resource).to be_updated
+        expect(resource).to be_updated_by_last_action
       end
     end
 
     describe "and provider does not support whyrun mode" do
+      let(:provider) { NoWhyrunDemonstrator.new(resource, run_context, :foo) }
+
       before do
         Chef::Config[:why_run] = true
-        @provider = NoWhyrunDemonstrator.new(@resource, @run_context)
       end
 
       after do
@@ -160,20 +175,20 @@ describe Chef::Provider do
       end
 
       it "should tell us that it doesn't support whyrun" do
-        @provider.should_not be_whyrun_supported
+        expect(provider).to_not be_whyrun_supported
       end
 
       it "should automatically generate a converge_by block on the provider's behalf" do
-        @provider.run_action(:foo)
-        @provider.send(:converge_actions).should have(0).actions
-        @provider.system_state_altered.should be_false
+        provider.run_action
+        expect(provider.send(:converge_actions)).to have(0).actions
+        expect(provider.system_state_altered).to be_false
       end
 
       it "should automatically execute the generated converge_by block" do
-        @provider.run_action(:foo)
-        @provider.system_state_altered.should be_false
-        @resource.should_not be_updated
-        @resource.should_not be_updated_by_last_action
+        provider.run_action
+        expect(provider.system_state_altered).to be_false
+        expect(resource).to_not be_updated
+        expect(resource).to_not be_updated_by_last_action
       end
     end
   end