From 601c2cabf9cd3101c624e86602a52df58d7b5d11 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Fri, 29 Dec 2017 20:37:39 -0800 Subject: Improve property warnings in resources Give more details on why people are seeing these problems so they can fix them. Signed-off-by: Tim Smith --- lib/chef/property.rb | 16 +++++++-------- spec/integration/recipes/resource_action_spec.rb | 2 +- spec/unit/property_spec.rb | 26 ++++++++++++------------ 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/chef/property.rb b/lib/chef/property.rb index a72e41a61e..9d0957dcdf 100644 --- a/lib/chef/property.rb +++ b/lib/chef/property.rb @@ -99,7 +99,7 @@ class Chef if options.has_key?(:name_attribute) # If we have both name_attribute and name_property and they differ, raise an error if options.has_key?(:name_property) - raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{self}." + raise ArgumentError, "name_attribute and name_property are functionally identical and both cannot be specified on a property at once. Use just one on property #{self}" end # replace name_property with name_attribute in place options = Hash[options.map { |k, v| k == :name_attribute ? [ :name_property, v ] : [ k, v ] }] @@ -107,7 +107,7 @@ class Chef end if options.has_key?(:default) && options.has_key?(:name_property) - raise ArgumentError, "Cannot specify both default and name_property/name_attribute together on property #{self}" + raise ArgumentError, "A property cannot be both a name_property/name_attribute and have a default value. Use one or the other on property #{self}" end # Recursively freeze the default if it isn't a lazy value. @@ -361,7 +361,7 @@ class Chef end if value.nil? && required? - raise Chef::Exceptions::ValidationFailed, "#{name} is required" + raise Chef::Exceptions::ValidationFailed, "#{name} is a required property" else value end @@ -386,7 +386,7 @@ class Chef value = set_value(resource, input_to_stored_value(resource, value)) if value.nil? && required? - raise Chef::Exceptions::ValidationFailed, "#{name} is required" + raise Chef::Exceptions::ValidationFailed, "#{name} is a required property" else value end @@ -457,7 +457,7 @@ class Chef # options. # # @param resource [Chef::Resource] The resource we're validating against - # (to provide context for the validate). + # (to provide context for the validation). # @param value The value to validate. # # @raise Chef::Exceptions::ValidationFailed If the value is invalid for @@ -514,18 +514,18 @@ class Chef # very confusing results. if property_redefines_method? resource_name = declared_in.respond_to?(:resource_name) ? declared_in.resource_name : declared_in - raise ArgumentError, "Property `#{name}` of resource `#{resource_name}` overwrites an existing method." + raise ArgumentError, "Property `#{name}` of resource `#{resource_name}` overwrites an existing method. A different name should be used for this property." end # We prefer this form because the property name won't show up in the # stack trace if you use `define_method`. declared_in.class_eval <<-EOM, __FILE__, __LINE__ + 1 def #{name}(value=NOT_PASSED) - raise "Property `#{name}` of `\#{self}` was incorrectly passed a block. Possible property-resource collision. To call a resource named `#{name}` either rename the property or else use `declare_resource(:#{name}, ...)`" if block_given? + raise "Property `#{name}` of `\#{self}` was incorrectly passed a block. Possible property-resource collision. To call a resource named `#{name}` either rename the property or else use `declare_resource(:#{name}, ...)`" if block_given? self.class.properties[#{name.inspect}].call(self, value) end def #{name}=(value) - raise "Property `#{name}` of `\#{self}` was incorrectly passed a block. Possible property-resource collision. To call a resource named `#{name}` either rename the property or else use `declare_resource(:#{name}, ...)`" if block_given? + raise "Property `#{name}` of `\#{self}` was incorrectly passed a block. Possible property-resource collision. To call a resource named `#{name}` either rename the property or else use `declare_resource(:#{name}, ...)`" if block_given? self.class.properties[#{name.inspect}].set(self, value) end EOM diff --git a/spec/integration/recipes/resource_action_spec.rb b/spec/integration/recipes/resource_action_spec.rb index 53f25cb35b..f698a65771 100644 --- a/spec/integration/recipes/resource_action_spec.rb +++ b/spec/integration/recipes/resource_action_spec.rb @@ -509,7 +509,7 @@ module ResourceActionSpec Chef::Config[:treat_deprecation_warnings_as_errors] = false expect_converge do has_property_named_template "hi" - end.to raise_error(/Property `template` of `has_property_named_template\[hi\]` was incorrectly passed a block. Possible property-resource collision. To call a resource named `template` either rename the property or else use `declare_resource\(:template, ...\)`/) + end.to raise_error(/Property `template` of `has_property_named_template\[hi\]` was incorrectly passed a block. Possible property-resource collision. To call a resource named `template` either rename the property or else use `declare_resource\(:template, ...\)`/) end end diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb index b8cf7f5d1b..996585ab98 100644 --- a/spec/unit/property_spec.rb +++ b/spec/unit/property_spec.rb @@ -93,7 +93,7 @@ describe "Chef::Resource.property" do expect(resource.bare_property).to eq 20 end it "can be set with =" do - expect(resource.bare_property 10).to eq 10 + expect(resource.bare_property = 10).to eq 10 expect(resource.bare_property).to eq 10 end it "can be set to nil with =" do @@ -1037,20 +1037,20 @@ describe "Chef::Resource.property" do context "default ordering deprecation warnings" do it "emits an error for property :x, default: 10, #{name}: true" do - expect { resource_class.property :x, :default => 10, name.to_sym => true }.to raise_error Chef::Exceptions::ArgumentError, - /Cannot specify both default and name_property\/name_attribute together on property x of resource chef_resource_property_spec_(\d+)/ + expect { resource_class.property :x, :default => 10, name.to_sym => true }.to raise_error ArgumentError, + /A property cannot be both a name_property\/name_attribute and have a default value. Use one or the other on property x of resource chef_resource_property_spec_(\d+)/ end it "emits an error for property :x, default: nil, #{name}: true" do - expect { resource_class.property :x, :default => nil, name.to_sym => true }.to raise_error Chef::Exceptions::ArgumentError, - /Cannot specify both default and name_property\/name_attribute together on property x of resource chef_resource_property_spec_(\d+)/ + expect { resource_class.property :x, :default => nil, name.to_sym => true }.to raise_error ArgumentError, + /A property cannot be both a name_property\/name_attribute and have a default value. Use one or the other on property x of resource chef_resource_property_spec_(\d+)/ end it "emits an error for property :x, #{name}: true, default: 10" do - expect { resource_class.property :x, name.to_sym => true, :default => 10 }.to raise_error Chef::Exceptions::ArgumentError, - /Cannot specify both default and name_property\/name_attribute together on property x of resource chef_resource_property_spec_(\d+)/ + expect { resource_class.property :x, name.to_sym => true, :default => 10 }.to raise_error ArgumentError, + /A property cannot be both a name_property\/name_attribute and have a default value. Use one or the other on property x of resource chef_resource_property_spec_(\d+)/ end it "emits an error for property :x, #{name}: true, default: nil" do - expect { resource_class.property :x, name.to_sym => true, :default => nil }.to raise_error Chef::Exceptions::ArgumentError, - /Cannot specify both default and name_property\/name_attribute together on property x of resource chef_resource_property_spec_(\d+)/ + expect { resource_class.property :x, name.to_sym => true, :default => nil }.to raise_error ArgumentError, + /A property cannot be both a name_property\/name_attribute and have a default value. Use one or the other on property x of resource chef_resource_property_spec_(\d+)/ end end end @@ -1058,13 +1058,13 @@ describe "Chef::Resource.property" do it "raises an error if both name_property and name_attribute are specified" do expect { resource_class.property :x, :name_property => false, :name_attribute => 1 }.to raise_error ArgumentError, - /Cannot specify both name_property and name_attribute together on property x of resource chef_resource_property_spec_(\d+)./ + /name_attribute and name_property are functionally identical and both cannot be specified on a property at once. Use just one on property x of resource chef_resource_property_spec_(\d+)./ expect { resource_class.property :x, :name_property => false, :name_attribute => nil }.to raise_error ArgumentError, - /Cannot specify both name_property and name_attribute together on property x of resource chef_resource_property_spec_(\d+)./ + /name_attribute and name_property are functionally identical and both cannot be specified on a property at once. Use just one on property x of resource chef_resource_property_spec_(\d+)./ expect { resource_class.property :x, :name_property => false, :name_attribute => false }.to raise_error ArgumentError, - /Cannot specify both name_property and name_attribute together on property x of resource chef_resource_property_spec_(\d+)./ + /name_attribute and name_property are functionally identical and both cannot be specified on a property at once. Use just one on property x of resource chef_resource_property_spec_(\d+)./ expect { resource_class.property :x, :name_property => true, :name_attribute => true }.to raise_error ArgumentError, - /Cannot specify both name_property and name_attribute together on property x of resource chef_resource_property_spec_(\d+)./ + /name_attribute and name_property are functionally identical and both cannot be specified on a property at once. Use just one on property x of resource chef_resource_property_spec_(\d+)./ end context "property_type" do -- cgit v1.2.1