update mount to use properties and fix 6851

Signed-off-by: Thom May <thom@chef.io>

5 files changed, 47 insertions, 139 deletions

diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index 4d0d273ea9..533dd2514b 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -83,7 +83,7 @@ class Chef
       end
 
       def action_enable
-        unless current_resource.enabled && mount_options_unchanged?
+        unless current_resource.enabled && mount_options_unchanged? && device_unchanged?
           converge_by("enable #{current_resource.device}") do
             enable_fs
             Chef::Log.info("#{new_resource} enabled")
@@ -120,6 +120,14 @@ class Chef
         raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #mount_options_unchanged?"
       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.
+      # See #6851 for more.
+      def device_unchanged?
+        @current_resource.device == @new_resource.device
+      end
+
       #
       # NOTE: for the following methods, this superclass will already have checked if the filesystem is
       # enabled and/or mounted and they will be called in converge_by blocks, so most defensive checking
diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb
index c3b1cc0141..1435a75ec3 100644
--- a/lib/chef/provider/mount/mount.rb
+++ b/lib/chef/provider/mount/mount.rb
@@ -57,12 +57,13 @@ class Chef
             case line
             when /^[#\s]/
               next
-            when /^#{device_fstab_regex}\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/
+            when /^(#{device_fstab_regex})\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/
               enabled = true
-              @current_resource.fstype($1)
-              @current_resource.options($2)
-              @current_resource.dump($3.to_i)
-              @current_resource.pass($4.to_i)
+              @current_resource.device($1)
+              @current_resource.fstype($2)
+              @current_resource.options($3)
+              @current_resource.dump($4.to_i)
+              @current_resource.pass($5.to_i)
               Chef::Log.debug("Found mount #{device_fstab} to #{@new_resource.mount_point} in /etc/fstab")
               next
             when /^[\/\w]+\s+#{Regexp.escape(@new_resource.mount_point)}\s+/
@@ -147,7 +148,7 @@ class Chef
         end
 
         def enable_fs
-          if @current_resource.enabled && mount_options_unchanged?
+          if @current_resource.enabled && mount_options_unchanged? && device_unchanged?
             Chef::Log.debug("#{@new_resource} is already enabled - nothing to do")
             return nil
           end
@@ -253,7 +254,7 @@ class Chef
           if @new_resource.device_type == :device
             device_mount_regex
           else
-            device_fstab
+            Regexp.union(device_fstab, device_mount_regex)
           end
         end
 
diff --git a/lib/chef/resource/mount.rb b/lib/chef/resource/mount.rb
index 1a1f8c0565..fca99886b7 100644
--- a/lib/chef/resource/mount.rb
+++ b/lib/chef/resource/mount.rb
@@ -24,10 +24,6 @@ class Chef
     # Use the mount resource to manage a mounted file system.
     class Mount < Chef::Resource
 
-      identity_attr :device
-
-      state_attrs :mount_point, :device_type, :fstype, :username, :password, :domain
-
       default_action :mount
       allowed_actions :mount, :umount, :unmount, :remount, :enable, :disable
 
@@ -37,130 +33,27 @@ class Chef
 
       property :password, String, sensitive: true
 
-      def initialize(name, run_context = nil)
-        super
-        @mount_point = name
-        @device = nil
-        @device_type = :device
-        @fsck_device = "-"
-        @fstype = "auto"
-        @options = ["defaults"]
-        @dump = 0
-        @pass = 2
-        @mounted = false
-        @enabled = false
-        @username = nil
-        @password = nil
-        @domain = nil
-      end
-
-      def mount_point(arg = nil)
-        set_or_return(
-          :mount_point,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def device(arg = nil)
-        set_or_return(
-          :device,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def device_type(arg = nil)
-        real_arg = arg.kind_of?(String) ? arg.to_sym : arg
-        valid_devices = if RUBY_PLATFORM =~ /solaris/i
-                          [ :device ]
-                        else
-                          [ :device, :label, :uuid ]
-                        end
-        set_or_return(
-          :device_type,
-          real_arg,
-          :equal_to => valid_devices
-        )
-      end
-
-      def fsck_device(arg = nil)
-        set_or_return(
-          :fsck_device,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def fstype(arg = nil)
-        set_or_return(
-          :fstype,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def options(arg = nil)
-        ret = set_or_return(
-                            :options,
-                            arg,
-                            :kind_of => [ Array, String ]
-                            )
-
-        if ret.is_a? String
-          ret.tr(",", " ").split(/ /)
-        else
-          ret
-        end
-      end
-
-      def dump(arg = nil)
-        set_or_return(
-          :dump,
-          arg,
-          :kind_of => [ Integer, FalseClass ]
-        )
-      end
+      property :mount_point, String, name_property: true
+      property :device, String, identity: true
 
-      def pass(arg = nil)
-        set_or_return(
-          :pass,
-          arg,
-          :kind_of => [ Integer, FalseClass ]
-        )
-      end
-
-      def mounted(arg = nil)
-        set_or_return(
-          :mounted,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      property :device_type, [String, Symbol],
+        coerce: proc { |arg| arg.kind_of?(String) ? arg.to_sym : arg },
+        default: :device,
+        equal_to: RUBY_PLATFORM =~ /solaris/i ? %i{ device } : %i{ device label uuid }
 
-      def enabled(arg = nil)
-        set_or_return(
-          :enabled,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      property :fsck_device, String, default: "-"
+      property :fstype, String, default: "auto"
 
-      def username(arg = nil)
-        set_or_return(
-          :username,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      property :options, [Array, String],
+        coerce: proc { |arg| arg.kind_of?(String) ? arg.split(",") : arg },
+        default: %w{defaults}
 
-      def domain(arg = nil)
-        set_or_return(
-          :domain,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      property :dump, [Integer, FalseClass], default: 0
+      property :pass, [Integer, FalseClass], default: 2
+      property :mounted, [TrueClass, FalseClass], default: false
+      property :enabled, [TrueClass, FalseClass], default: false
+      property :username, String
+      property :domain, String
 
       private
 
diff --git a/spec/unit/provider/mount/mount_spec.rb b/spec/unit/provider/mount/mount_spec.rb
index b4ce10d861..ca6cf59d71 100644
--- a/spec/unit/provider/mount/mount_spec.rb
+++ b/spec/unit/provider/mount/mount_spec.rb
@@ -475,26 +475,28 @@ describe Chef::Provider::Mount::Mount do
     # we should not create two mount lines, but update the existing one
     context "when the device is described differently" do
       it "should update the existing line" do
+        @current_resource.enabled(true)
         status = double(:stdout => "/dev/sdz1\n", :exitstatus => 1)
         expect(@provider).to receive(:shell_out).with("/sbin/findfs UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a").and_return(status)
 
         filesystems = [%q{/dev/sdy1 /tmp/foo  ext3  defaults  1 2},
                       %q{/dev/sdz1 /tmp/foo  ext3  defaults  1 2}].join("\n")
-        fstab = StringIO.new
-        fstab.puts filesystems
+        fstab = StringIO.new filesystems
+
+        fstab_write = StringIO.new
 
         allow(::File).to receive(:readlines).with("/etc/fstab").and_return(fstab.readlines)
-        allow(::File).to receive(:open).with("/etc/fstab", "w").and_yield(fstab)
-        allow(::File).to receive(:open).with("/etc/fstab", "a").and_yield(fstab)
+        allow(::File).to receive(:open).with("/etc/fstab", "w").and_yield(fstab_write)
+        allow(::File).to receive(:open).with("/etc/fstab", "a").and_yield(fstab_write)
 
         @new_resource.device_type :uuid
         @new_resource.device "d21afe51-a0fe-4dc6-9152-ac733763ae0a"
         @new_resource.dump 1
 
         @provider.enable_fs
-        expect(fstab.string).to match(%r{/dev/sdy1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
-        expect(fstab.string).to match(%r{UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
-        expect(fstab.string).not_to match(%r{/dev/sdz1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+        expect(fstab_write.string).to match(%r{/dev/sdy1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+        expect(fstab_write.string).to match(%r{UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+        expect(fstab_write.string).not_to match(%r{/dev/sdz1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
       end
     end
   end
diff --git a/spec/unit/resource/mount_spec.rb b/spec/unit/resource/mount_spec.rb
index da181b7f8c..1d4cc880d9 100644
--- a/spec/unit/resource/mount_spec.rb
+++ b/spec/unit/resource/mount_spec.rb
@@ -66,6 +66,10 @@ describe Chef::Resource::Mount do
     expect(resource.fstype).to eql("nfs")
   end
 
+  it "sets fstype to 'auto' by default" do
+    expect(resource.fstype).to eql("auto")
+  end
+
   it "allows you to set the dump attribute" do
     resource.dump 1
     expect(resource.dump).to eql(1)