Merge pull request #40 from da-ar/optional_attrs

(PDK-819) Ensure checks for mandatory type attributes
puppetlabs · Mar 23, 2018 · d3d8ef5 · d3d8ef5
2 parents 8382909 + 18c616e
commit d3d8ef5
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 56 deletions.
diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb
@@ -61,6 +61,21 @@ def feature_support?(feature)
         super(attributes)
       end
 
+      validate do
+        # enforce mandatory attributes
+        missing_attrs = []
+        definition[:attributes].each do |name, options|
+          type = Puppet::Pops::Types::TypeParser.singleton.parse(options[:type])
+          unless [:read_only, :namevar].include? options[:behaviour]
+            missing_attrs << name if value(name).nil? && !(type.instance_of? Puppet::Pops::Types::POptionalType)
+          end
+        end
+        if missing_attrs.any?
+          error_msg = "The following mandatory attributes where not provided:\n    *  " + missing_attrs.join(", \n    *  ")
+          raise Puppet::ResourceError, error_msg
+        end
+      end
+
       definition[:attributes].each do |name, options|
         # puts "#{name}: #{options.inspect}"
 
@@ -117,6 +132,10 @@ def feature_support?(feature)
               end
             end
 
+            if type.instance_of? Puppet::Pops::Types::POptionalType
+              type = type.type
+            end
+
             # provide better handling of the standard types
             case type
             when Puppet::Pops::Types::PStringType

diff --git a/spec/acceptance/device_spec.rb b/spec/acceptance/device_spec.rb
@@ -3,6 +3,11 @@
 
 RSpec.describe 'exercising a device provider' do
   let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
+  let(:default_type_values) do
+    'string="meep" boolean=true integer=15 float=1.23 ensure=present variant_pattern=AE321EEF '\
+                                'url="http://www.puppet.com" boolean_param=false integer_param=99 float_param=3.21 '\
+                                'ensure_param=present variant_pattern_param=0xAE321EEF url_param="https://www.google.com"'
+  end
 
   before(:each) { skip 'No device --apply in the puppet gems yet' if ENV['PUPPET_GEM_VERSION'] }
 
@@ -14,7 +19,7 @@
       expect(status).to eq 0
     end
     it 'manages resources on the target system' do
-      stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider foo ensure=present")
+      stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider foo ensure=present #{default_type_values}")
       expect(stdout_str).to match %r{Notice: /Device_provider\[foo\]/ensure: defined 'ensure' as 'present'}
       expect(status).to eq 0
     end
@@ -23,7 +28,7 @@
       let(:common_args) { '--verbose --trace --strict=error --modulepath spec/fixtures' }
 
       it 'deals with canonicalized resources correctly' do
-        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present #{default_type_values}")
         stdmatch = 'Error: /Device_provider\[wibble\]: Could not evaluate: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
                    'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample"\}\n'\
                    'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed"\}'
@@ -36,7 +41,7 @@
       let(:common_args) { '--verbose --trace --strict=warning --modulepath spec/fixtures' }
 
       it 'deals with canonicalized resources correctly' do
-        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present  #{default_type_values}")
         stdmatch = 'Warning: device_provider\[wibble\]#get has not provided canonicalized values.\n'\
                    'Returned values:       \{:name=>"wibble", :ensure=>:present, :string=>"sample"\}\n'\
                    'Canonicalized values:  \{:name=>"wibble", :ensure=>:present, :string=>"changed"\}'
@@ -49,7 +54,7 @@
       let(:common_args) { '--verbose --trace --strict=off --modulepath spec/fixtures' }
 
       it 'deals with canonicalized resources correctly' do
-        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present")
+        stdout_str, status = Open3.capture2e("puppet resource #{common_args} device_provider wibble ensure=present  #{default_type_values}")
         stdmatch = 'Notice: /Device_provider\[wibble\]/string: string changed \'sample\' to \'changed\''
         expect(stdout_str).to match %r{#{stdmatch}}
         expect(status.success?).to be_truthy # rubocop:disable RSpec/PredicateMatcher
@@ -98,7 +103,10 @@
 
     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\": }'")
+        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, '\
+                                              '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:}
       end
     end

diff --git a/spec/fixtures/test_module/lib/puppet/type/device_provider.rb b/spec/fixtures/test_module/lib/puppet/type/device_provider.rb
@@ -35,10 +35,6 @@
       type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]',
       desc: 'a pattern parameter',
     },
-    path: {
-      type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-      desc: 'a path or URL parameter',
-    },
     url: {
       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 parameter',
@@ -78,11 +74,6 @@
       desc: 'a pattern parameter',
       behaviour: :parameter,
     },
-    path_param: {
-      type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-      desc: 'a path or URL parameter',
-      behaviour: :parameter,
-    },
     url_param: {
       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 parameter',
@@ -97,47 +88,42 @@
       type: 'String',
       desc: 'a string readonly',
       default: 'default value',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     boolean_ro: {
       type: 'Boolean',
       desc: 'a boolean readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     integer_ro: {
       type: 'Integer',
       desc: 'an integer readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     float_ro: {
       type: 'Float',
       desc: 'a floating point readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     ensure_ro: {
       type: 'Enum[present, absent]',
       desc: 'a ensure readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     variant_pattern_ro: {
       type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]',
       desc: 'a pattern readonly',
-      behaviour: :readonly,
-    },
-    path_ro: {
-      type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-      desc: 'a path or URL readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     url_ro: {
       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 readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
     optional_string_ro: {
       type: 'Optional[String]',
       desc: 'a optional string readonly',
-      behaviour: :readonly,
+      behaviour: :read_only,
     },
   },
 )
