Rework macos_userdefaults resource

This is a reboot of sorts on the macos_userdefaults resource to fix some fundamental flaws in the UX of the resource and how it gets state / sets the values.

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

1 files changed, 73 insertions, 42 deletions

diff --git a/lib/chef/resource/macos_userdefaults.rb b/lib/chef/resource/macos_userdefaults.rb
index bc3bab67fd..3bfd280105 100644
--- a/lib/chef/resource/macos_userdefaults.rb
+++ b/lib/chef/resource/macos_userdefaults.rb
@@ -28,40 +28,74 @@ class Chef
 
       description "Use the **macos_userdefaults** resource to manage the macOS user defaults system. The properties of this resource are passed to the defaults command, and the parameters follow the convention of that command. See the defaults(1) man page for details on how the tool works."
       introduced "14.0"
+      examples <<~DOC
+        **Specify a global domain value**
+
+        ```ruby
+        macos_userdefaults 'full keyboard access to all controls' do
+          key 'AppleKeyboardUIMode'
+          value '2'
+        end
+        ```
+
+        **Use an integer value**
+
+        ```ruby
+        macos_userdefaults 'enable macOS firewall' do
+          domain '/Library/Preferences/com.apple.alf'
+          key 'globalstate'
+          value '1'
+          type 'int'
+        end
+        ```
+
+        **Use a boolean value**
+
+        ```ruby
+        macos_userdefaults 'finder expanded save dialogs' do
+          key 'NSNavPanelExpandedStateForSaveMode'
+          value 'TRUE'
+          type 'bool'
+        end
+        ```
+      DOC
 
       property :domain, String,
         description: "The domain that the user defaults belong to.",
-        required: true
+        default: "NSGlobalDomain",
+        default_description: "NSGlobalDomain: the global domain.",
+        desired_state: false
 
       property :global, [TrueClass, FalseClass],
         description: "Determines whether or not the domain is global.",
-        default: false
+        deprecated: true,
+        default: false,
+        desired_state: false
 
       property :key, String,
-        description: "The preference key."
+        description: "The preference key.",
+        required: true,
+        desired_state: false
 
       property :value, [Integer, Float, String, TrueClass, FalseClass, Hash, Array],
-        description: "The value of the key.",
+        description: "The value of the key. Note: When setting boolean values you can either specify 0/1 or you can pass true/false, 'true'/false', or 'yes'/'no' and we'll automattically convert these to the proper boolean values Apple expects.",
+        coerce: proc { |v| coerce_booleans(v) },
         required: true
 
       property :type, String,
         description: "The value type of the preference key.",
-        default: ""
+        default: "",
+        desired_state: false
 
       property :user, String,
-        description: "The system user that the default will be applied to."
+        description: "The system user that the default will be applied to.",
+        desired_state: false
 
       property :sudo, [TrueClass, FalseClass],
         description: "Set to true if the setting you wish to modify requires privileged access.",
         default: false,
         desired_state: false
 
-      # @todo this should get refactored away: https://github.com/chef/chef/issues/7622
-      property :is_set, [TrueClass, FalseClass],
-        default: false,
-        desired_state: false,
-        skip_docs: true
-
       # coerce various ways of representing a boolean into either 0 (false) or 1 (true)
       # which is what the defaults CLI expects. Why? Well defaults itself accepts a few
       # different formats, but when you do a read command it all comes back as 1 or 0.
@@ -73,34 +107,38 @@ class Chef
       end
 
       load_current_value do |desired|
-        value = coerce_booleans(desired.value)
-        cmd = "defaults read '#{desired.domain}' "
-        cmd << "'#{desired.key}' " if desired.key
-        cmd << " | grep -qx '#{value}'"
-
-        vc = if desired.user.nil?
-               shell_out(cmd)
-             else
-               shell_out(cmd, user: desired.user)
-             end
-
-        is_set !vc.error?
+        coerced_value = coerce_booleans(desired.value)
+
+        state_cmd = ['/usr/bin/defaults', 'read', desired.domain, desired.key]
+
+        state = if desired.user.nil?
+          shell_out(state_cmd)
+        else
+          shell_out(cmd, user: desired.user)
+        end
+
+        current_value_does_not_exist! if state.error?
+
+        value state.stdout.strip
       end
 
       action :write do
-        description "Write the setting to the specified domain"
+        description "Write the value to the specified domain/key."
 
-        unless current_resource.is_set
-          cmd = ["defaults write"]
-          cmd.unshift("sudo") if new_resource.sudo
+        converge_if_changed do
+          # FIXME: this should use cmd directly as an array argument, but then the quoting
+          # of individual args above needs to be removed as well.
+          execute defaults_write_cmd.join(" ") do
+            user new_resource.user unless new_resource.user.nil?
+          end
+        end
+      end
 
-          cmd << if new_resource.global
-                   "NSGlobalDomain"
-                 else
-                   "'#{new_resource.domain}'"
-                 end
+      action_class do
+        def defaults_write_cmd
+          cmd = ["defaults write '#{new_resource.domain}' '#{new_resource.key}' "]
+          cmd.unshift("sudo") if new_resource.sudo
 
-          cmd << "'#{new_resource.key}'" if new_resource.key
           value = new_resource.value
           type = new_resource.type.empty? ? value_type(value) : new_resource.type
           # creates a string of Key1 Value1 Key2 Value2...
@@ -111,16 +149,9 @@ class Chef
           end
           cmd << "-#{type}" if type
           cmd << value
-
-          # FIXME: this should use cmd directly as an array argument, but then the quoting
-          # of individual args above needs to be removed as well.
-          execute cmd.join(" ") do
-            user new_resource.user unless new_resource.user.nil?
-          end
+          cmd
         end
-      end
 
-      action_class do
         def value_type(value)
           case value
           when true, false