-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update validate_data_hash to return modified hash #9326
Update validate_data_hash to return modified hash #9326
Conversation
e1b4c33
to
b1207b9
Compare
spec/unit/pops/lookup/lookup_spec.rb
Outdated
@@ -28,6 +28,7 @@ module Lookup | |||
a: a (from global) | |||
d: d (from global) | |||
mod::e: mod::e (from global) | |||
other::y: should be removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to add a test that this data gets filtered out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although I've been digging through existing tests and I'm unsure about how to approach this. It would be easy to look at what validate_data_hash
returns and make sure that it does not include a given key, but I assume we want to search for and validate that the data has been removed within Puppet. Any pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a key to a module's hiera.yaml
, where the key doesn't match the module's name. Looking at
puppet/spec/unit/pops/lookup/lookup_spec.rb
Lines 57 to 58 in fa213f5
'common.yaml' => <<-YAML.unindent | |
mod::c: mod::c (from module) |
You could add an entry like the following:
'common.yaml' => <<-YAML.unindent
....
other::c: other::c (from module)
YAML
And then add tests around here
puppet/spec/unit/pops/lookup/lookup_spec.rb
Lines 192 to 194 in fa213f5
it 'finds data in module layer' do | |
expect(Lookup.lookup('mod::c', nil, 'not found', true, nil, invocation)).to eql('mod::c (from module)') | |
end |
to test that if you do Lookup.lookup('other::c', ...)
then it should return nil. And if you lookup an unqualified Lookup.lookup('c', ...)
then it should return the mod::c (from module)
, since the value from other::c
should be ignored.
Prior to your changes, I would expect other::c (from) module
to be returned in both cases, since it wasn't filtered out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like unqualified lookups work in tests, both in my PR or in main
. I'll dig into it a bit more and see if that's expected, it's a bug, or if there's something wrong with the tests.
1509eed
to
d6e5e73
Compare
d6e5e73
to
73aa3f8
Compare
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.
This commit simplifies instances where map is called on a select call to instead use filter_map.
73aa3f8
to
7a86ebf
Compare
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.