(PDK-895) Basic Array support

Use the new `mungify` function to replace the entire validate/munge cycle on Parameters and Properties. This includes a couple of hacks to not break other parts of puppet. Also includes some fixes were test data for the patterns did not actually match the expectations (either Integers instead of Strings, or only seven digits, instead of eight).
puppetlabs · Apr 9, 2018 · 770310f · 770310f
1 parent 6aaf6b0
commit 770310f
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 51 deletions.
diff --git a/README.md b/README.md
@@ -191,11 +191,12 @@ This gem is still under heavy development. This section is a living document of
 Currently working:
 * Basic type and provider definition, using `name`, `desc`, and `attributes`.
 * Scalar puppet 4 [data types](https://puppet.com/docs/puppet/5.3/lang_data_type.html#core-data-types):
-  * String
+  * String, Enum, Pattern
   * Integer, Float, Numeric
   * Boolean
-  * Enum[absent, present]
-  * simple Optionals
+  * Array
+  * Optional
+  * Variant
 * The `canonicalize`, `simple_get_filter`, and `remote_resource` features.
 * All the logging facilities.
 * Executing the new provider under the following commands:
@@ -205,7 +206,7 @@ Currently working:
   * `puppet device` (if applicable)
 
 There are still a few notable gaps between the implementation and the specification:
-* Complex data types, like Array, Hash, Tuple or Struct are not yet implemented.
+* Complex data types, like Hash, Tuple or Struct are not yet implemented.
 * Only a single runtime environment (the Puppet commands) is currently implemented.
 
 Restrictions of running under puppet:

diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb
@@ -145,76 +145,59 @@ def feature_support?(feature)
           end
 
           type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type])
-          validate do |value|
-            if options[:behaviour] == :read_only
-              raise Puppet::ResourceError, "Attempting to set `#{name}` read_only attribute value to `#{value}`"
+          if param_or_property == :newproperty
+            define_method(:should) do
+              @should ? @should.first : @should
             end
 
-            return true if type.instance?(value)
-
-            if value.is_a? String
-              # when running under `puppet resource`, we need to try to coerce from strings to the real type
-              case value
-              when %r{^-?\d+$}, Puppet::Pops::Patterns::NUMERIC
-                value = Puppet::Pops::Utils.to_n(value)
-              when %r{\Atrue|false\Z}
-                value = value == 'true'
+            define_method(:should=) do |value|
+              @shouldorig = value
+              # Puppet requires the @should value to always be stored as an array. We do not use this
+              # for anything else
+              # @see Puppet::Property.should=(value)
+              @should = [Puppet::ResourceApi.mungify(type, value, "#{definition[:name]}.#{name}")]
+            end
+          else
+            define_method(:value=) do |value|
+              if options[:behaviour] == :read_only
+                raise Puppet::ResourceError, "Attempting to set `#{name}` read_only attribute value to `#{value}`"
               end
-              return true if type.instance?(value)
 
-              inferred_type = Puppet::Pops::Types::TypeCalculator.infer_set(value)
-              error_msg = Puppet::Pops::Types::TypeMismatchDescriber.new.describe_mismatch("#{definition[:name]}.#{name}", type, inferred_type)
-              raise Puppet::ResourceError, error_msg
+              @value = Puppet::ResourceApi.mungify(type, value, "#{definition[:name]}.#{name}")
             end
           end
 
           if type.instance_of? Puppet::Pops::Types::POptionalType
             type = type.type
           end
 
-          # provide better handling of the standard types
+          # puppet symbolizes some values through puppet/paramter/value.rb (see .convert()), but (especially) Enums
+          # are strings. specifying a munge block here skips the value_collection fallback in puppet/parameter.rb's
+          # default .unsafe_munge() implementation.
+          munge { |v| v }
+
+          # provide hints to `puppet type generate` for better parsing
           case type
           when Puppet::Pops::Types::PStringType
             # require any string value
             Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{})
-          # rubocop:disable Lint/BooleanSymbol
           when Puppet::Pops::Types::PBooleanType
             Puppet::ResourceApi.def_newvalues(self, param_or_property, 'true', 'false')
+            # rubocop:disable Lint/BooleanSymbol
             aliasvalue true, 'true'
             aliasvalue false, 'false'
             aliasvalue :true, 'true'
             aliasvalue :false, 'false'
-
-            munge do |v|
-              case v
-              when 'true', :true
-                true
-              when 'false', :false
-                false
-              else
-                v
-              end
-            end
-          # rubocop:enable Lint/BooleanSymbol
+            # rubocop:enable Lint/BooleanSymbol
           when Puppet::Pops::Types::PIntegerType
             Puppet::ResourceApi.def_newvalues(self, param_or_property, %r{^-?\d+$})
-            munge do |v|
-              Puppet::Pops::Utils.to_n(v)
-            end
           when Puppet::Pops::Types::PFloatType, Puppet::Pops::Types::PNumericType
             Puppet::ResourceApi.def_newvalues(self, param_or_property, Puppet::Pops::Patterns::NUMERIC)
