Updated unit spec according to remount_require?ashwini/fix_idempotent_remount

Signed-off-by: NAshwini <ashwini.nehate@msystechnologies.com>

4 files changed, 53 insertions, 3 deletions

diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index b099c267e6..17c4b9e8cf 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -118,6 +118,11 @@ class Chef
         raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #mount_options_unchanged?"
       end
 
+      # should actually check if the filesystem is mounted and new resource have any difference (not just return current_resource) and return true/false
+      def remount_require?
+        raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #remount_require?"
+      end
+
       # It's entirely plausible that a site might prefer UUIDs or labels, so
       # we need to be able to update fstab to conform with their wishes
       # without necessarily needing to remount the device.
diff --git a/lib/chef/provider/mount/linux.rb b/lib/chef/provider/mount/linux.rb
index 57e0f8c324..0213f6a1e0 100644
--- a/lib/chef/provider/mount/linux.rb
+++ b/lib/chef/provider/mount/linux.rb
@@ -80,6 +80,8 @@ class Chef
           end
           @current_resource.mounted(mounted)
         end
+
+        # should actually check if the filesystem is mounted and new resource have any difference (not just return current_resource) and return true/false
         def remount_require?
           remount_require = true
           real_mount_point = shell_out("findmnt --kernel #{@new_resource.device}| tail -1 | awk '{print$4}'").stdout.split(",")
diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb
index 2021d6ece5..aacec71a89 100644
--- a/lib/chef/provider/mount/mount.rb
+++ b/lib/chef/provider/mount/mount.rb
@@ -255,6 +255,11 @@ class Chef
             @current_resource.pass == @new_resource.pass
         end
 
+        # should actually check if the filesystem is mounted and new resource have any difference (not just return current_resource) and return true/false
+        def remount_require?
+          raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #remount_require?"
+        end
+
         # It will update or delete the entry from fstab.
         def edit_fstab(remove: false)
           if @current_resource.enabled
diff --git a/spec/unit/provider/mount_spec.rb b/spec/unit/provider/mount_spec.rb
index 7800d0c666..115fe04e2d 100644
--- a/spec/unit/provider/mount_spec.rb
+++ b/spec/unit/provider/mount_spec.rb
@@ -33,6 +33,7 @@ describe Chef::Provider::Mount do
     new_resource.name        "/tmp/foo"
     new_resource.mount_point "/tmp/foo"
     new_resource.fstype      "ext3"
+    new_resource.options %w{nodev nosuid}
     new_resource
   end
 
@@ -90,9 +91,10 @@ describe Chef::Provider::Mount do
       new_resource.supports[:remount] = true
     end
 
-    it "should remount the filesystem if it is mounted" do
+    it "should remount the filesystem if it is mounted and remount_require? is true" do
       allow(current_resource).to receive(:mounted).and_return(true)
-      expect(provider).to receive(:remount_fs).and_return(true)
+      allow(provider).to receive(:remount_require?).and_return(true)
+      expect(provider).to receive(:remount_fs)
       provider.run_action(:remount)
       expect(new_resource).to be_updated_by_last_action
     end
@@ -111,7 +113,10 @@ describe Chef::Provider::Mount do
       allow(current_resource).to receive(:mounted).and_return(true)
     end
 
-    it "should try a umount/remount of the filesystem" do
+    it "should try a umount and mount of the filesystem if it is mounted and remount_require? is true" do
+      allow(current_resource).to receive(:mounted).and_return(true)
+      allow(provider).to receive(:remount_require?).and_return(true)
+      expect(provider).not_to receive(:remount_fs)
       expect(provider).to receive(:umount_fs)
       expect(provider).to receive(:mounted?).and_return(true, false)
       expect(provider).to receive(:mount_fs)
@@ -123,10 +128,43 @@ describe Chef::Provider::Mount do
       provider.unmount_retries = 1
       expect(provider).to receive(:umount_fs)
       expect(provider).to receive(:mounted?).and_return(true, true)
+      expect(provider).to receive(:remount_require?).and_return(true)
       expect { provider.run_action(:remount) }.to raise_error(Chef::Exceptions::Mount)
     end
   end
 
+  describe "when the filesystem should not be remounted" do
+    it "should remount the filesystem if it is mounted is false and remount_require? is false" do
+      allow(current_resource).to receive(:mounted).and_return(false)
+      allow(provider).to receive(:remount_require?).and_return(false)
+      expect(provider).not_to receive(:remount_fs)
+      expect(provider).not_to receive(:umount_fs)
+      expect(provider).not_to receive(:mount_fs)
+      provider.run_action(:remount)
+      expect(new_resource).not_to be_updated_by_last_action
+    end
+
+    it "should remount the filesystem if it is mounted is false and remount_require? is true" do
+      allow(current_resource).to receive(:mounted).and_return(false)
+      allow(provider).to receive(:remount_require?).and_return(true)
+      expect(provider).not_to receive(:remount_fs)
+      expect(provider).not_to receive(:umount_fs)
+      expect(provider).not_to receive(:mount_fs)
+      provider.run_action(:remount)
+      expect(new_resource).not_to be_updated_by_last_action
+    end
+
+    it "should remount the filesystem if it is mounted is true and remount_require? is false" do
+      allow(current_resource).to receive(:mounted).and_return(true)
+      allow(provider).to receive(:remount_require?).and_return(false)
+      expect(provider).not_to receive(:remount_fs)
+      expect(provider).not_to receive(:umount_fs)
+      expect(provider).not_to receive(:mount_fs)
+      provider.run_action(:remount)
+      expect(new_resource).not_to be_updated_by_last_action
+    end
+  end
+
   describe "when enabling the filesystem to be mounted" do
     it "should enable the mount if it isn't enable" do
       allow(current_resource).to receive(:enabled).and_return(false)