nillable deploy resource + nillable LWRP args

introduced nillable_set_or_return so that we can actually set a nil value
on a resource from the DSL.

this fixes the case where you want to do (and we document) setting
symlink_before_migrate to nil on the deploy resource.

to implement this better also moved up the attribute DSL method to the
base Chef::Resource class.

all LWRP resources are now nillable as well.

5 files changed, 124 insertions, 244 deletions

diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index 78d72dc801..0a84d9b6e9 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -81,34 +81,53 @@ class Chef
         DelayedEvaluator.new(&block)
       end
 
+      NULL_ARG = Object.new
+
+      def nillable_set_or_return(symbol, arg, validation)
+        iv_symbol = "@#{symbol.to_s}".to_sym
+        if arg == NULL_ARG && self.instance_variable_defined?(iv_symbol) == true
+          get_ivar(iv_symbol, symbol, validation)
+        else
+          set_ivar(iv_symbol, symbol, arg, validation)
+        end
+      end
+
       def set_or_return(symbol, arg, validation)
         iv_symbol = "@#{symbol.to_s}".to_sym
         if arg == nil && self.instance_variable_defined?(iv_symbol) == true
-          ivar = self.instance_variable_get(iv_symbol)
-          if(ivar.is_a?(DelayedEvaluator))
-            validate({ symbol => ivar.call }, { symbol => validation })[symbol]
-          else
-            ivar
-          end
+          get_ivar(iv_symbol, symbol, validation)
         else
-          if(arg.is_a?(DelayedEvaluator))
-            val = arg
-          else
-            val = validate({ symbol => arg }, { symbol => validation })[symbol]
-
-            # Handle the case where the "default" was a DelayedEvaluator. In
-            # this case, the block yields an optional parameter of +self+,
-            # which is the equivalent of "new_resource"
-            if val.is_a?(DelayedEvaluator)
-              val = val.call(self)
-            end
-          end
-          self.instance_variable_set(iv_symbol, val)
+          set_ivar(iv_symbol, symbol, arg, validation)
         end
       end
 
       private
 
+      def get_ivar(iv_symbol, symbol, validation)
+        ivar = self.instance_variable_get(iv_symbol)
+        if(ivar.is_a?(DelayedEvaluator))
+          validate({ symbol => ivar.call }, { symbol => validation })[symbol]
+        else
+          ivar
+        end
+      end
+
+      def set_ivar(iv_symbol, symbol, arg, validation)
+        if(arg.is_a?(DelayedEvaluator))
+          val = arg
+        else
+          val = validate({ symbol => arg }, { symbol => validation })[symbol]
+
+          # Handle the case where the "default" was a DelayedEvaluator. In
+          # this case, the block yields an optional parameter of +self+,
+          # which is the equivalent of "new_resource"
+          if val.is_a?(DelayedEvaluator)
+            val = val.call(self)
+          end
+        end
+        self.instance_variable_set(iv_symbol, val)
+      end
+
         # Return the value of a parameter, or nil if it doesn't exist.
         def _pv_opts_lookup(opts, key)
           if opts.has_key?(key.to_s)
@@ -239,4 +258,3 @@ class Chef
     end
   end
 end
-
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index ea220b6c70..2df73a52e8 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -997,6 +997,15 @@ class Chef
     end
 
     #
+    # DSL method used to define attribute on a resource wrapping params_validate
+    #
+    def self.attribute(attr_name, validation_opts={})
+      define_method(attr_name) do |arg=NULL_ARG|
+        nillable_set_or_return(attr_name.to_sym, arg, validation_opts)
+      end
+    end
+
+    #
     # The cookbook in which this Resource was defined (if any).
     #
     # @return Chef::CookbookVersion The cookbook in which this Resource was defined.
diff --git a/lib/chef/resource/deploy.rb b/lib/chef/resource/deploy.rb
index 4252aa230f..f886f856df 100644
--- a/lib/chef/resource/deploy.rb
+++ b/lib/chef/resource/deploy.rb
@@ -82,6 +82,7 @@ class Chef
         @keep_releases = 5
         @enable_checkout = true
         @checkout_branch = "deploy"
+        @timeout = nil
       end
 
       # where the checked out/cloned code goes
@@ -104,42 +105,18 @@ class Chef
       end
 
       # note: deploy_to is your application "meta-root."
