Use facterdb_string_keys configuration option for custom facts

[jordanbreen28]

When passing custom facts, it would result in a duplicate fact key if those facts were already present, for example:

 puppetversion: "7.25.0",
 facterversion: "4.4.1"

would result in a facts hash like below being returned

:facterversion=>"4.2.11",
:puppetversion=>"7.25.0",
...
"puppetversion"=>"7.25.0",
"facterversion"=>"4.4.1",

Now, when run, the logic will update the element already present in the hash, as opposed to duplicating the sym key as a string.

:puppetversion =>"7.25.0",
:facterversion =>"4.4.1",

Fixes #142

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6ac4fba) 94.93% compared to head (ab01ecc) 95.23%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   94.93%   95.23%   +0.30%     
==========================================
  Files           2        2              
  Lines         158      168      +10     
==========================================
+ Hits          150      160      +10     
  Misses          8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ekohl]

Deep merging by default feels wrong, because then you can't replace facts entirely anymore.

You're also ignoring RSpec.configuration.facterdb_string_keys exists. We should really do a breaking change at some point to and flip it to true. That does mean everyone needs to change their specs. I've also thought about ActiveSupport::HashWithIndifferentAccess but that's a dependency I don't want to pull in.

[jordanbreen28]

Deep merging by default feels wrong, because then you can't replace facts entirely anymore.

You're also ignoring RSpec.configuration.facterdb_string_keys exists. We should really do a breaking change at some point to and flip it to true. That does mean everyone needs to change their specs. I've also thought about ActiveSupport::HashWithIndifferentAccess but that's a dependency I don't want to pull in.

Agreed, seems like bit of an anti-pattern in hindsight to have this deep_merge all the time..
I'll get back to the drawing board and update this as soon as I can.

[ekohl]

There is also the weird bit where top level facts are symbolized but all other levels aren't (and thus strings). Right now I can see someone trying to add a custom (structured) fact to override built in facts and use symbols for the second level. So

add_custom_fact 'identity', { user: "test_user" }, merge_facts: true

The result would be:

{
  "gid"=>0,
  "group"=>"root",
  "privileged"=>true,
  "uid"=>0,
  "user"=>"root",
  :user => "test_user"
}

I tried to look at a setting for this, but I don't think there is one. It happens here: https://github.com/voxpupuli/facterdb/blob/4d3f5e846de545dad89ad5e4196024eedbcaeec2/lib/facterdb.rb#L146

[jordanbreen28]

There is also the weird bit where top level facts are symbolized but all other levels aren't (and thus strings). Right now I can see someone trying to add a custom (structured) fact to override built in facts and use symbols for the second level. So

add_custom_fact 'identity', { user: "test_user" }, merge_facts: true

The result would be:

{
  "gid"=>0,
  "group"=>"root",
  "privileged"=>true,
  "uid"=>0,
  "user"=>"root",
  :user => "test_user"
}

I tried to look at a setting for this, but I don't think there is one. It happens here: https://github.com/voxpupuli/facterdb/blob/4d3f5e846de545dad89ad5e4196024eedbcaeec2/lib/facterdb.rb#L146

Yeah okay that is strange.. Not ideal.

[jordanbreen28]

thanks @ekohl i'll look to address your comments shortly

[jordanbreen28]

Let me know if anything else is needed.

[ekohl]

For the changelog I would really want the custom fact as symbol change separated into its own PR because that part backwards incompatible requiring a major version bump. It does break test suites that have conditionals on custom facts.

[jordanbreen28]

I've separated the two changes between here and #160.
Once this is merged, I'll work to get #160 in shape, hoping a rebase is all it will need but we will see..

[ekohl]

I've rephrased the PR title to better capture what it's covering. I think this is now ready to merge.

[jordanbreen28]

@ekohl thanks for all your guidance with this one

[bastelfreak]

Hi,
We've quite a few unreleased PRs already merged. Two are marked as breaking:

Breaking changes:

• Do not query for the exact facter version #151 (ekohl)
• Drop Ruby 2.4/2.5/2.6 support #149 (bastelfreak)

objections on how we proceed? Should we merge #157 and release it with those changes? Or do two major releases in a row?

[ekohl]

I'd combine them. Possibly also di the breaking change in FacterDB of removing old stuff.

I can't find the issue now, but I'd also consider swapping the string keys value default and only deal with strings instead of symbols/strings

[jordanbreen28]

@ekohl @bastelfreak 👋 can we merge this and I'll start looking at what #160 needs?