From e90f6382f9a414f1f957957ed51b1763056a86bd Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Mon, 10 Jun 2019 16:32:12 +0100 Subject: [PATCH 1/5] (maint) add a spec:unit rake task This task can be used by developers for a quick check of their changes. This also includes switching the `Rake::Task[:spec_prep].invoke` to a task dependency to avoid double calls. --- Rakefile | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Rakefile b/Rakefile index 69052e84..e30132c6 100644 --- a/Rakefile +++ b/Rakefile @@ -14,7 +14,6 @@ end require 'rspec/core/rake_task' RSpec::Core::RakeTask.new(:spec) do |t| - Rake::Task[:spec_prep].invoke # thanks to the fixtures/modules/ symlinks this needs to exclude fixture modules explicitely excludes = ['fixtures/**/*.rb,fixtures/modules/*/**/*.rb'] if RUBY_PLATFORM == 'java' @@ -24,12 +23,20 @@ RSpec::Core::RakeTask.new(:spec) do |t| t.exclude_pattern = "spec/{#{excludes.join ','}}" end +task :spec => :spec_prep + namespace :spec do desc 'Run RSpec code examples with coverage collection' task :coverage do ENV['SIMPLECOV'] = 'yes' Rake::Task['spec'].execute end + + RSpec::Core::RakeTask.new(:unit) do |t| + t.pattern = "spec/puppet/**/*_spec.rb,spec/integration/**/*_spec.rb" + end + + task :unit => :spec_prep end #### LICENSE_FINDER #### From d75a1342b06d4374af0dffd46692c36e25a22b17 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 20 Jun 2019 16:13:59 +0100 Subject: [PATCH 2/5] (maint) clean up unneeded `define_method` calls These calls add to the complexity of the code without adding any value. Since the introduction of the `type_definition` there are very few reasons for methods to remain defined as a block. This commit changes all of them and a few remaining carte blanche references to `definition`. --- lib/puppet/resource_api.rb | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index f36b3604..7824be94 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -91,7 +91,7 @@ def type_definition apply_to_device end - define_method(:initialize) do |attributes| + def initialize(attributes) # $stderr.puts "A: #{attributes.inspect}" if attributes.is_a? Puppet::Resource @title = attributes.title @@ -119,7 +119,7 @@ def type_definition # the `Puppet::Resource::Ral.find` method, when `instances` does not return a match, uses a Hash with a `:name` key to create # an "absent" resource. This is often hit by `puppet resource`. This needs to work, even if the namevar is not called `name`. # This bit here relies on the default `title_patterns` (see below) to match the title back to the first (and often only) namevar - if definition[:attributes][:name].nil? && attributes[:title].nil? + if type_definition.attributes[:name].nil? && attributes[:title].nil? attributes[:title] = attributes.delete(:name) if attributes[:title].nil? && !type_definition.namevars.empty? attributes[:title] = @title @@ -137,7 +137,7 @@ def to_resource to_resource_shim(super) end - define_method(:to_resource_shim) do |resource| + def to_resource_shim(resource) resource_hash = Hash[resource.keys.map { |k| [k, resource[k]] }] resource_hash[:title] = resource.title ResourceShim.new(resource_hash, type_definition.name, type_definition.namevars, type_definition.attributes, catalog) @@ -244,7 +244,7 @@ def to_resource end end - define_singleton_method(:instances) do + def self.instances # puts 'instances' # force autoloading of the provider provider(type_definition.name) @@ -268,7 +268,7 @@ def to_resource end end - define_method(:refresh_current_state) do + def refresh_current_state @rsapi_current_state = if type_definition.feature?('simple_get_filter') my_provider.get(context, [title]).find { |h| namevar_match?(h) } else @@ -290,7 +290,7 @@ def cache_current_state(resource_hash) strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize') end - define_method(:retrieve) do + def retrieve refresh_current_state unless @rsapi_current_state Puppet.debug("Current State: #{@rsapi_current_state.inspect}") @@ -304,13 +304,13 @@ def cache_current_state(resource_hash) result end - define_method(:namevar_match?) do |item| + def namevar_match?(item) context.type.namevars.all? do |namevar| item[namevar] == @parameters[namevar].value if @parameters[namevar].respond_to? :value end end - define_method(:flush) do + def flush raise_missing_attrs # puts 'flush' @@ -328,7 +328,7 @@ def cache_current_state(resource_hash) # enforce init_only attributes if Puppet.settings[:strict] != :off && @rsapi_current_state && (@rsapi_current_state[:ensure] == 'present' && target_state[:ensure] == 'present') target_state.each do |name, value| - next unless definition[:attributes][name][:behaviour] == :init_only && value != @rsapi_current_state[name] + next unless type_definition.attributes[name][:behaviour] == :init_only && value != @rsapi_current_state[name] message = "Attempting to change `#{name}` init_only attribute value from `#{@rsapi_current_state[name]}` to `#{value}`" case Puppet.settings[:strict] when :warning @@ -350,17 +350,17 @@ def cache_current_state(resource_hash) @rsapi_current_state = target_state end - define_method(:raise_missing_attrs) do + def raise_missing_attrs error_msg = "The following mandatory attributes were not provided:\n * " + @missing_attrs.join(", \n * ") raise Puppet::ResourceError, error_msg if @missing_attrs.any? && (value(:ensure) != :absent && !value(:ensure).nil?) end - define_method(:raise_missing_params) do + def raise_missing_params error_msg = "The following mandatory parameters were not provided:\n * " + @missing_params.join(", \n * ") raise Puppet::ResourceError, error_msg end - define_method(:strict_check) do |current_state| + def strict_check(current_state) return if Puppet.settings[:strict] == :off # if strict checking is on we must notify if the values are changed by canonicalize @@ -374,7 +374,7 @@ def cache_current_state(resource_hash) #:nocov: # codecov fails to register this multiline as covered, even though simplecov does. message = < Date: Fri, 28 Jun 2019 10:36:09 +0100 Subject: [PATCH 3/5] (MODULES-9428) show how simple_get_filter+composite namevars is broken This change adds acceptance tests showing how enabling `simple_get_filter` for a type with composite namevars will break when a user sets a custom title for a resource and uses attributes to pass in the namevar values instead: ``` let(:manifest) { 'composite_namevar { "sometitle": ensure => "present", package => "php", manager => "wibble" }' } it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } ``` This behaviour is misleading at best (as authors might expect `name` to always match all namevars through a title pattern) and completely debilitating at worst, when a custom title is passed to `delete` and there is no indication which resource to delete. --- spec/acceptance/composite_namevar_spec.rb | 111 +++++++++++++----- .../composite_namevar/composite_namevar.rb | 3 +- .../lib/puppet/type/composite_namevar.rb | 5 + 3 files changed, 86 insertions(+), 33 deletions(-) diff --git a/spec/acceptance/composite_namevar_spec.rb b/spec/acceptance/composite_namevar_spec.rb index 6c56aa6a..ed808ac6 100644 --- a/spec/acceptance/composite_namevar_spec.rb +++ b/spec/acceptance/composite_namevar_spec.rb @@ -1,5 +1,6 @@ -require 'spec_helper' require 'open3' +require 'spec_helper' +require 'tempfile' RSpec.describe 'a type with composite namevars' do let(:common_args) { '--verbose --trace --debug --strict=error --modulepath spec/fixtures' } @@ -8,6 +9,7 @@ it 'is returns the values correctly' do stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar") expect(stdout_str.strip).to match %r{^composite_namevar} + expect(stdout_str.strip).to match %r{Looking for \[\]} expect(status).to eq 0 end it 'returns the required resource correctly' do @@ -16,23 +18,27 @@ expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} expect(stdout_str.strip).to match %r{package\s*=> \'php\'} expect(stdout_str.strip).to match %r{manager\s*=> \'yum\'} + expect(stdout_str.strip).to match %r{Looking for \[\]} expect(status.exitstatus).to eq 0 end - it 'Throws error if title is not a matching title_pattern' do - stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php package=php manager=yum") - expect(stdout_str.strip).to match %r{No set of title patterns matched the title "php"} + it 'throws error if title is not a matching title_pattern' do + stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php123 package=php manager=yum") + expect(stdout_str.strip).to match %r{No set of title patterns matched the title "php123"} + expect(stdout_str.strip).not_to match %r{Looking for} expect(status.exitstatus).to eq 1 end it 'returns the match if alternative title_pattern matches' do stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php/gem") expect(stdout_str.strip).to match %r{^composite_namevar \{ \'php/gem\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} + expect(stdout_str.strip).to match %r{Looking for \["php/gem"\]} expect(status.exitstatus).to eq 0 end it 'properly identifies an absent resource if only the title is provided' do stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php-wibble") expect(stdout_str.strip).to match %r{^composite_namevar \{ \'php-wibble\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'} + expect(stdout_str.strip).to match %r{Looking for \["php-wibble"\]} expect(status.exitstatus).to eq 0 end it 'creates a previously absent resource' do @@ -41,6 +47,7 @@ expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} expect(stdout_str.strip).to match %r{package\s*=> \'php\'} expect(stdout_str.strip).to match %r{manager\s*=> \'wibble\'} + expect(stdout_str.strip).to match %r{Looking for \["php-wibble"\]} expect(status.exitstatus).to eq 0 end it 'will remove an existing resource' do @@ -49,16 +56,16 @@ expect(stdout_str.strip).to match %r{package\s*=> \'php\'} expect(stdout_str.strip).to match %r{manager\s*=> \'gem\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'} + expect(stdout_str.strip).to match %r{Looking for \["php-gem"\]} expect(status.exitstatus).to eq 0 end end describe 'using `puppet apply`' do - require 'tempfile' - let(:common_args) { super() + ' --detailed-exitcodes' } - # run Open3.capture2e only once to get both output, and exitcode # rubocop:disable RSpec/InstanceVariable + # run Open3.capture2e only once to get both output, and exitcode + # rubocop:disable RSpec/InstanceVariable before(:each) do Tempfile.create('acceptance') do |f| f.write(manifest) @@ -67,42 +74,82 @@ end end - context 'when managing a present instance' do - let(:manifest) { 'composite_namevar { php-gem: }' } + context 'when matching title patterns' do + context 'when managing a present instance' do + let(:manifest) { 'composite_namevar { php-gem: }' } - it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } - it { expect(@status.exitstatus).to eq 0 } - end + it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } + it { expect(@stdout_str).to match %r{Looking for \["php-gem"\]} } + it { expect(@status.exitstatus).to eq 0 } + end - context 'when managing an absent instance' do - let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'absent\' }' } + context 'when managing an absent instance' do + let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'absent\' }' } - it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]: Nothing to manage: no ensure and the resource doesn't exist} } - it { expect(@status.exitstatus).to eq 0 } - end + it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]: Nothing to manage: no ensure and the resource doesn't exist} } + it { expect(@stdout_str).to match %r{Looking for \["php-wibble"\]} } + it { expect(@status.exitstatus).to eq 0 } + end - context 'when creating a previously absent instance' do - let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'present\' }' } + context 'when creating a previously absent instance' do + let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'present\' }' } - it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]/ensure: defined 'ensure' as 'present'} } - it { expect(@status.exitstatus).to eq 2 } - end + it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]/ensure: defined 'ensure' as 'present'} } + it { expect(@stdout_str).to match %r{Looking for \["php-wibble"\]} } + it { expect(@status.exitstatus).to eq 2 } + end - context 'when removing a previously present instance' do - let(:manifest) { 'composite_namevar { php-yum: ensure=>\'absent\' }' } + context 'when removing a previously present instance' do + let(:manifest) { 'composite_namevar { php-yum: ensure=>\'absent\' }' } - it { expect(@stdout_str).to match %r{Composite_namevar\[php-yum\]/ensure: undefined 'ensure' from 'present'} } - it { expect(@status.exitstatus).to eq 2 } - end + it { expect(@stdout_str).to match %r{Composite_namevar\[php-yum\]/ensure: undefined 'ensure' from 'present'} } + it { expect(@stdout_str).to match %r{Looking for \["php-yum"\]} } + it { expect(@status.exitstatus).to eq 2 } + end - context 'when modifying an existing resource through an alternative title_pattern' do - let(:manifest) { 'composite_namevar { \'php/gem\': value=>\'c\' }' } + context 'when modifying an existing resource through an alternative title_pattern' do + let(:manifest) { 'composite_namevar { \'php/gem\': value=>\'c\' }' } - it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } - it { expect(@stdout_str).to match %r{Target State: \{:package=>"php", :manager=>"gem", :value=>"c", :ensure=>"present"\}} } - it { expect(@status.exitstatus).to eq 2 } + it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } + it { expect(@stdout_str).to match %r{Target State: \{:package=>"php", :manager=>"gem", :value=>"c", :ensure=>"present"\}} } + it { expect(@stdout_str).to match %r{Looking for \["php/gem"\]} } + it { expect(@status.exitstatus).to eq 2 } + end end + context 'when using attributes' do + context 'when managing a present instance' do + let(:manifest) { 'composite_namevar { "sometitle": package => "php", manager => "gem" }' } + + it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } + it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@status.exitstatus).to eq 0 } + end + + context 'when managing an absent instance' do + let(:manifest) { 'composite_namevar { "sometitle": ensure => "absent", package => "php", manager => "wibble" }' } + + it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]: Nothing to manage: no ensure and the resource doesn't exist} } + it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@status.exitstatus).to eq 0 } + end + + context 'when creating a previously absent instance' do + let(:manifest) { 'composite_namevar { "sometitle": ensure => "present", package => "php", manager => "wibble" }' } + + it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]/ensure: defined 'ensure' as 'present'} } + it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@status.exitstatus).to eq 2 } + end + + context 'when removing a previously present instance' do + let(:manifest) { 'composite_namevar { "sometitle": ensure => "absent", package => "php", manager => "yum" }' } + + it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]/ensure: undefined 'ensure' from 'present'} } + it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@status.exitstatus).to eq 2 } + end + end # rubocop:enable RSpec/InstanceVariable end end diff --git a/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb index 61a6a9b6..8b588071 100644 --- a/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb +++ b/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb @@ -14,7 +14,8 @@ def initialize ] end - def get(_context) + def get(context, names = nil) + context.notice("Looking for #{names.inspect}") @current_values end diff --git a/spec/fixtures/test_module/lib/puppet/type/composite_namevar.rb b/spec/fixtures/test_module/lib/puppet/type/composite_namevar.rb index dc84a385..bcd54417 100644 --- a/spec/fixtures/test_module/lib/puppet/type/composite_namevar.rb +++ b/spec/fixtures/test_module/lib/puppet/type/composite_namevar.rb @@ -5,6 +5,7 @@ docs: <<-DOC, This type provides Puppet with the capabilities to manage ... DOC + features: [ 'simple_get_filter' ], title_patterns: [ { pattern: %r{^(?.*[^-])-(?.*)$}, @@ -14,6 +15,10 @@ pattern: %r{^(?.*[^/])/(?.*)$}, desc: 'Where the package and the manager are provided with a forward slash separator', }, + { + pattern: %r{^(?[a-z]*[^/])$}, + desc: 'Fallback pattern where only the package is provided', + }, ], attributes: { ensure: { From 7ab1d9e21c58130d985545afff013f5fe138eea2 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 28 Jun 2019 10:44:38 +0100 Subject: [PATCH 4/5] (MODULES-9428) Pass through full composite namevar values for simple_get_filter To make sure that `simple_get_filter` can work for types with composite namevars, we need to pass through the full set of namevar values, since `resource_hash[type_definition.namevars.first]` is not enough for the provider to return an instance. When a provider implements `simple_get_filter`, and composite namevars, the Resource API does not provide sufficient information to `get` to retrieve the required resource state: ``` Puppet::ResourceApi.register_type( name: 'gpgkey', docs: <<-EOS, This type provides Puppet with the capabilities to manage ... EOS title_patterns: [ { pattern: %r{^(?.*)/(?[^/]*)$}, desc: 'Where the gpgdir and the key_name is provided as the last field of the path', }, { pattern: %r{^(?.*)$}, desc: 'Where only the key_name is provided, if using the default folder', }, ], features: ['simple_get_filter'], attributes: { ensure: { type: 'Enum[present, absent]', desc: 'Whether this resource should be present or absent on the target system.', default: 'present', }, name: { type: 'String', desc: 'The name of the resource you want to manage.', behaviour: :namevar, }, gpgdir: { type: 'String', desc: 'Path to store the GPG key.', default: '/root', behaviour: :namevar, }, }, ) ``` ``` class Puppet::Provider::Gpgkey::Gpgkey < Puppet::ResourceApi::SimpleProvider def get(context, name = []) context.debug('Returning pre-canned example data for: %s' % name.inspect) [ { title: '/root/foo', name: 'foo', gpgdir: '/root', ensure: 'present', }, #{ # title: '/root/bar', # name: 'bar', # gpgdir: '/root', # ensure: 'present', #}, ] end def create(context, name, should) context.notice("Creating '#{name}' with #{should.inspect}") end def update(context, name, should) context.notice("Updating '#{name}' with #{should.inspect}") end def delete(context, name) context.notice("Deleting '#{name}'") end end ``` ``` gpgkey { 'baz1': ensure => present, name => 'foo', gpgdir => '/root'; '/root/bar': ensure => present; '/root/foo2': ensure => present; } ``` ``` Info: Applying configuration version '1558363097' Debug: gpgkey supports `simple_get_filter` Debug: gpgkey: Returning pre-canned example data for: ["foo"] Debug: gpgkey does not support `canonicalize` Debug: Current State: {:title=>"/root/foo", :name=>"foo", :gpgdir=>"/root", :ensure=>"present"} Debug: gpgkey supports `simple_get_filter` Debug: gpgkey: Returning pre-canned example data for: ["bar"] Debug: Current State: {:title=>"bar", :ensure=>:absent} Notice: /Stage[main]/Main/Gpgkey[bar]/ensure: defined 'ensure' as 'present' Debug: gpgkey does not support `canonicalize` Debug: Target State: {:name=>"bar", :ensure=>"present", :gpgdir=>"/root"} Debug: gpgkey does not support `supports_noop` Debug: gpgkey supports `simple_get_filter` Debug: gpgkey[bar]: Creating: Start Notice: gpgkey[bar]: Creating: Creating 'bar' with {:name=>"bar", :ensure=>"present", :gpgdir=>"/root"} Notice: gpgkey[bar]: Creating: Finished in 0.000085 seconds Debug: /Stage[main]/Main/Gpgkey[bar]: The container Class[Main] will propagate my refresh event Debug: gpgkey supports `simple_get_filter` Debug: gpgkey: Returning pre-canned example data for: ["foo2"] Debug: Current State: {:title=>"foo2", :ensure=>:absent} Notice: /Stage[main]/Main/Gpgkey[foo2]/ensure: defined 'ensure' as 'present' Debug: gpgkey does not support `canonicalize` Debug: Target State: {:name=>"foo2", :ensure=>"present", :gpgdir=>"/root"} Debug: gpgkey does not support `supports_noop` Debug: gpgkey supports `simple_get_filter` Debug: gpgkey[foo2]: Creating: Start Notice: gpgkey[foo2]: Creating: Creating 'foo2' with {:name=>"foo2", :ensure=>"present", :gpgdir=>"/root"} Notice: gpgkey[foo2]: Creating: Finished in 0.000069 seconds Debug: /Stage[main]/Main/Gpgkey[foo2]: The container Class[Main] will propagate my refresh event Debug: Class[Main]: The container Stage[main] will propagate my refresh event Debug: Finishing transaction 47323787575300 Debug: Storing state Debug: Pruned old state cache entries in 0.00 seconds Debug: Stored state in 0.01 seconds Notice: Applied catalog in 0.03 seconds ``` As can be seen in the `Returning pre-canned example data for` messages, only `name`, but not `gpgdir` is passed through. This is because of the weakness of title generation in https://github.com/puppetlabs/puppet-resource_api/blob/d249941cf7544005ad66b9bb54e079ffdfc0ac45/lib/puppet/resource_api.rb#L230-L235 After these changes, a hash of namevars and their values is used as a title when requesting resources from the provider: ``` Info: Applying configuration version '1561397939' Debug: gpgkey: Returning pre-canned example data for: [{:name=>"foo", :gpgdir=>"/root"}] Debug: Current State: {:title=>"/root/foo", :name=>"foo", :gpgdir=>"/root", :ensure=>"present"} Debug: gpgkey: Returning pre-canned example data for: [{:name=>"bar", :gpgdir=>"/root"}] Debug: Current State: {:name=>"bar", :gpgdir=>"/root", :ensure=>:absent} Notice: /Stage[main]/Main/Gpgkey[bar]/ensure: defined 'ensure' as 'present' Debug: Target State: {:name=>"bar", :gpgdir=>"/root", :ensure=>"present"} Debug: gpgkey[{:name=>"bar", :gpgdir=>"/root"}]: Creating: Start Notice: gpgkey[{:name=>"bar", :gpgdir=>"/root"}]: Creating: Creating '{:title=>{:name=>"bar", :gpgdir=>"/root"}, :name=>"bar", :gpgdir=>"/root"}' with {:name=>"bar", :gpgdir=>"/root", :ensure=>"present"} Notice: gpgkey[{:name=>"bar", :gpgdir=>"/root"}]: Creating: Finished in 0.000091 seconds Debug: /Stage[main]/Main/Gpgkey[bar]: The container Class[Main] will propagate my refresh event Debug: gpgkey: Returning pre-canned example data for: [{:name=>"foo2", :gpgdir=>"/root"}] Debug: Current State: {:name=>"foo2", :gpgdir=>"/root", :ensure=>:absent} Notice: /Stage[main]/Main/Gpgkey[foo2]/ensure: defined 'ensure' as 'present' Debug: Target State: {:name=>"foo2", :gpgdir=>"/root", :ensure=>"present"} Debug: gpgkey[{:name=>"foo2", :gpgdir=>"/root"}]: Creating: Start Notice: gpgkey[{:name=>"foo2", :gpgdir=>"/root"}]: Creating: Creating '{:title=>{:name=>"foo2", :gpgdir=>"/root"}, :name=>"foo2", :gpgdir=>"/root"}' with {:name=>"foo2", :gpgdir=>"/root", :ensure=>"present"} Notice: gpgkey[{:name=>"foo2", :gpgdir=>"/root"}]: Creating: Finished in 0.000116 seconds Debug: /Stage[main]/Main/Gpgkey[foo2]: The container Class[Main] will propagate my refresh event Debug: Class[Main]: The container Stage[main] will propagate my refresh event Debug: Finishing transaction 47326046031800 Debug: Storing state Debug: Pruned old state cache entries in 0.00 seconds Debug: Stored state in 0.01 seconds Notice: Applied catalog in 0.02 seconds ``` The provider should return a properly formatted `title` value from `get` to enable pretty formatting in logs and `puppet resource` output. # Conflicts: # spec/acceptance/composite_namevar_spec.rb --- Rakefile | 1 + lib/puppet/resource_api.rb | 28 ++++++++-- lib/puppet/resource_api/glue.rb | 15 ++++++ spec/acceptance/composite_namevar_spec.rb | 26 ++++----- spec/acceptance/namevar_spec.rb | 7 +-- .../composite_namevar/composite_namevar.rb | 2 +- .../multiple_namevar/multiple_namevar.rb | 12 ++--- spec/integration/resource_api_spec.rb | 32 +++++++++-- spec/puppet/resource_api/glue_spec.rb | 16 ++++++ spec/puppet/resource_api_spec.rb | 53 +++++++++++++------ 10 files changed, 146 insertions(+), 46 deletions(-) diff --git a/Rakefile b/Rakefile index e30132c6..1134d81a 100644 --- a/Rakefile +++ b/Rakefile @@ -19,6 +19,7 @@ RSpec::Core::RakeTask.new(:spec) do |t| if RUBY_PLATFORM == 'java' excludes += ['acceptance/**/*.rb', 'integration/**/*.rb', 'puppet/resource_api/*_context_spec.rb', 'puppet/util/network_device/simple/device_spec.rb'] t.rspec_opts = '--tag ~agent_test' + t.rspec_opts << ' --tag ~j17_exclude' if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.0.0') end t.exclude_pattern = "spec/{#{excludes.join ','}}" end diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 7824be94..4c5e7ed2 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -133,6 +133,20 @@ def name title end + def self.build_title(type_definition, resource_hash) + if type_definition.namevars.size > 1 + # use a MonkeyHash to allow searching in Puppet's RAL + Puppet::ResourceApi::MonkeyHash[type_definition.namevars.map { |attr| [attr, resource_hash[attr]] }] + else + resource_hash[type_definition.namevars[0]] + end + end + + def rsapi_title + @rsapi_title ||= self.class.build_title(type_definition, self) + @rsapi_title + end + def to_resource to_resource_shim(super) end @@ -261,7 +275,7 @@ def self.instances result = if resource_hash.key? :title new(title: resource_hash[:title]) else - new(title: resource_hash[type_definition.namevars.first]) + new(title: build_title(type_definition, resource_hash)) end result.cache_current_state(resource_hash) result @@ -270,7 +284,7 @@ def self.instances def refresh_current_state @rsapi_current_state = if type_definition.feature?('simple_get_filter') - my_provider.get(context, [title]).find { |h| namevar_match?(h) } + my_provider.get(context, [rsapi_title]).find { |h| namevar_match?(h) } else my_provider.get(context).find { |h| namevar_match?(h) } end @@ -279,7 +293,11 @@ def refresh_current_state type_definition.check_schema(@rsapi_current_state) strict_check(@rsapi_current_state) if type_definition.feature?('canonicalize') else - @rsapi_current_state = { title: title } + @rsapi_current_state = if rsapi_title.is_a? Hash + rsapi_title.dup + else + { title: rsapi_title } + end @rsapi_current_state[:ensure] = :absent if type_definition.ensurable? end end @@ -340,9 +358,9 @@ def flush end if type_definition.feature?('supports_noop') - my_provider.set(context, { title => { is: @rsapi_current_state, should: target_state } }, noop: noop?) + my_provider.set(context, { rsapi_title => { is: @rsapi_current_state, should: target_state } }, noop: noop?) else - my_provider.set(context, title => { is: @rsapi_current_state, should: target_state }) unless noop? + my_provider.set(context, rsapi_title => { is: @rsapi_current_state, should: target_state }) unless noop? end raise 'Execution encountered an error' if context.failed? diff --git a/lib/puppet/resource_api/glue.rb b/lib/puppet/resource_api/glue.rb index 30773a08..3e8a2911 100644 --- a/lib/puppet/resource_api/glue.rb +++ b/lib/puppet/resource_api/glue.rb @@ -57,4 +57,19 @@ def filtered_keys values.keys.reject { |k| k == :title || !attr_def[k] || (attr_def[k][:behaviour] == :namevar && @namevars.size == 1) } end end + + # this hash allows key-value based ordering comparisons between instances of this and instances of this and other classes + # this is required for `lib/puppet/indirector/resource/ral.rb`'s `search` method which expects all titles to be comparable + class MonkeyHash < Hash + def <=>(other) + result = self.class.name <=> other.class.name + if result.zero? + result = keys.sort <=> other.keys.sort + end + if result.zero? + result = keys.sort.map { |k| self[k] } <=> other.keys.sort.map { |k| other[k] } + end + result + end + end end diff --git a/spec/acceptance/composite_namevar_spec.rb b/spec/acceptance/composite_namevar_spec.rb index ed808ac6..30a31110 100644 --- a/spec/acceptance/composite_namevar_spec.rb +++ b/spec/acceptance/composite_namevar_spec.rb @@ -31,14 +31,14 @@ stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php/gem") expect(stdout_str.strip).to match %r{^composite_namevar \{ \'php/gem\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} - expect(stdout_str.strip).to match %r{Looking for \["php/gem"\]} + expect(stdout_str.strip).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]} expect(status.exitstatus).to eq 0 end it 'properly identifies an absent resource if only the title is provided' do stdout_str, status = Open3.capture2e("puppet resource #{common_args} composite_namevar php-wibble") expect(stdout_str.strip).to match %r{^composite_namevar \{ \'php-wibble\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'} - expect(stdout_str.strip).to match %r{Looking for \["php-wibble"\]} + expect(stdout_str.strip).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} expect(status.exitstatus).to eq 0 end it 'creates a previously absent resource' do @@ -47,7 +47,7 @@ expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} expect(stdout_str.strip).to match %r{package\s*=> \'php\'} expect(stdout_str.strip).to match %r{manager\s*=> \'wibble\'} - expect(stdout_str.strip).to match %r{Looking for \["php-wibble"\]} + expect(stdout_str.strip).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} expect(status.exitstatus).to eq 0 end it 'will remove an existing resource' do @@ -56,7 +56,7 @@ expect(stdout_str.strip).to match %r{package\s*=> \'php\'} expect(stdout_str.strip).to match %r{manager\s*=> \'gem\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'absent\'} - expect(stdout_str.strip).to match %r{Looking for \["php-gem"\]} + expect(stdout_str.strip).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]} expect(status.exitstatus).to eq 0 end end @@ -79,7 +79,7 @@ let(:manifest) { 'composite_namevar { php-gem: }' } it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } - it { expect(@stdout_str).to match %r{Looking for \["php-gem"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]} } it { expect(@status.exitstatus).to eq 0 } end @@ -87,7 +87,7 @@ let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'absent\' }' } it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]: Nothing to manage: no ensure and the resource doesn't exist} } - it { expect(@stdout_str).to match %r{Looking for \["php-wibble"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} } it { expect(@status.exitstatus).to eq 0 } end @@ -95,7 +95,7 @@ let(:manifest) { 'composite_namevar { php-wibble: ensure=>\'present\' }' } it { expect(@stdout_str).to match %r{Composite_namevar\[php-wibble\]/ensure: defined 'ensure' as 'present'} } - it { expect(@stdout_str).to match %r{Looking for \["php-wibble"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} } it { expect(@status.exitstatus).to eq 2 } end @@ -103,7 +103,7 @@ let(:manifest) { 'composite_namevar { php-yum: ensure=>\'absent\' }' } it { expect(@stdout_str).to match %r{Composite_namevar\[php-yum\]/ensure: undefined 'ensure' from 'present'} } - it { expect(@stdout_str).to match %r{Looking for \["php-yum"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"yum"\}\]} } it { expect(@status.exitstatus).to eq 2 } end @@ -112,7 +112,7 @@ it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } it { expect(@stdout_str).to match %r{Target State: \{:package=>"php", :manager=>"gem", :value=>"c", :ensure=>"present"\}} } - it { expect(@stdout_str).to match %r{Looking for \["php/gem"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]} } it { expect(@status.exitstatus).to eq 2 } end end @@ -122,7 +122,7 @@ let(:manifest) { 'composite_namevar { "sometitle": package => "php", manager => "gem" }' } it { expect(@stdout_str).to match %r{Current State: \{:title=>"php-gem", :package=>"php", :manager=>"gem", :ensure=>"present", :value=>"b"\}} } - it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"gem"\}\]} } it { expect(@status.exitstatus).to eq 0 } end @@ -130,7 +130,7 @@ let(:manifest) { 'composite_namevar { "sometitle": ensure => "absent", package => "php", manager => "wibble" }' } it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]: Nothing to manage: no ensure and the resource doesn't exist} } - it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} } it { expect(@status.exitstatus).to eq 0 } end @@ -138,7 +138,7 @@ let(:manifest) { 'composite_namevar { "sometitle": ensure => "present", package => "php", manager => "wibble" }' } it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]/ensure: defined 'ensure' as 'present'} } - it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"wibble"\}\]} } it { expect(@status.exitstatus).to eq 2 } end @@ -146,7 +146,7 @@ let(:manifest) { 'composite_namevar { "sometitle": ensure => "absent", package => "php", manager => "yum" }' } it { expect(@stdout_str).to match %r{Composite_namevar\[sometitle\]/ensure: undefined 'ensure' from 'present'} } - it { expect(@stdout_str).to match %r{Looking for \["sometitle"\]} } + it { expect(@stdout_str).to match %r{Looking for \[\{:package=>"php", :manager=>"yum"\}\]} } it { expect(@status.exitstatus).to eq 2 } end end diff --git a/spec/acceptance/namevar_spec.rb b/spec/acceptance/namevar_spec.rb index 2a434548..a162151d 100644 --- a/spec/acceptance/namevar_spec.rb +++ b/spec/acceptance/namevar_spec.rb @@ -24,11 +24,12 @@ expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} expect(status).to eq 0 end - it 'returns the match if title matches a namevar value' do - stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php") - expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php\'} + it 'returns the match if title matches a title value' do + stdout_str, status = Open3.capture2e("puppet resource #{common_args} multiple_namevar php-gem") + expect(stdout_str.strip).to match %r{^multiple_namevar \{ \'php-gem\'} expect(stdout_str.strip).to match %r{ensure\s*=> \'present\'} expect(stdout_str.strip).to match %r{package\s*=> \'php\'} + expect(stdout_str.strip).to match %r{manager\s*=> \'gem\'} expect(status).to eq 0 end it 'creates a previously absent resource if all namevars are provided' do diff --git a/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb index 8b588071..9b450f2a 100644 --- a/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb +++ b/spec/fixtures/test_module/lib/puppet/provider/composite_namevar/composite_namevar.rb @@ -1,7 +1,7 @@ require 'puppet/resource_api' require 'puppet/resource_api/simple_provider' -# Implementation for the title_provider type using the Resource API. +# Implementation for the composite_namevar type using the Resource API. class Puppet::Provider::CompositeNamevar::CompositeNamevar < Puppet::ResourceApi::SimpleProvider def initialize @current_values ||= [ diff --git a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb index 857e73b9..c1eea1c0 100644 --- a/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb +++ b/spec/fixtures/test_module/lib/puppet/provider/multiple_namevar/multiple_namevar.rb @@ -4,12 +4,12 @@ class Puppet::Provider::MultipleNamevar::MultipleNamevar def initialize @current_values ||= [ - { package: 'php', manager: 'yum', ensure: 'present' }, - { package: 'php', manager: 'gem', ensure: 'present' }, - { package: 'mysql', manager: 'yum', ensure: 'present' }, - { package: 'mysql', manager: 'gem', ensure: 'present' }, - { package: 'foo', manager: 'bar', ensure: 'present' }, - { package: 'bar', manager: 'foo', ensure: 'present' }, + { title: 'php-yum', package: 'php', manager: 'yum', ensure: 'present' }, + { title: 'php-gem', package: 'php', manager: 'gem', ensure: 'present' }, + { title: 'mysql-yum', package: 'mysql', manager: 'yum', ensure: 'present' }, + { title: 'mysql-gem', package: 'mysql', manager: 'gem', ensure: 'present' }, + { title: 'foo-bar', package: 'foo', manager: 'bar', ensure: 'present' }, + { title: 'bar-foo', package: 'bar', manager: 'foo', ensure: 'present' }, ] end diff --git a/spec/integration/resource_api_spec.rb b/spec/integration/resource_api_spec.rb index b8d1cdf8..543f15ea 100644 --- a/spec/integration/resource_api_spec.rb +++ b/spec/integration/resource_api_spec.rb @@ -7,12 +7,27 @@ let(:definition) do { name: 'integration', + title_patterns: [ + { + pattern: %r{^(?.*[^-])-(?.*)$}, + desc: 'Where the name and the name2 are provided with a hyphen separator', + }, + { + pattern: %r{^(?.*)$}, + desc: 'Where only the name is provided', + }, + ], attributes: { name: { type: 'String', behaviour: :namevar, desc: 'the title', }, + name2: { + type: 'String', + behaviour: :namevar, + desc: 'the other title', + }, string: { type: 'String', desc: 'a string attribute', @@ -170,7 +185,18 @@ s = setter Class.new do def get(_context) - [] + [ + { + name: 'foo', + name2: 'bar', + title: 'foo-bar', + }, + { + name: 'foo2', + name2: 'bar2', + title: 'foo2-bar2', + }, + ] end attr_reader :last_changes @@ -198,7 +224,7 @@ def get(_context) # See spec/acceptance/metaparam_spec.rb for more in-depth testing with a full puppet apply let(:catalog) { instance_double('Puppet::Resource::Catalog', 'catalog') } let(:instance) do - type.new(name: 'somename', ensure: 'present', boolean: true, integer: 15, float: 1.23, + type.new(name: 'somename', name2: 'othername', ensure: 'present', boolean: true, integer: 15, float: 1.23, variant_pattern: '0x1234ABCD', url: 'http://www.google.com', sensitive: Puppet::Pops::Types::PSensitiveType::Sensitive.new('a password'), string_array: %w[a b c], variant_array: 'not_an_array', array_of_arrays: [%w[a b c], %w[d e f]], @@ -225,7 +251,7 @@ def get(_context) end it 'can serialize to json' do - expect({ 'resources' => [instance.to_resource] }.to_json).to eq '{"resources":[{"somename":{"ensure":"absent","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":"d","e":"f","string_param":"default value"}}]}' # rubocop:disable Metrics/LineLength + expect({ 'resources' => [instance.to_resource] }.to_json).to eq '{"resources":[{"somename":{"name":"somename","name2":"othername","ensure":"absent","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":"d","e":"f","string_param":"default value"}}]}' # rubocop:disable Metrics/LineLength end end diff --git a/spec/puppet/resource_api/glue_spec.rb b/spec/puppet/resource_api/glue_spec.rb index b8b656ea..80ae642f 100644 --- a/spec/puppet/resource_api/glue_spec.rb +++ b/spec/puppet/resource_api/glue_spec.rb @@ -78,4 +78,20 @@ it { expect(instance.to_hash).to eq(namevarname: 'title', attr: 'value', attr_ro: 'fixed') } end end + + describe Puppet::ResourceApi::MonkeyHash do + it { expect(described_class.ancestors).to include Hash } + + describe '#<=>(other)' do + subject(:value) { described_class[b: 'b', c: 'c'] } + + it { expect(value <=> 'string').to eq(-1) } + # Avoid this test on jruby 1.7, where it is hitting a implementation inconsistency and `'string' <=> value` returns `nil` + it('compares to a string', j17_exclude: true) { expect('string' <=> value).to eq 1 } + it { expect(value <=> described_class[b: 'b', c: 'c']).to eq 0 } + it { expect(value <=> described_class[d: 'd']).to eq(-1) } + it { expect(value <=> described_class[a: 'a']).to eq 1 } + it { expect(value <=> described_class[b: 'a', c: 'c']).to eq 1 } + end + end end diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 59a32121..e87f1d40 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -1133,7 +1133,9 @@ def set(_context, changes) end } end - it { expect { described_class.register_type(definition) }.not_to raise_error } + before(:each) do + described_class.register_type(definition) + end describe 'the registered type' do subject(:type) { Puppet::Type.type(:composite) } @@ -1142,7 +1144,7 @@ def set(_context, changes) end it { expect(type.parameters).to eq [:package, :manager] } end - describe 'an instance of the type' do + describe 'the type\'s class' do let(:provider_class) do Class.new do def get(_context) @@ -1152,33 +1154,54 @@ def get(_context) def set(_context, _changes); end end end - let(:instance) { Puppet::Type.type(:composite) } + let(:type_class) { Puppet::Type.type(:composite) } before(:each) do stub_const('Puppet::Provider::Composite', Module.new) stub_const('Puppet::Provider::Composite::Composite', provider_class) end - context 'when title_patterns called' do + describe '.title_patterns' do it 'returns correctly generated pattern' do # [[ %r{^(?.*[^/])/(?.*)$},[[:package],[:manager]]],[%r{^(?.*)$},[[:package]]]] - expect(instance.title_patterns.first[0]).to be_a Regexp - expect(instance.title_patterns.first[0]).to eq(%r{^(?.*[^/])/(?.*)$}) - expect(instance.title_patterns.first[1].size).to eq 2 - expect(instance.title_patterns.first[1][0][0]).to eq :package - expect(instance.title_patterns.first[1][1][0]).to eq :manager + expect(type_class.title_patterns.first[0]).to be_a Regexp + expect(type_class.title_patterns.first[0]).to eq(%r{^(?.*[^/])/(?.*)$}) + expect(type_class.title_patterns.first[1].size).to eq 2 + expect(type_class.title_patterns.first[1][0][0]).to eq :package + expect(type_class.title_patterns.first[1][1][0]).to eq :manager - expect(instance.title_patterns.last[0]).to be_a Regexp - expect(instance.title_patterns.last[0]).to eq(%r{^(?.*)$}) - expect(instance.title_patterns.last[1].size).to eq 1 - expect(instance.title_patterns.last[1][0][0]).to eq :package + expect(type_class.title_patterns.last[0]).to be_a Regexp + expect(type_class.title_patterns.last[0]).to eq(%r{^(?.*)$}) + expect(type_class.title_patterns.last[1].size).to eq 1 + expect(type_class.title_patterns.last[1][0][0]).to eq :package end end - context 'when instances called' do + describe '.instances' do it 'uses the title provided by the provider' do - expect(instance.instances[0].title).to eq('php/yum') + expect(type_class.instances[0].title).to eq('php/yum') + end + end + + context 'when flushing an instance' do + let(:provider_instance) { instance_double(provider_class, 'provider_instance') } + + before(:each) do + allow(provider_class).to receive(:new).and_return(provider_instance) + end + + after(:each) do + # reset cached provider between tests + type_class.instance_variable_set(:@my_provider, nil) + end + + it 'uses a hash as `name` when setting values' do + allow(provider_instance).to receive(:get).and_return([{ title: 'php/yum', package: 'php', manager: 'yum', ensure: 'present' }]) + expect(provider_instance).to receive(:set) { |_context, changes| + expect(changes.keys).to eq [{ package: 'php', manager: 'yum' }] + } + type_class.new(title: 'php/yum', ensure: :absent).flush end end end From 781083f157d3f5d6bddd38675e19b2602ef76a37 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 19 Jun 2019 12:06:07 +0100 Subject: [PATCH 5/5] (MODULES-9428) Improve handling of composite namevars in SimpleProvider Before this change, edgecases where `is` and `should` were not provided would lead to failures due to `name:` being filled wrong. --- lib/puppet/resource_api/simple_provider.rb | 18 ++++++++++-- .../resource_api/simple_provider_spec.rb | 29 +++++++------------ 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/puppet/resource_api/simple_provider.rb b/lib/puppet/resource_api/simple_provider.rb index a1e94ee4..d515bc87 100644 --- a/lib/puppet/resource_api/simple_provider.rb +++ b/lib/puppet/resource_api/simple_provider.rb @@ -1,4 +1,5 @@ module Puppet; end # rubocop:disable Style/Documentation + module Puppet::ResourceApi # This class provides a default implementation for set(), when your resource does not benefit from batching. # Instead of processing changes yourself, the `create`, `update`, and `delete` functions, are called for you, @@ -19,12 +20,12 @@ def set(context, changes) raise 'SimpleProvider cannot be used with a Type that is not ensurable' unless context.type.ensurable? - is = { name: name, ensure: 'absent' } if is.nil? - should = { name: name, ensure: 'absent' } if should.nil? + is = SimpleProvider.create_absent(:name, name) if is.nil? + should = SimpleProvider.create_absent(:name, name) if should.nil? name_hash = if context.type.namevars.length > 1 # pass a name_hash containing the values of all namevars - name_hash = { title: name } + name_hash = {} context.type.namevars.each do |namevar| name_hash[namevar] = change[:should][namevar] end @@ -60,5 +61,16 @@ def update(_context, _name, _should) def delete(_context, _name) raise "#{self.class} has not implemented `delete`" end + + # @api private + def self.create_absent(namevar, title) + result = if title.is_a? Hash + title.dup + else + { namevar => title } + end + result[:ensure] = 'absent' + result + end end end diff --git a/spec/puppet/resource_api/simple_provider_spec.rb b/spec/puppet/resource_api/simple_provider_spec.rb index e0ea67f0..caf7290b 100644 --- a/spec/puppet/resource_api/simple_provider_spec.rb +++ b/spec/puppet/resource_api/simple_provider_spec.rb @@ -214,33 +214,26 @@ def delete(context, _name); end it { expect { provider.set(context, changes) }.to raise_error %r{SimpleProvider cannot be used with a Type that is not ensurable} } end - context 'with a type with multiple namevars' do - let(:should_values) { { name: 'title', parent: 'foo', wibble: 'wub', ensure: 'present' } } - let(:title) { 'foo#wub' } + context 'with changes from a composite namevar type' do let(:changes) do - { title => - { - should: should_values, - } } - end - let(:name_hash) do { - title: title, - parent: 'foo', - wibble: 'wub', + { name1: 'value1', name2: 'value2' } => + { + should: { name1: 'value1', name2: 'value2', ensure: 'present' }, + }, } end before(:each) do - allow(context).to receive(:creating).with(title).and_yield - allow(context).to receive(:type).and_return(type_def) - allow(type_def).to receive(:feature?).with('simple_get_filter') + allow(context).to receive(:creating).with(name1: 'value1', name2: 'value2').and_yield + allow(type_def).to receive(:feature?).with('simple_get_filter').and_return(true) + allow(type_def).to receive(:namevars).and_return([:name1, :name2]) allow(type_def).to receive(:check_schema) - allow(type_def).to receive(:namevars).and_return([:parent, :wibble]) end - it 'calls create once' do - expect(provider).to receive(:create).with(context, name_hash, should_values).once + it 'calls the crud methods with the right title' do + expect(provider).to receive(:create).with(context, { name1: 'value1', name2: 'value2' }, hash_including(name1: 'value1')) + provider.set(context, changes) end end