Merge pull request #2032 from opscode/lcg/1785

Rebase and ChangeLOG for pr/1785

5 files changed, 55 insertions, 42 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index daf680b047..0344d0da83 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Unreleased: 12.0.0
 
+* [**Kazuki Saito**](https://github.com/sakazuki):
+  CHEF-4933: idempotency fixes for ifconfig provider
 * [**Kirill Shirinkin**](https://github.com/Fodoj):
   The knife bootstrap command expands the path of the secret-file
 * [**Malte Swart**](https://github.com/mswart):
diff --git a/lib/chef/provider/ifconfig.rb b/lib/chef/provider/ifconfig.rb
index 31f88e5406..ac52100b56 100644
--- a/lib/chef/provider/ifconfig.rb
+++ b/lib/chef/provider/ifconfig.rb
@@ -19,6 +19,7 @@
 require 'chef/log'
 require 'chef/mixin/command'
 require 'chef/provider'
+require 'chef/resource/file'
 require 'chef/exceptions'
 require 'erb'
 
@@ -109,11 +110,11 @@ class Chef
                 :command => command
               )
               Chef::Log.info("#{@new_resource} added")
-              # Write out the config files
-              generate_config
             end
           end
         end
+        # Write out the config files
+        generate_config
       end
 
       def action_enable
@@ -140,12 +141,12 @@ class Chef
             run_command(
               :command => command
             )
-            delete_config
             Chef::Log.info("#{@new_resource} deleted")
           end
         else
           Chef::Log.debug("#{@new_resource} does not exist - nothing to do")
         end
+        delete_config
       end
 
       def action_disable
@@ -168,27 +169,25 @@ class Chef
         ! @config_template.nil? and ! @config_path.nil?
       end
 
+      def resource_for_config(path)
+        Chef::Resource::File.new(path, run_context)
+      end
+
       def generate_config
         return unless can_generate_config?
         b = binding
         template = ::ERB.new(@config_template)
-        converge_by ("generate configuration file : #{@config_path}") do
-          network_file = ::File.new(@config_path, "w")
-          network_file.puts(template.result(b))
-          network_file.close
-        end
-        Chef::Log.info("#{@new_resource} created configuration file")
+        config = resource_for_config(@config_path)
+        config.content(template.result(b))
+        config.run_action(:create)
+        @new_resource.updated_by_last_action(true) if config.updated?
       end
 
       def delete_config
         return unless can_generate_config?
-        require 'fileutils'
-        if ::File.exist?(@config_path)
-          converge_by ("delete the #{@config_path}") do
-            FileUtils.rm_f(@config_path, :verbose => false)
-          end
-        end
-        Chef::Log.info("#{@new_resource} deleted configuration file")
+        config = resource_for_config(@config_path)
+        config.run_action(:delete)
+        @new_resource.updated_by_last_action(true) if config.updated?
       end
 
       private
diff --git a/spec/unit/provider/ifconfig/debian_spec.rb b/spec/unit/provider/ifconfig/debian_spec.rb
index c6a37fdd5b..f4fd1480e0 100644
--- a/spec/unit/provider/ifconfig/debian_spec.rb
+++ b/spec/unit/provider/ifconfig/debian_spec.rb
@@ -56,8 +56,6 @@ describe Chef::Provider::Ifconfig::Debian do
   describe "generate_config" do
 
     context "when writing a file" do
-      let(:config_file_ifcfg) { StringIO.new }
-
       let(:tempfile) { Tempfile.new("rspec-chef-ifconfig-debian") }
 
       let(:tempdir_path) { Dir.mktmpdir("rspec-chef-ifconfig-debian-dir") }
@@ -67,14 +65,13 @@ describe Chef::Provider::Ifconfig::Debian do
       before do
         stub_const("Chef::Provider::Ifconfig::Debian::INTERFACES_FILE", tempfile.path)
         stub_const("Chef::Provider::Ifconfig::Debian::INTERFACES_DOT_D_DIR", tempdir_path)
-        expect(File).to receive(:new).with(config_filename_ifcfg, "w").and_return(config_file_ifcfg)
       end
 
       it "should write a network-script" do
         provider.run_action(:add)
-        expect(config_file_ifcfg.string).to match(/^iface eth0 inet static\s*$/)
-        expect(config_file_ifcfg.string).to match(/^\s+address 10\.0\.0\.1\s*$/)
-        expect(config_file_ifcfg.string).to match(/^\s+netmask 255\.255\.254\.0\s*$/)
+        expect(File.read(config_filename_ifcfg)).to match(/^iface eth0 inet static\s*$/)
+        expect(File.read(config_filename_ifcfg)).to match(/^\s+address 10\.0\.0\.1\s*$/)
+        expect(File.read(config_filename_ifcfg)).to match(/^\s+netmask 255\.255\.254\.0\s*$/)
       end
 
       context "when the interface_dot_d directory does not exist" do
@@ -123,7 +120,6 @@ iface eth0 inet static
   netmask 255.255.254.0
 EOF
         )
-        expect(File).to receive(:new).with(config_filename_ifcfg, "w").and_return(config_file_ifcfg)
         expect(File.exists?(tempdir_path)).to be_true  # since the file exists, the enclosing dir must also exist
       end
 
@@ -139,6 +135,8 @@ EOF
         before do
           tempfile.write(expected_string)
           tempfile.close
+
+          expect(provider).not_to receive(:converge_by).with(/modifying #{tempfile.path} to source #{tempdir_path}/)
         end
 
         it "should preserve all the contents" do
@@ -165,6 +163,9 @@ EOF
         before do
           tempfile.write("a line\nanother line\n")
           tempfile.close
+
+          allow(provider).to receive(:converge_by).and_call_original
+          expect(provider).to receive(:converge_by).with(/modifying #{tempfile.path} to source #{tempdir_path}/).and_call_original
         end
 
         it "should preserve the original contents and add the source line" do
@@ -318,8 +319,17 @@ source #{tempdir_path}/*
 
     it "should delete network-script if it exists" do
       current_resource.device new_resource.device
-      expect(File).to receive(:exist?).with(config_filename_ifcfg).and_return(true)
-      expect(FileUtils).to receive(:rm_f).with(config_filename_ifcfg, :verbose => false)
+
+      [:exist?, :exists?, :writable?].each do |cmd|
+        # need to stub :writable? to make why_run? happy
+        allow(File).to receive(cmd).and_call_original
+        allow(File).to receive(cmd).with(config_filename_ifcfg).and_return(true)
+      end
+
+      # stub for Chef::Util::Backup#do_backup
+      expect(FileUtils).to receive(:cp)
+        .with(config_filename_ifcfg, /#{Chef::Config[:file_backup_path]}/, :preserve => true)
+      expect(File).to receive(:delete).with(config_filename_ifcfg)
 
       provider.run_action(:delete)
     end
diff --git a/spec/unit/provider/ifconfig/redhat_spec.rb b/spec/unit/provider/ifconfig/redhat_spec.rb
index f4b98dfc32..138c2a389d 100644
--- a/spec/unit/provider/ifconfig/redhat_spec.rb
+++ b/spec/unit/provider/ifconfig/redhat_spec.rb
@@ -37,22 +37,26 @@ describe Chef::Provider::Ifconfig::Redhat do
     status = double("Status", :exitstatus => 0)
     @provider.instance_variable_set("@status", status)
     @provider.current_resource = @current_resource
- end
+
+    config_filename = "/etc/sysconfig/network-scripts/ifcfg-#{@new_resource.device}"
+    @config = double("chef-resource-file")
+    @provider.should_receive(:resource_for_config).with(config_filename).and_return(@config)
+  end
 
   describe "generate_config for action_add" do
 
-     it "should write network-script for centos" do
+    it "should write network-script for centos" do
       @provider.stub(:load_current_resource)
       @provider.stub(:run_command)
-      config_filename = "/etc/sysconfig/network-scripts/ifcfg-#{@new_resource.device}"
-      config_file = StringIO.new
-      File.should_receive(:new).with(config_filename, "w").and_return(config_file)
-
+      @config.should_receive(:content) do |arg|
+        arg.should match(/^\s*DEVICE=eth0\s*$/)
+        arg.should match(/^\s*IPADDR=10\.0\.0\.1\s*$/)
+        arg.should match(/^\s*NETMASK=255\.255\.254\.0\s*$/)
+      end
+      @config.should_receive(:run_action).with(:create)
+      @config.should_receive(:updated?).and_return(true)
       @provider.run_action(:add)
-      config_file.string.should match(/^\s*DEVICE=eth0\s*$/)
-      config_file.string.should match(/^\s*IPADDR=10\.0\.0\.1\s*$/)
-      config_file.string.should match(/^\s*NETMASK=255\.255\.254\.0\s*$/)
-     end
+    end
   end
 
   describe "delete_config for action_delete" do
@@ -61,10 +65,8 @@ describe Chef::Provider::Ifconfig::Redhat do
       @current_resource.device @new_resource.device
       @provider.stub(:load_current_resource)
       @provider.stub(:run_command)
-      config_filename =  "/etc/sysconfig/network-scripts/ifcfg-#{@new_resource.device}"
-      File.should_receive(:exist?).with(config_filename).and_return(true)
-      FileUtils.should_receive(:rm_f).with(config_filename, :verbose => false)
-
+      @config.should_receive(:run_action).with(:delete)
+      @config.should_receive(:updated?).and_return(true)
       @provider.run_action(:delete)
     end
   end
diff --git a/spec/unit/provider/ifconfig_spec.rb b/spec/unit/provider/ifconfig_spec.rb
index fb8d0956d7..b09e365c65 100644
--- a/spec/unit/provider/ifconfig_spec.rb
+++ b/spec/unit/provider/ifconfig_spec.rb
@@ -72,7 +72,7 @@ describe Chef::Provider::Ifconfig do
       @provider.stub(:load_current_resource)
       @provider.should_not_receive(:run_command)
       @current_resource.inet_addr "10.0.0.1"
-      @provider.should_not_receive(:generate_config)
+      @provider.should_receive(:generate_config)
 
       @provider.run_action(:add)
       @new_resource.should_not be_updated
@@ -123,7 +123,7 @@ describe Chef::Provider::Ifconfig do
     it "should not delete interface if it does not exist" do
       @provider.stub(:load_current_resource)
       @provider.should_not_receive(:run_command)
-      @provider.should_not_receive(:delete_config)
+      @provider.should_receive(:delete_config)
 
       @provider.run_action(:delete)
       @new_resource.should_not be_updated
@@ -171,7 +171,7 @@ describe Chef::Provider::Ifconfig do
       @provider.stub(:load_current_resource)
       # This is so that nothing actually runs
       @provider.should_not_receive(:run_command)
-      @provider.should_not_receive(:delete_config)
+      @provider.should_receive(:delete_config)
 
       @provider.run_action(:delete)
       @new_resource.should_not be_updated