diff --git a/spec/fixtures/test_module/lib/puppet/type/test_autorequire.rb b/spec/fixtures/test_module/lib/puppet/type/test_autorequire.rb
@@ -18,7 +18,7 @@
       behaviour: :namevar,
     },
     target:      {
-      type:      'String',
+      type:      'Optional[String]',
       desc:      'The resource to autorequire.',
       behaviour: :parameter,
     },

diff --git a/spec/fixtures/test_module/lib/puppet/type/test_simple_get_filter.rb b/spec/fixtures/test_module/lib/puppet/type/test_simple_get_filter.rb
@@ -18,7 +18,7 @@
       behaviour: :namevar,
     },
     test_string: {
-        type:      'String',
+        type:      'Optional[String]',
         desc:      'Used for testing our expected outcomes',
     },
   },

diff --git a/spec/integration/resource_api_spec.rb b/spec/integration/resource_api_spec.rb
@@ -38,10 +38,6 @@
             type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]',
             desc: 'a pattern parameter',
           },
-          path: {
-            type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-            desc: 'a path or URL parameter',
-          },
           url: {
             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 parameter',
@@ -81,11 +77,6 @@
             desc: 'a pattern parameter',
             behaviour: :parameter,
           },
-          path_param: {
-            type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-            desc: 'a path or URL parameter',
-            behaviour: :parameter,
-          },
           url_param: {
             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 parameter',
@@ -100,47 +91,42 @@
             type: 'String',
             desc: 'a string readonly',
             default: 'default value',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           boolean_ro: {
             type: 'Boolean',
             desc: 'a boolean readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           integer_ro: {
             type: 'Integer',
             desc: 'an integer readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           float_ro: {
             type: 'Float',
             desc: 'a floating point readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           ensure_ro: {
             type: 'Enum[present, absent]',
             desc: 'a ensure readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           variant_pattern_ro: {
             type: 'Variant[Pattern[/\A(0x)?[0-9a-fA-F]{8}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{16}\Z/], Pattern[/\A(0x)?[0-9a-fA-F]{40}\Z/]]',
             desc: 'a pattern readonly',
-            behaviour: :readonly,
-          },
-          path_ro: {
-            type: 'Variant[Stdlib::Absolutepath, Pattern[/\A(https?|ftp):\/\//]]',
-            desc: 'a path or URL readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           url_ro: {
             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 readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
           optional_string_ro: {
             type: 'Optional[String]',
             desc: 'a optional string readonly',
-            behaviour: :readonly,
+            behaviour: :read_only,
           },
         },
       }
@@ -171,7 +157,12 @@ def get(_context)
     end
 
     context 'when setting an attribute' do
-      let(:instance) { type.new(name: 'somename', ensure: 'present') }
+      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')
+      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
@@ -157,7 +157,10 @@ def extract_values(function)
     describe 'an instance of this type' do
       subject(:instance) { Puppet::Type.type(:with_string).new(params) }
 
-      let(:params) { { title: 'test' } }
+      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' }
+      end
 
       it('uses defaults correctly') { expect(instance[:test_string]).to eq 'default value' }
 
@@ -183,6 +186,12 @@ def extract_values(function)
         it('the test_url value is set correctly') { expect(instance[:test_url]).to eq('hkp://example.com') }
       end
 
+      context 'when mandatory attributes are missing' do
+        let(:params) { { title: 'test' } }
+
+        it { expect { instance }.to raise_exception Puppet::ResourceError, %r{The following mandatory attributes where not provided} }
+      end
+
       describe 'different boolean values' do
         let(:params) do
           {
@@ -191,6 +200,9 @@ def extract_values(function)
             test_boolean: the_boolean,
             test_integer: '-1',
             test_float: '-1.5',
+            test_ensure: :present,
+            test_variant_pattern: 'a' * 8,
+            test_url: 'http://example.com',
           }
         end
 
@@ -300,7 +312,10 @@ def extract_values(function)
     describe 'an instance of this type' do
       subject(:instance) { Puppet::Type.type(:with_parameters).new(params) }
 
-      let(:params) { { title: 'test' } }
+      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' }
+      end
 
       it('uses defaults correctly') { expect(instance[:test_string]).to eq 'default value' }
 
@@ -777,7 +792,7 @@ def set(_context, changes)
         end
 
         describe 'an existing instance' do
-          let(:instance) { type.new(name: 'somename') }
+          let(:instance) { type.new(name: 'somename', test_string: 'foo') }
 
           it('its name is set correctly') { expect(resource[:name]).to eq 'somename' }
           it('its properties are set correctly') do
@@ -793,7 +808,7 @@ def set(_context, changes)
         end
 
         describe 'an absent instance' do
-          let(:instance) { type.new(name: 'does_not_exist') }
+          let(:instance) { type.new(name: 'does_not_exist', test_string: 'foo') }
 
           it('its name is set correctly') { expect(resource[:name]).to eq 'does_not_exist' }
           it('its properties are set correctly') do