-      def deploy_to(arg=nil)
-        set_or_return(
-          :deploy_to,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :deploy_to, :kind_of => [ String ]
 
-      def repo(arg=nil)
-        set_or_return(
-          :repo,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :repo, :kind_of => [ String ]
       alias :repository :repo
 
-      def remote(arg=nil)
-        set_or_return(
-          :remote,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :remote, :kind_of => [ String ]
 
-      def role(arg=nil)
-        set_or_return(
-          :role,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :role, :kind_of => [ String ]
 
-      def restart_command(arg=nil, &block)
-        arg ||= block
-        set_or_return(
+      def restart_command(arg=NULL_ARG, &block)
+        arg = block if block_given?
+        nillable_set_or_return(
           :restart_command,
           arg,
           :kind_of => [ String, Proc ]
@@ -147,155 +124,60 @@ class Chef
       end
       alias :restart :restart_command
 
-      def migrate(arg=nil)
-        set_or_return(
-          :migrate,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      attribute :migrate, :kind_of => [ TrueClass, FalseClass ]
 
-      def migration_command(arg=nil)
-        set_or_return(
-          :migration_command,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :migration_command, kind_of: String
 
-      def rollback_on_error(arg=nil)
-        set_or_return(
-          :rollback_on_error,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      attribute :rollback_on_error, :kind_of => [ TrueClass, FalseClass ]
 
-      def user(arg=nil)
-        set_or_return(
-          :user,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :user, kind_of: String
 
-      def group(arg=nil)
-        set_or_return(
-          :group,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :group, kind_of: [ String ]
 
-      def enable_submodules(arg=nil)
-        set_or_return(
-          :enable_submodules,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      attribute :enable_submodules, kind_of: [ TrueClass, FalseClass ]
 
-      def shallow_clone(arg=nil)
-        set_or_return(
-          :shallow_clone,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      attribute :shallow_clone, kind_of: [ TrueClass, FalseClass ]
 
-      def repository_cache(arg=nil)
-        set_or_return(
-          :repository_cache,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :repository_cache, kind_of: String
 
-      def copy_exclude(arg=nil)
-        set_or_return(
-          :copy_exclude,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :copy_exclude, kind_of: String
 
-      def revision(arg=nil)
-        set_or_return(
-          :revision,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :revision, kind_of: String
       alias :branch :revision
 
-      def git_ssh_wrapper(arg=nil)
-        set_or_return(
-          :git_ssh_wrapper,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :git_ssh_wrapper, kind_of: String
       alias :ssh_wrapper :git_ssh_wrapper
 
-      def svn_username(arg=nil)
-        set_or_return(
-          :svn_username,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :svn_username, kind_of: String
 
-      def svn_password(arg=nil)
-        set_or_return(
-          :svn_password,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :svn_password, kind_of: String
 
-      def svn_arguments(arg=nil)
-        set_or_return(
-          :svn_arguments,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      attribute :svn_arguments, kind_of: String
 
-      def svn_info_args(arg=nil)
-        set_or_return(
-          :svn_arguments,
-          arg,
-          :kind_of => [ String ])
-      end
+      attribute :svn_info_args, kind_of: String
 
-      def scm_provider(arg=nil)
+      def scm_provider(arg=NULL_ARG)
         klass = if arg.kind_of?(String) || arg.kind_of?(Symbol)
                   lookup_provider_constant(arg)
                 else
                   arg
                 end
-        set_or_return(
+        nillable_set_or_return(
           :scm_provider,
           klass,
           :kind_of => [ Class ]
         )
       end
 
-      def svn_force_export(arg=nil)
-        set_or_return(
-          :svn_force_export,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      attribute :svn_force_export, kind_of: [ TrueClass, FalseClass ]
 
-      def environment(arg=nil)
+      def environment(arg=NULL_ARG)
         if arg.is_a?(String)
           Chef::Log.debug "Setting RAILS_ENV, RACK_ENV, and MERB_ENV to `#{arg}'"
           Chef::Log.warn "[DEPRECATED] please modify your deploy recipe or attributes to set the environment using a hash"
           arg = {"RAILS_ENV"=>arg,"MERB_ENV"=>arg,"RACK_ENV"=>arg}
         end
-        set_or_return(
+        nillable_set_or_return(
           :environment,
           arg,
           :kind_of => [ Hash ]
@@ -303,8 +185,8 @@ class Chef
       end
 
        # The number of old release directories to keep around after cleanup
-      def keep_releases(arg=nil)
-        [set_or_return(
+      def keep_releases(arg=NULL_ARG)
+        [nillable_set_or_return(
           :keep_releases,
           arg,
           :kind_of => [ Integer ]), 1].max
@@ -314,13 +196,7 @@ class Chef
       # SCM clone/checkout before symlinking. Use this to get rid of files and
       # directories you want to be shared between releases.
       # Default: ["log", "tmp/pids", "public/system"]
-      def purge_before_symlink(arg=nil)
-        set_or_return(
-          :purge_before_symlink,
-          arg,
-          :kind_of => Array
-        )
-      end
+      attribute :purge_before_symlink, kind_of: Array
 
       # An array of paths, relative to your app's root, where you expect dirs to
       # exist before symlinking. This runs after #purge_before_symlink, so you
@@ -330,13 +206,7 @@ class Chef
       # then specify tmp here so that the tmp directory will exist when you
       # symlink the pids directory in to the current release.
       # Default: ["tmp", "public", "config"]
-      def create_dirs_before_symlink(arg=nil)
-        set_or_return(
-          :create_dirs_before_symlink,
-          arg,
-          :kind_of => Array
-        )
-      end
+      attribute :create_dirs_before_symlink, kind_of: Array
 
       # A Hash of shared/dir/path => release/dir/path. This attribute determines
       # which files and dirs in the shared directory get symlinked to the current
@@ -344,13 +214,7 @@ class Chef
       # $shared/pids that you would like to symlink as $current_release/tmp/pids
       # you specify it as "pids" => "tmp/pids"
       # Default {"system" => "public/system", "pids" => "tmp/pids", "log" => "log"}
-      def symlinks(arg=nil)
-        set_or_return(
-          :symlinks,
-          arg,
-          :kind_of => Hash
-        )
-      end
+      attribute :symlinks, kind_of: Hash
 
       # A Hash of shared/dir/path => release/dir/path. This attribute determines
       # which files in the shared directory get symlinked to the current release
@@ -359,74 +223,44 @@ class Chef
       # For a rails/merb app, this is used to link in a known good database.yml
       # (with the production db password) before running migrate.
       # Default {"config/database.yml" => "config/database.yml"}
-      def symlink_before_migrate(arg=nil)
-        set_or_return(
-          :symlink_before_migrate,
-          arg,
-          :kind_of => Hash
-        )
-      end
+      attribute :symlink_before_migrate, kind_of: Hash
 
       # Callback fires before migration is run.
-      def before_migrate(arg=nil, &block)
-        arg ||= block
-        set_or_return(:before_migrate, arg, :kind_of => [Proc, String])
+      def before_migrate(arg=NULL_ARG, &block)
+        arg = block if block_given?
+        nillable_set_or_return(:before_migrate, arg, kind_of: [Proc, String])
       end
 
       # Callback fires before symlinking
-      def before_symlink(arg=nil, &block)
-        arg ||= block
-        set_or_return(:before_symlink, arg, :kind_of => [Proc, String])
+      def before_symlink(arg=NULL_ARG, &block)
+        arg = block if block_given?
+        nillable_set_or_return(:before_symlink, arg, kind_of: [Proc, String])
       end
 
       # Callback fires before restart
-      def before_restart(arg=nil, &block)
-        arg ||= block
-        set_or_return(:before_restart, arg, :kind_of => [Proc, String])
+      def before_restart(arg=NULL_ARG, &block)
+        arg = block if block_given?
+        nillable_set_or_return(:before_restart, arg, kind_of: [Proc, String])
       end
 
       # Callback fires after restart
-      def after_restart(arg=nil, &block)
-        arg ||= block
-        set_or_return(:after_restart, arg, :kind_of => [Proc, String])
+      def after_restart(arg=NULL_ARG, &block)
+        arg = block if block_given?
+        nillable_set_or_return(:after_restart, arg, kind_of: [Proc, String])
       end
 
-      def additional_remotes(arg=nil)
-        set_or_return(
-          :additional_remotes,
-          arg,
-          :kind_of => Hash
-        )
-      end
+      attribute :additional_remotes, kind_of: Hash
 
-      def enable_checkout(arg=nil)
-        set_or_return(
-          :enable_checkout,
-          arg,
-          :kind_of => [TrueClass, FalseClass]
-        )
-      end
+      attribute :enable_checkout, kind_of: [ TrueClass, FalseClass ]
 
-      def checkout_branch(arg=nil)
-        set_or_return(
-          :checkout_branch,
-          arg,
-          :kind_of => String
-        )
-      end
+      attribute :checkout_branch, kind_of: String
 
       # FIXME The Deploy resource may be passed to an SCM provider as its
       # resource.  The SCM provider knows that SCM resources can specify a
       # timeout for SCM operations. The deploy resource must therefore support
       # a timeout method, but the timeout it describes is for SCM operations,
       # not the overall deployment. This is potentially confusing.
-      def timeout(arg=nil)
-        set_or_return(
-          :timeout,
-          arg,
-          :kind_of => Integer
-        )
-      end
+      attribute :timeout, kind_of: Integer
 
     end
   end
diff --git a/lib/chef/resource/lwrp_base.rb b/lib/chef/resource/lwrp_base.rb
index ce72e98028..a777c511f0 100644
--- a/lib/chef/resource/lwrp_base.rb
+++ b/lib/chef/resource/lwrp_base.rb
@@ -70,14 +70,6 @@ class Chef
         alias_method :resource_name=, :resource_name
       end
 
-      # Define an attribute on this resource, including optional validation
-      # parameters.
-      def self.attribute(attr_name, validation_opts={})
-        define_method(attr_name) do |arg=nil|
-          set_or_return(attr_name.to_sym, arg, validation_opts)
-        end
-      end
-
       # Sets the default action
       def self.default_action(action_name=NULL_ARG)
         unless action_name.equal?(NULL_ARG)
diff --git a/spec/unit/resource/deploy_spec.rb b/spec/unit/resource/deploy_spec.rb
index 0403a7ba6b..07f5f973c0 100644
--- a/spec/unit/resource/deploy_spec.rb
+++ b/spec/unit/resource/deploy_spec.rb
@@ -30,12 +30,35 @@ describe Chef::Resource::Deploy do
 
 
   class << self
+
+    def resource_has_a_hash_attribute(attr_name)
+      it "has a Hash attribute for #{attr_name.to_s}" do
+        @resource.send(attr_name, {foo: "bar"})
+        expect(@resource.send(attr_name)).to eql({foo: "bar"})
+        expect {@resource.send(attr_name, 8675309)}.to raise_error(ArgumentError)
+      end
+
+      it "the Hash attribute for #{attr_name.to_s} is nillable" do
+        @resource.send(attr_name, {foo: "bar"})
+        expect(@resource.send(attr_name)).to eql({foo: "bar"})
+        @resource.send(attr_name, nil)
+        expect(@resource.send(attr_name)).to eql(nil)
+      end
+    end
+
     def resource_has_a_string_attribute(attr_name)
       it "has a String attribute for #{attr_name.to_s}" do
         @resource.send(attr_name, "this is a string")
         expect(@resource.send(attr_name)).to eql("this is a string")
         expect {@resource.send(attr_name, 8675309)}.to raise_error(ArgumentError)
       end
+
+      it "the String attribute for #{attr_name.to_s} is nillable" do
+        @resource.send(attr_name, "this is a string")
+        expect(@resource.send(attr_name)).to eql("this is a string")
+        @resource.send(attr_name, nil)
+        expect(@resource.send(attr_name)).to eql(nil)
+      end
     end
 
     def resource_has_a_boolean_attribute(attr_name, opts={:defaults_to=>false})
@@ -189,6 +212,10 @@ describe Chef::Resource::Deploy do
     expect(@resource.symlink_before_migrate).to eq({"wtf?" => "wtf is going on"})
   end
 
+  resource_has_a_hash_attribute :symlink_before_migrate
+  resource_has_a_hash_attribute :symlinks
+  resource_has_a_hash_attribute :additional_remotes
+
   resource_has_a_callback_attribute :before_migrate
   resource_has_a_callback_attribute :before_symlink
   resource_has_a_callback_attribute :before_restart