stringify facts, if required, before collecting custom facts #173
Conversation
This is the result of some poking in voxpupuli/voxpupuli-test#131 and voxpupuli/voxpupuli-test#132 (comment) Tested in: voxpupuli/puppet-example#54
| @@ -170,11 +170,10 @@ def on_supported_os_implementation(opts = {}) | |||
| os = "#{facts[:operatingsystem].downcase}-#{operatingsystemmajrelease}-#{facts[:hardwaremodel]}" | |||
| next unless os.start_with? RspecPuppetFacts.spec_facts_os_filter if RspecPuppetFacts.spec_facts_os_filter | |||
| facts.merge! RspecPuppetFacts.common_facts | |||
There was a problem hiding this comment.
This made me wonder about common_facts and sadly we'll also need to deal with that. The rabbit hole is always deeper than you'd like :(
There was a problem hiding this comment.
Yes. I would like to takle that in another PR.
There was a problem hiding this comment.
ah wait, that already fixes common_facts as well? self.common_facts always returns symbols but afterwards we call facts = stringify_keys(facts) if RSpec.configuration.facterdb_string_keys so that's fine?
We could call stringify_keys directly in self.common_facts but I think that justs adds one useless iteration on the hash that we can avoid?
There was a problem hiding this comment.
We could call
stringify_keysdirectly inself.common_factsbut I think that justs adds one useless iteration on the hash that we can avoid?
It's about performance overhead IMHO. It may not matter that much, but we know rspec-puppet-facts has proven slow at times. If you make common_facts return correct values you can then implement it here as:
| facts.merge! RspecPuppetFacts.common_facts | |
| facts = stringify_keys(facts) if RSpec.configuration.facterdb_string_keys | |
| facts.merge! RspecPuppetFacts.common_facts |
Then longer term we can also push the changes into facterdb itself. There we do this:
https://github.com/voxpupuli/facterdb/blob/9c33730c537b2891aa1db7e26bd0faf3895e8b1a/lib/facterdb.rb#L146
If we drop the to_sym there we can drop stringify_keys here. voxpupuli/facterdb#362 introduces a symbolize_keys parameter.
Does that clarify the long term path I see for this?
There was a problem hiding this comment.
I'm wondering if we should wait with this until we've a new facterdb version that only returns strings by default. Let's see if we can get voxpupuli/facterdb#364 merged.
|
this is obsolete now, got implemented in #175 |
This is the result of some poking in
voxpupuli/voxpupuli-test#131 and voxpupuli/voxpupuli-test#132 (comment)
Tested in voxpupuli/puppet-example#54