Merge pull request #10472 from MsysTechnologiesllc/antima/fixes_for_mutltiple_entry_in_fstab

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 59 insertions, 40 deletions

diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb
index e065ce09e5..71cecb47aa 100644
--- a/lib/chef/provider/mount/mount.rb
+++ b/lib/chef/provider/mount/mount.rb
@@ -70,10 +70,6 @@ class Chef
               @current_resource.dump($4.to_i)
               @current_resource.pass($5.to_i)
               logger.trace("Found mount #{device_fstab} to #{@new_resource.mount_point} in /etc/fstab")
-              next
-            when %r{^[/\w]+\s+#{Regexp.escape(@new_resource.mount_point)}\s+}
-              enabled = false
-              logger.trace("Found conflicting mount point #{@new_resource.mount_point} in /etc/fstab")
             end
           end
           @current_resource.enabled(enabled)
@@ -168,36 +164,19 @@ class Chef
 
           if @current_resource.enabled
             # The current options don't match what we have, so
-            # disable, then enable.
-            disable_fs
-          end
-          ::File.open("/etc/fstab", "a") do |fstab|
-            fstab.puts("#{device_fstab} #{@new_resource.mount_point} #{@new_resource.fstype} #{@new_resource.options.nil? ? default_mount_options : @new_resource.options.join(",")} #{@new_resource.dump} #{@new_resource.pass}")
-            logger.trace("#{@new_resource} is enabled at #{@new_resource.mount_point}")
+            # update the last matching entry with current option
+            # and order will remain the same.
+            edit_fstab
+          else
+            ::File.open("/etc/fstab", "a") do |fstab|
+              fstab.puts("#{device_fstab} #{@new_resource.mount_point} #{@new_resource.fstype} #{@new_resource.options.nil? ? default_mount_options : @new_resource.options.join(",")} #{@new_resource.dump} #{@new_resource.pass}")
+              logger.trace("#{@new_resource} is enabled at #{@new_resource.mount_point}")
+            end
           end
         end
 
         def disable_fs
-          if @current_resource.enabled
-            contents = []
-
-            found = false
-            ::File.readlines("/etc/fstab").reverse_each do |line|
-              if !found && line =~ /^#{device_fstab_regex}\s+#{Regexp.escape(@new_resource.mount_point)}\s/
-                found = true
-                logger.trace("#{@new_resource} is removed from fstab")
-                next
-              else
-                contents << line
-              end
-            end
-
-            ::File.open("/etc/fstab", "w") do |fstab|
-              contents.reverse_each { |line| fstab.puts line }
-            end
-          else
-            logger.trace("#{@new_resource} is not enabled - nothing to do")
-          end
+          edit_fstab(remove: true)
         end
 
         def network_device?
@@ -268,6 +247,35 @@ class Chef
             @current_resource.pass == @new_resource.pass
         end
 
+        # It will update or delete the entry from fstab.
+        def edit_fstab(remove: false)
+          if @current_resource.enabled
+            contents = []
+
+            found = false
+            ::File.readlines("/etc/fstab").reverse_each do |line|
+              if !found && line =~ /^#{device_fstab_regex}\s+#{Regexp.escape(@new_resource.mount_point)}\s/
+                found = true
+                if remove
+                  logger.trace("#{@new_resource} is removed from fstab")
+                else
+                  contents << ("#{device_fstab} #{@new_resource.mount_point} #{@new_resource.fstype} #{@new_resource.options.nil? ? default_mount_options : @new_resource.options.join(",")} #{@new_resource.dump} #{@new_resource.pass}")
+                  logger.trace("#{@new_resource} is updated with new content in fstab")
+                end
+                next
+              else
+                contents << line
+              end
+            end
+
+            ::File.open("/etc/fstab", "w") do |fstab|
+              contents.reverse_each { |line| fstab.puts line }
+            end
+          else
+            logger.trace("#{@new_resource} is not enabled - nothing to do")
+          end
+        end
+
       end
     end
   end
diff --git a/spec/unit/provider/mount/mount_spec.rb b/spec/unit/provider/mount/mount_spec.rb
index b84a07e9ce..9a7d9198b5 100644
--- a/spec/unit/provider/mount/mount_spec.rb
+++ b/spec/unit/provider/mount/mount_spec.rb
@@ -247,15 +247,6 @@ describe Chef::Provider::Mount::Mount do
       expect(@provider.current_resource.enabled).to be_falsey
     end
 
-    it "should set enabled to false if the mount point is not last in fstab" do
-      line_1 = "#{@new_resource.device} #{@new_resource.mount_point}  ext3  defaults  1 2\n"
-      line_2 = "/dev/sdy1 #{@new_resource.mount_point}  ext3  defaults  1 2\n"
-      allow(::File).to receive(:foreach).with("/etc/fstab").and_yield(line_1).and_yield(line_2)
-
-      @provider.load_current_resource
-      expect(@provider.current_resource.enabled).to be_falsey
-    end
-
     it "should not mangle the mount options if the device in fstab is a symlink" do
       # expand the target path to correct specs on Windows
       target = "/dev/mapper/target"
@@ -427,10 +418,30 @@ describe Chef::Provider::Mount::Mount do
         @fstab = StringIO.new
         allow(::File).to receive(:readlines).and_return([])
         expect(::File).to receive(:open).once.with("/etc/fstab", "w").and_yield(@fstab)
-        expect(::File).to receive(:open).once.with("/etc/fstab", "a").and_yield(@fstab)
 
         @provider.enable_fs
       end
+
+      it "should update the last matching entry if enabled is true" do
+        @new_resource.fstype("ext4")
+        @new_resource.dump(2)
+        @new_resource.pass(1)
+        allow(@current_resource).to receive(:enabled).and_return(true)
+        fstab_read = ["/dev/sdz1 /tmp/foo  ext3  defaults  1 2\n",
+                      "/dev/sdy1 /tmp/foo  ext3  defaults  1 2\n",
+                      "/dev/sdz1 /tmp/foo  ext3  defaults  1 2\n",
+                      "/dev/sdz1 /tmp/foobar  ext3  defaults  1 2\n"]
+
+        fstab_write = StringIO.new
+        allow(::File).to receive(:readlines).with("/etc/fstab").and_return(fstab_read)
+        allow(::File).to receive(:open).with("/etc/fstab", "w").and_yield(fstab_write)
+
+        @provider.enable_fs
+        expect(fstab_write.string).to eq("/dev/sdz1 /tmp/foo  ext3  defaults  1 2\n" +
+          "/dev/sdy1 /tmp/foo  ext3  defaults  1 2\n" +
+          "/dev/sdz1 /tmp/foo #{@new_resource.fstype} defaults #{@new_resource.dump} #{@new_resource.pass}\n" +
+          "/dev/sdz1 /tmp/foobar  ext3  defaults  1 2\n")
+      end
     end
 
     describe "when disabling the fs" do