(MODULES-7485) Fix validation for required parameters #71
Conversation
jpogran
force-pushed
the
MODULES-7485-fix-validation-for-required-parameters
branch
4 times, most recently
from
58e3c15
to
85885f0
Compare
spec/unit/puppet/type/dsc_spec.rb
Outdated
| @@ -13,15 +20,6 @@ | |||
| resource[:resource_name] = 'foo' | |||
| expect(resource.type).to eq(:Dsc_lite_foo) | |||
| end | |||
|
|
|||
| it "should return Dsc_lite_unspecified given a missing :resource_name" do | |||
There was a problem hiding this comment.
@Iristyle these tests were added from the reporting changes, but as I explain in the comment this situation will never happen because we now require resource_name and fail if it's not specified. If left in, calls to resource.type always return the correct name
There was a problem hiding this comment.
Yup, also noted that we can remove some of the guards in type now - see #69 (comment)
| validate do |value| | ||
| if value.nil? or value.empty? | ||
| raise ArgumentError, "A non-empty #{self.name.to_s} must be specified." | ||
| end | ||
| fail "#{self.name.to_s} should be a Hash" unless value.is_a? ::Hash | ||
| end | ||
| end | ||
|
|
||
| validate do | ||
| raise ArgumentError, 'dsc: resource_name is required' unless self[:resource_name] |
There was a problem hiding this comment.
Do we also want to guard these against people supplying the values as nil or ''?
There was a problem hiding this comment.
Whelp, ignore me, I can't read.
Looks at the validate blocks for the parameters
There was a problem hiding this comment.
Yeah, its a little confusing given we have multiple validations that run at different points in the lifecycle. I think it's still worth seeing (even if it ends up just being for academic reasons) what message we see in the console with :resource_name set to '' or undef - just to verify the UX.
|
I think we probably want to merge the commits together, given the first commit doesn't pass tests. |
lib/puppet/type/dsc.rb
Outdated
| @@ -98,6 +98,8 @@ def change_to_s(currentvalue, newvalue) | |||
| raise ArgumentError, 'dsc: resource_name is required' unless self[:resource_name] | |||
| raise ArgumentError, 'dsc: module is required' unless self[:module] | |||
| raise ArgumentError, 'dsc: properties is required' unless self[:properties] | |||
|
|
|||
| provider.validate if provider.respond_to?(:validate) | |||
There was a problem hiding this comment.
Our provider doesn't have a validate - was this an idiom you saw used elsewhere?
spec/unit/puppet/type/dsc_spec.rb
Outdated
| @@ -20,15 +20,6 @@ | |||
| resource[:resource_name] = 'foo' | |||
There was a problem hiding this comment.
Think we can also remove this line, as it's already being set by default in the call at line 7.
jpogran
force-pushed
the
MODULES-7485-fix-validation-for-required-parameters
branch
from
85885f0
to
a54f67d
Compare
This commit uses global type validation to require the `module`, `resources_name`, and `properties`, instead of `isrequired`. `isrequired` does not work ( see https://projects.puppetlabs.com/issues/4049 / https://tickets.puppetlabs.com/browse/PUP-1591 ) This also removes the tests for situations where `resource_name` is not supplied, as the prior commit adds validation to ensure `resource_name` is supplied. The tests would always fail because `resource_name` is now required.
jpogran
force-pushed
the
MODULES-7485-fix-validation-for-required-parameters
branch
from
a54f67d
to
9945acf
Compare
| raise ArgumentError, 'dsc: module is required' unless self[:module] | ||
| raise ArgumentError, 'dsc: properties is required' unless self[:properties] | ||
|
|
||
| provider.validate if provider.respond_to?(:validate) |
There was a problem hiding this comment.
Only type / provider I found in Puppet that does this is the file type, for the sake of the Windows file provider.
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider/file/windows.rb#L83-L87
In the general sense here I don't think it's necessary as we don't have a provider that has a specific validate (we don't have multiple providers, which is the case for file), and I don't expect we will in the future.
Can remove in a follow-up.
This commit uses global type validation to require the
module,resources_name, andproperties, instead ofisrequired.isrequireddoes not work ( see https://projects.puppetlabs.com/issues/4049 / https://tickets.puppetlabs.com/browse/PUP-1591 )