[CHEF-3506] suppress final node save when using override run list

6 files changed, 36 insertions, 7 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index cb4a9e1224..722c9915e9 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -265,12 +265,12 @@ class Chef
 
 
     def save_updated_node
-      unless Chef::Config[:solo]
+      if Chef::Config[:solo]
+        # nothing to do
+      elsif policy_builder.temporary_policy?
+        Chef::Log.warn("Skipping final node save because override_runlist was given")
+      else
         Chef::Log.debug("Saving the current state of node #{node_name}")
-        if(@original_runlist)
-          @node.run_list(*@original_runlist)
-          @node.automatic_attrs[:runlist_override_history] = {Time.now.to_i => @override_runlist.inspect}
-        end
         @node.save
       end
     end
diff --git a/lib/chef/policy_builder/expand_node_object.rb b/lib/chef/policy_builder/expand_node_object.rb
index dbb11380c8..38b8b7551b 100644
--- a/lib/chef/policy_builder/expand_node_object.rb
+++ b/lib/chef/policy_builder/expand_node_object.rb
@@ -186,6 +186,13 @@ class Chef
         cookbook_hash
       end
 
+      # Indicates whether the policy is temporary, which means an
+      # override_runlist was provided. Chef::Client uses this to decide whether
+      # to do the final node save at the end of the run or not.
+      def temporary_policy?
+        !!@original_runlist
+      end
+
       ########################################
       # Internal public API
       ########################################
diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb
index e9041d5c0f..19b9aaf542 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -186,6 +186,12 @@ class Chef
         cookbooks_to_sync
       end
 
+      # Whether or not this is a temporary policy. Since PolicyBuilder doesn't
+      # support override_runlist, this is always false.
+      def temporary_policy?
+        false
+      end
+
       ## Internal Public API ##
 
       def run_list_with_versions_for_display
diff --git a/spec/unit/client_spec.rb b/spec/unit/client_spec.rb
index 6dc10c21b8..58a643d403 100644
--- a/spec/unit/client_spec.rb
+++ b/spec/unit/client_spec.rb
@@ -422,7 +422,7 @@ shared_examples_for Chef::Client do
       @client = Chef::Client.new(nil, :override_runlist => 'role[a], role[b]')
     end
 
-    it "should override the run list and save original runlist" do
+    it "should override the run list and skip the final node save" do
       @client = Chef::Client.new(nil, :override_runlist => 'role[test_role]')
       @client.node = @node
 
@@ -438,7 +438,7 @@ shared_examples_for Chef::Client do
       mock_chef_rest.should_receive(:get_rest).with("roles/test_role").and_return(override_role)
       Chef::REST.should_receive(:new).and_return(mock_chef_rest)
 
-      @node.should_receive(:save).and_return(nil)
+      @node.should_not_receive(:save)
 
       @client.policy_builder.stub(:node).and_return(@node)
       @client.policy_builder.build_node
diff --git a/spec/unit/policy_builder/expand_node_object_spec.rb b/spec/unit/policy_builder/expand_node_object_spec.rb
index dc8b532e2b..5c6f39d28c 100644
--- a/spec/unit/policy_builder/expand_node_object_spec.rb
+++ b/spec/unit/policy_builder/expand_node_object_spec.rb
@@ -59,6 +59,10 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
       expect(policy_builder).to respond_to(:sync_cookbooks)
     end
 
+    it "implements a temporary_policy? method" do
+      expect(policy_builder).to respond_to(:temporary_policy?)
+    end
+
     describe "loading the node" do
 
       context "on chef-solo" do
@@ -181,6 +185,10 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
       expect(node["fqdn"]).to eq(ohai_data["fqdn"])
     end
 
+    it "reports that a temporary_policy is not being used" do
+      expect(policy_builder.temporary_policy?).to be_false
+    end
+
     describe "when the given run list is not in expanded form" do
 
       # NOTE: for chef-client, the behavior is always to expand the run list,
@@ -239,6 +247,10 @@ describe Chef::PolicyBuilder::ExpandNodeObject do
         expect(policy_builder.original_runlist).to eq(primary_runlist)
       end
 
+      it "reports that a temporary policy is being used" do
+        expect(policy_builder.temporary_policy?).to be_true
+      end
+
     end
 
     context "when no environment is specified" do
diff --git a/spec/unit/policy_builder/policyfile_spec.rb b/spec/unit/policy_builder/policyfile_spec.rb
index aeda1af773..f02c79ef12 100644
--- a/spec/unit/policy_builder/policyfile_spec.rb
+++ b/spec/unit/policy_builder/policyfile_spec.rb
@@ -106,6 +106,10 @@ describe Chef::PolicyBuilder::Policyfile do
       Chef::PolicyBuilder::Policyfile.new(node_name, ohai_data, json_attribs, override_runlist, events)
     end
 
+    it "always gives `false` for #temporary_policy?" do
+      expect(initialize_pb.temporary_policy?).to be_false
+    end
+
     context "chef-solo" do
       before { Chef::Config[:solo] = true }