From d8bbc1e323012ec9df9dff1400429c8d2a764b7a Mon Sep 17 00:00:00 2001 From: Michael Hashizume Date: Fri, 19 Apr 2024 11:11:46 -0700 Subject: [PATCH 1/2] Update validate_data_hash to return modified hash Puppet::Pops::Lookup::ModuleDataProvider#validate_data_hash is a method that is supposed to prune a data hash of any keys that are not prefixed with a given module's name. However prior to this commit it incorrectly took the data hash, cloned it, operated on the clone, then returned the unchanged original hash. This commit updates validate_data_hash to instead return the modified hash, and updates the warning message generated when a key is pruned to include the key. --- lib/puppet/pops/lookup/module_data_provider.rb | 18 +++++++++--------- spec/unit/functions/lookup_fixture_spec.rb | 12 ++++++------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/puppet/pops/lookup/module_data_provider.rb b/lib/puppet/pops/lookup/module_data_provider.rb index bca3b3f2241..7f11da7ac8f 100644 --- a/lib/puppet/pops/lookup/module_data_provider.rb +++ b/lib/puppet/pops/lookup/module_data_provider.rb @@ -47,16 +47,16 @@ def key_lookup_in_default(key, lookup_invocation, merge) def validate_data_hash(data_hash) super module_prefix = "#{module_name}::" - data_hash.each_key.reduce(data_hash) do |memo, k| - next memo if k == LOOKUP_OPTIONS || k.start_with?(module_prefix) - - msg = "#{yield} must use keys qualified with the name of the module" - memo = memo.clone if memo.equal?(data_hash) - memo.delete(k) - Puppet.warning("Module '#{module_name}': #{msg}") - memo + data_hash_to_return = {} + data_hash.keys.each do |k| + if k == LOOKUP_OPTIONS || k.start_with?(module_prefix) + data_hash_to_return[k] = data_hash[k] + else + msg = "#{yield} must use keys qualified with the name of the module" + Puppet.warning("Module '#{module_name}': #{msg}; got #{k}") + end end - data_hash + data_hash_to_return end protected diff --git a/spec/unit/functions/lookup_fixture_spec.rb b/spec/unit/functions/lookup_fixture_spec.rb index cff86df030f..8d4a2c25197 100644 --- a/spec/unit/functions/lookup_fixture_spec.rb +++ b/spec/unit/functions/lookup_fixture_spec.rb @@ -377,7 +377,7 @@ def compile_and_get_notifications(code) expect { compiler.compile }.to raise_error(Puppet::ParseError, /did not find a value for the name 'bad_data::b'/) end warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } - expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module") + expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module; got b") end it 'will succeed finding prefixed keys even when a key in the function provided module data is not prefixed' do @@ -391,7 +391,7 @@ def compile_and_get_notifications(code) expect(resources).to include('module_c') end warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } - expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module") + expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module; got b") end it 'will resolve global, environment, and module correctly' do @@ -422,8 +422,8 @@ def compile_and_get_notifications(code) Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do compiler.compile end - warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } - expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module") + warnings = logs.filter_map { |log| log.message if log.level == :warning } + expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module; got b") end it 'a warning will be logged when key in the hiera provided module data is not prefixed' do @@ -432,8 +432,8 @@ def compile_and_get_notifications(code) Puppet::Util::Log.with_destination(Puppet::Test::LogCollector.new(logs)) do compiler.compile end - warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } - expect(warnings).to include("Module 'hieraprovider': Value returned from data_hash function 'json_data', when using location '#{environmentpath}/production/modules/hieraprovider/data/first.json', must use keys qualified with the name of the module") + warnings = logs.filter_map { |log| log.message if log.level == :warning } + expect(warnings).to include("Module 'hieraprovider': Value returned from data_hash function 'json_data', when using location '#{environmentpath}/production/modules/hieraprovider/data/first.json', must use keys qualified with the name of the module; got test::param_b") end end From 7a86ebfdc6e802cc3aa8b453ce662456deb93892 Mon Sep 17 00:00:00 2001 From: Michael Hashizume Date: Fri, 19 Apr 2024 11:29:54 -0700 Subject: [PATCH 2/2] Simplify select.map to filter_map This commit simplifies instances where map is called on a select call to instead use filter_map. --- spec/unit/functions/lookup_fixture_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/unit/functions/lookup_fixture_spec.rb b/spec/unit/functions/lookup_fixture_spec.rb index 8d4a2c25197..44fb72ab1d1 100644 --- a/spec/unit/functions/lookup_fixture_spec.rb +++ b/spec/unit/functions/lookup_fixture_spec.rb @@ -47,7 +47,7 @@ def compile_and_get_notifications(code) Puppet[:code] = code node.environment.check_for_reparse catalog = block_given? ? compiler.compile { |cat| yield(compiler.topscope); cat } : compiler.compile - catalog.resources.map(&:ref).select { |r| r.start_with?('Notify[') }.map { |r| r[7..-2] } + catalog.resources.map(&:ref).filter_map { |r| r[7..-2] if r.start_with?('Notify[') } end # There is a fully configured 'production' environment in fixtures at this location @@ -376,7 +376,7 @@ def compile_and_get_notifications(code) Puppet[:code] = "include bad_data\nlookup('bad_data::b')" expect { compiler.compile }.to raise_error(Puppet::ParseError, /did not find a value for the name 'bad_data::b'/) end - warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } + warnings = logs.filter_map { |log| log.message if log.level == :warning } expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module; got b") end @@ -390,7 +390,7 @@ def compile_and_get_notifications(code) PUPPET expect(resources).to include('module_c') end - warnings = logs.select { |log| log.level == :warning }.map { |log| log.message } + warnings = logs.filter_map { |log| log.message if log.level == :warning } expect(warnings).to include("Module 'bad_data': Value returned from deprecated API function 'bad_data::data' must use keys qualified with the name of the module; got b") end