(PUP-10853) Remove legacy facts

[bastelfreak]

Replaces legacy facts with newer structured facts.

This is cherry-picked from 82cef23

Puppet 7 already has an option to run without legacy facts. But disabling them causes a lot of issues. To workaround this, I backported the commit, that replaces them with modern facts, from the main branch.

[puppetlabs-jenkins]

Can one of the admins verify this patch?

[joshcooper]

I think this is ok from a purely "does puppet agent work" perspective. But I'm fairly certain it will break other parts of the ecosystem. Thinking of rspec-puppet and modules that stub out facts selectively. Do you have some confidence that vox modules won't be affected?

[bastelfreak]

Latest FacterDB doesn't contain any legacy facts. We were at the assumption that when there's an option like 'include_legacy_facts' it's safe to set it to false. And we need a FacterDB version without legacy facts to validate if modules work on Puppet 8. And maintaining different tooling versions for Testing on Puppet 7 vs Puppet 8 is incredible complicated because we don't do that at all at the moment.

An example of issues we run into at the moment:

• https://github.com/voxpupuli/puppet-icinga2/actions/runs/10093309162/job/27908764700?pr=780 fails with latest FacterDB
• because of https://github.com/puppetlabs/puppet/blob/main/lib/puppet/type/exec.rb#L294
• based on https://github.com/puppetlabs/puppet/blob/7.31.0/lib/puppet/provider/exec/windows.rb#L5-L6

Latest FacterDB, rspec-puppet-facts and rspec-puppet don't rely on legacy facts. It's the opposite, they demand structured facts.

[bastelfreak]

Another broken module due to this:

• modulesync 9.2.0 voxpupuli/puppet-nftables#250

[joshcooper]

Another broken module due to this:

Would the issue have been detected if rspec-puppet was modified to warn about legacy facts? For example, in the FacterImpl#avlue method https://github.com/puppetlabs/rspec-puppet/blob/5084326780869b0d60aa066c8ec234d02bab7307/lib/rspec-puppet/facter_impl.rb#L11 have it check if value matches one of these https://github.com/puppetlabs/puppet-lint/blob/main/lib/puppet-lint/plugins/legacy_facts/legacy_facts.rb#L12-L104?

[bastelfreak]

Would the issue have been detected if rspec-puppet was modified to warn about legacy facts?

Maybe. But that would only work if we downgrade facterdb to have legacy facts again. And if rspec-puppet works with a deny list there's a chance that we miss a fact. I still would prefer to merge this PR. I don't see potential impact for an actual running puppet agent. Only in tests when you don't use rspec-puppet-facts (which is the recommendation to use since years) + explicitly stub the facts hash with legacy facts without modern facts. And even that is deprecated since years. Those facts are marked as legacy and deprecated for years.

[ekohl]

I like where @joshcooper is heading. Technically rspec-puppet could look at the Puppet version (which it already does in various places). For Puppet 7 it could "rewrite" the fact for the known list of facts. That means we don't have to update FacterDB and test code can simply stub the modern fact consistently while still working in old Puppet versions.

Yielding a warning is a good thing, but if Puppet always looks up some values that warning is going to be shown every single time. If a user can't get rid of a warning, they're going to ignore it. It's probably best to filter out the facts Puppet uses.

From a code perspective it introduces less risk since it's only in the testing framework, not actual application code.

[joshcooper]

This discussion reminds me of a previous regression with provider confinement/suitability based on facts. IIRC we modified something in puppet and the init service provider broke module CI because rspec-puppet stubbed facts a certain way and the provider called downcase on a nil fact value. Maybe this was it? 9c6ce9c

So while I agree this change is relatively safe for puppet, for example osfamily and os.family will resolve the same way, there's a high likelihood this will break someone's module CI. And since Puppet 7.0 was released on Nov 2020, I don't think we can prescribe a specific workflow for modules 3+ years later.

I'm wondering if there's a way for rspec-puppet to differentiate legacy fact lookups caused by builtin providers (and ignore those) versus everything else.