-            munge do |v|
-              Puppet::Pops::Utils.to_n(v)
-            end
           end
 
           case options[:type]
           when 'Enum[present, absent]'
             Puppet::ResourceApi.def_newvalues(self, param_or_property, 'absent', 'present')
-            # puppet symbolizes these values through puppet/paramter/value.rb (see .convert()), but Enums are strings
-            # specifying a munge block here skips the value_collection fallback in puppet/parameter.rb's
-            # default .unsafe_munge() implementation
-            munge { |v| v }
           end
         end
       end
@@ -432,7 +415,7 @@ def self.try_mungify(type, value, error_msg_prefix)
       if value =~ %r{^-?\d+$} || value =~ Puppet::Pops::Patterns::NUMERIC
         value = Puppet::Pops::Utils.to_n(value)
       end
-    when Puppet::Pops::Types::PEnumType, Puppet::Pops::Types::PStringType,  Puppet::Pops::Types::PPatternType
+    when Puppet::Pops::Types::PEnumType, Puppet::Pops::Types::PStringType, Puppet::Pops::Types::PPatternType
       if value.is_a? Symbol
         value = value.to_s
       end

diff --git a/spec/acceptance/device_spec.rb b/spec/acceptance/device_spec.rb
@@ -104,7 +104,7 @@
     context 'with a device resource in the catalog' do
       it 'applies the catalog successfully' do
         stdout_str, _status = Open3.capture2e("puppet device #{common_args} --deviceconfig #{device_conf.path} --apply 'device_provider{\"foo\": "\
-                                              'ensure => "present", boolean => true, integer => 15, float => 1.23, variant_pattern => 0xA245EED, '\
+                                              'ensure => "present", boolean => true, integer => 15, float => 1.23, variant_pattern => "0x1234ABCD", '\
                                               'url => "http://www.google.com", boolean_param => false, integer_param => 99, float_param => 3.21, '\
                                               "ensure_param => \"present\", variant_pattern_param => \"9A2222ED\", url_param => \"http://www.puppet.com\"}'")
         expect(stdout_str).not_to match %r{Error:}

diff --git a/spec/integration/resource_api_spec.rb b/spec/integration/resource_api_spec.rb
@@ -42,6 +42,10 @@
             type: 'Pattern[/\A((hkp|http|https):\/\/)?([a-z\d])([a-z\d-]{0,61}\.)+[a-z\d]+(:\d{2,5})?$/]',
             desc: 'a hkp or http(s) url attribute',
           },
+          string_array: {
+            type: 'Array[String]',
+            desc: 'An attribute to exercise Array handling.',
+          },
           optional_string: {
             type: 'Optional[String]',
             desc: 'a optional string attribute',
@@ -82,6 +86,11 @@
             desc: 'a hkp or http(s) url parameter',
             behaviour: :parameter,
           },
+          string_array_param: {
+            type: 'Array[String]',
+            desc: 'A parameter to exercise Array handling.',
+            behaviour: :parameter,
+          },
           optional_string_param: {
             type: 'Optional[String]',
             desc: 'a optional string parameter',
@@ -123,6 +132,11 @@
             desc: 'a hkp or http(s) url readonly',
             behaviour: :read_only,
           },
+          string_array_ro: {
+            type: 'Array[String]',
+            desc: 'A readonly parameter to exercise Array handling.',
+            behaviour: :read_only,
+          },
           optional_string_ro: {
             type: 'Optional[String]',
             desc: 'a optional string readonly',
@@ -159,9 +173,10 @@ def get(_context)
     context 'when setting an attribute' do
       let(:instance) do
         type.new(name: 'somename', ensure: 'present', boolean: true, integer: 15, float: 1.23,
-                 variant_pattern: 0xA245EED, url: 'http://www.google.com', boolean_param: false,
-                 integer_param: 99, float_param: 3.21, ensure_param: 'present',
-                 variant_pattern_param: '9A2222ED', url_param:  'http://www.puppet.com')
+                 variant_pattern: '0x1234ABCD', url: 'http://www.google.com', string_array: %w[a b c],
+                 boolean_param: false, integer_param: 99, float_param: 3.21, ensure_param: 'present',
+                 variant_pattern_param: '1234ABCD', url_param:  'http://www.puppet.com',
+                 string_array_param: %w[d e f])
       end
 
       it('flushes') { expect { instance.flush }.not_to raise_exception }

diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb
@@ -163,7 +163,7 @@ def extract_values(function)
 
       let(:params) do
         { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: 'present',
-          test_enum: 'a', test_variant_pattern: 0xAEF123FF, test_url: 'http://example.com' }
+          test_enum: 'a', test_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' }
       end
 
       it('uses defaults correctly') { expect(instance[:test_string]).to eq 'default value' }
@@ -344,7 +344,7 @@ def extract_values(function)
 
       let(:params) do
         { title: 'test', test_boolean: true, test_integer: 15, test_float: 1.23, test_ensure: 'present',
-          test_variant_pattern: 0xAEF123FF, test_url: 'http://example.com' }
+          test_variant_pattern: '0xAEF123FF', test_url: 'http://example.com' }
       end
 
       it('uses defaults correctly') { expect(instance[:test_string]).to eq 'default value' }