code review update

- add some comments on implementing subclass
- use accessors not instance variables
- poll instead of just sleeping

1 files changed, 44 insertions, 27 deletions

diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index bd62f07762..0eb1de0de8 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -36,92 +36,109 @@ class Chef
       end
 
       def action_mount
-        unless @current_resource.mounted
-          converge_by("mount #{@current_resource.device} to #{@current_resource.mount_point}") do
+        unless current_resource.mounted
+          converge_by("mount #{current_resource.device} to #{current_resource.mount_point}") do
             mount_fs
-            Chef::Log.info("#{@new_resource} mounted")
+            Chef::Log.info("#{new_resource} mounted")
           end
         else
-          Chef::Log.debug("#{@new_resource} is already mounted")
+          Chef::Log.debug("#{new_resource} is already mounted")
         end
       end
 
       def action_umount
-        if @current_resource.mounted
-          converge_by("unmount #{@current_resource.device}") do
+        if current_resource.mounted
+          converge_by("unmount #{current_resource.device}") do
             umount_fs
-            Chef::Log.info("#{@new_resource} unmounted")
+            Chef::Log.info("#{new_resource} unmounted")
           end
         else
-          Chef::Log.debug("#{@new_resource} is already unmounted")
+          Chef::Log.debug("#{new_resource} is already unmounted")
         end
       end
 
       def action_remount
-        if @current_resource.mounted
-          if @new_resource.supports[:remount]
-            converge_by("remount #{@current_resource.device}") do
+        if current_resource.mounted
+          if new_resource.supports[:remount]
+            converge_by("remount #{current_resource.device}") do
               remount_fs
-              Chef::Log.info("#{@new_resource} remounted")
+              Chef::Log.info("#{new_resource} remounted")
             end
           else
-            converge_by("unmount #{@current_resource.device}") do
+            converge_by("unmount #{current_resource.device}") do
               umount_fs
-              Chef::Log.info("#{@new_resource} unmounted")
+              Chef::Log.info("#{new_resource} unmounted")
             end
-            sleep 3
-            converge_by("mount #{@current_resource.device}") do
+            sleep 1 while mounted?
+            converge_by("mount #{current_resource.device}") do
               mount_fs
-              Chef::Log.info("#{@new_resource} mounted")
+              Chef::Log.info("#{new_resource} mounted")
             end
           end
         else
-          Chef::Log.debug("#{@new_resource} not mounted, nothing to remount")
+          Chef::Log.debug("#{new_resource} not mounted, nothing to remount")
         end
       end
 
       def action_enable
-        unless @current_resource.enabled && mount_options_unchanged?
-          converge_by("enable #{@current_resource.device}") do
+        unless current_resource.enabled && mount_options_unchanged?
+          converge_by("enable #{current_resource.device}") do
             enable_fs
-            Chef::Log.info("#{@new_resource} enabled")
+            Chef::Log.info("#{new_resource} enabled")
           end
         else
-          Chef::Log.debug("#{@new_resource} already enabled")
+          Chef::Log.debug("#{new_resource} already enabled")
         end
       end
 
       def action_disable
-        if @current_resource.enabled
-          converge_by("disable #{@current_resource.device}") do
+        if current_resource.enabled
+          converge_by("disable #{current_resource.device}") do
             disable_fs
-            Chef::Log.info("#{@new_resource} disabled")
+            Chef::Log.info("#{new_resource} disabled")
           end
         else
-          Chef::Log.debug("#{@new_resource} already disabled")
+          Chef::Log.debug("#{new_resource} already disabled")
         end
       end
 
+      # should actually check if the filesystem is mounted (not just return current_resource) and return true/false
+      def mounted?
+        raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not implement #mounted?"
+      end
+
+      # should check new_resource against current_resource to see if mount options have changed, returns true/false
       def mount_options_unchanged?
-        raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not implement #mount_options_unchnaged?"
+        raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not implement #mount_options_unchanged?"
       end
 
+      #
+      # NOTE: for the following functions, they will already have checked if the filesystem is enabled and/or mounted and
+      # will be called in converge_by blocks, so most defensive checking does not need to be done, just do the thing.
+      #
+
+      # should implement mounting of the filesystem, raises if action does not succeed
       def mount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :mount"
       end
 
+      # should implement unmounting of the filesystem, raises if action does not succeed
       def umount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :umount"
       end
 
+      # should implement remounting of the filesystem (via a -o remount or some other atomic-ish action that isn't
+      # simply a umount/mount style remount), raises if action does not succeed
       def remount_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :remount"
       end
 
+      # should implement enabling of the filesystem (e.g. in /etc/fstab), raises if action does not succeed
       def enable_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :enable"
       end
 
+      # should implement disabling of the filesystem (e.g. in /etc/fstab), raises if action does not succeed
       def disable_fs
         raise Chef::Exceptions::UnsupportedAction, "#{self.to_s} does not support :disable"
       end