(PDK-946) Passes ensure values to puppet as symbols. #74
Conversation
There was a problem hiding this comment.
- Make use of
TypeDefinition.ensurable?instead of duplicating the test across the change. - Move
ensurevalidation to the initial validation block in L10-L12 - symbolization happens in multiple places, but should be centralized into
def should, anddef value, see https://github.com/puppetlabs/puppet-resource_api/pull/62/files - Several minor comments
Style:
- Do not end the commit message subject with punctuation
lib/puppet/resource_api.rb
Outdated
| @@ -101,6 +104,12 @@ def type_definition | |||
| raise_missing_params if @missing_params.any? | |||
| end | |||
|
|
|||
| define_method(:validate_ensure) do |options, type| | |||
There was a problem hiding this comment.
Since this method is not accessing register_type-scoped information, it is cleaner to use a straight def here.
lib/puppet/resource_api.rb
Outdated
| @@ -101,6 +104,12 @@ def type_definition | |||
| raise_missing_params if @missing_params.any? | |||
| end | |||
|
|
|||
| define_method(:validate_ensure) do |options, type| | |||
| unless type.is_a?(Puppet::Pops::Types::PEnumType) && type.values == %w[absent present] && options[:type] == 'Enum[present, absent]' | |||
There was a problem hiding this comment.
Comparing the options[:type] string is brittle and redundant. e.g. "Enum[present,absent]" and "Enum[absent, present]" both would be valid ensure definitions, but fail the check.
lib/puppet/resource_api.rb
Outdated
| @@ -101,6 +104,12 @@ def type_definition | |||
| raise_missing_params if @missing_params.any? | |||
| end | |||
|
|
|||
| define_method(:validate_ensure) do |options, type| | |||
| unless type.is_a?(Puppet::Pops::Types::PEnumType) && type.values == %w[absent present] && options[:type] == 'Enum[present, absent]' | |||
| raise Puppet::ResourceError, '`:ensure` attribute must have a type of: `Enum[present, absent]`' | |||
There was a problem hiding this comment.
This is a Puppet::DevError. It needs to be fixed by the developer before the type/provider is valid.
lib/puppet/resource_api.rb
Outdated
| @@ -82,6 +82,9 @@ def type_definition | |||
| @missing_params = [] | |||
| definition[:attributes].each do |name, options| | |||
| type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type]) | |||
|
|
|||
| validate_ensure(options, type) if name == :ensure | |||
There was a problem hiding this comment.
Please push this up the the global validations around #L10-L12
lib/puppet/resource_api.rb
Outdated
| @@ -152,6 +161,9 @@ def type_definition | |||
| # work around https://tickets.puppetlabs.com/browse/PUP-2368 | |||
| rs_value ? :true : :false # rubocop:disable Lint/BooleanSymbol | |||
| else | |||
| if options[:type] == 'Enum[present, absent]' && rs_value | |||
There was a problem hiding this comment.
This special functionality only needs to happen for the ensure property. gating it on the name, instead of the type is much more robust (see comment about matching above)
lib/puppet/resource_api.rb
Outdated
| @@ -269,6 +281,9 @@ def call_provider(value); end | |||
| result[:ensure] = 'absent' if definition[:attributes].key?(:ensure) | |||
| end | |||
|
|
|||
| # puppet needs ensure to be a symbol | |||
| result[:ensure] = result[:ensure].to_sym if definition[:attributes].key?(:ensure) | |||
There was a problem hiding this comment.
The @should value for ensure is symbolized in https://github.com/puppetlabs/puppet-resource_api/pull/74/files#diff-33f447dabde26100aefa52fd359fe9b2R165 , no need to taint the values here.
lib/puppet/resource_api.rb
Outdated
|
|
||
| # fall through to default handling | ||
| end | ||
|
|
||
| # require 'pry'; binding.pry |
lib/puppet/resource_api.rb
Outdated
| when Puppet::Pops::Types::PEnumType, Puppet::Pops::Types::PStringType, Puppet::Pops::Types::PPatternType | ||
| when Puppet::Pops::Types::PEnumType | ||
| if name == :ensure | ||
| value = value.to_sym |
There was a problem hiding this comment.
The @should value for ensure is symbolized in https://github.com/puppetlabs/puppet-resource_api/pull/74/files#diff-33f447dabde26100aefa52fd359fe9b2R165 , no need to taint the values here.
|
Apologies for not pointing at https://github.com/puppetlabs/puppet-resource_api/pull/62/files earlier! |
|
I've added some changes to avoid the mungify changes. Still needs some more cleanup to revert the |
Providers still deal with ensure values as strings, but puppet is always passed symbols.
Providers still deal with ensure values as strings, but puppet is always
passed symbols.