Make it easy to test without legacy facts

[ekohl]

Use Case

Puppet 7.21.0 gained the configuration option include_legacy_facts. It defaults to true but Puppet 8 will default to false. It would make future proofing of modules easier if you could already test in rspec-puppet with include_legacy_facts set to false.

Describe the Solution You Would Like

Add an option to rspec-puppet to use this.

Describe Alternatives You've Considered

It is very common to use rspec-puppet-facts and perhaps that should gain the option. Then FacterDB does need another set of facts for each OS and Facter version.

[ekohl]

There was a PR: #41 and now there's also voxpupuli/rspec-puppet-facts#143. Perhaps this should be combined.

[ekohl]

Note we configure the actual Facter implementation here:

rspec-puppet/lib/rspec-puppet/adapters.rb

Lines 240 to 260 in 612d8af

	def setup_puppet(example_group)
	case RSpec.configuration.facter_implementation.to_sym
	when :rspec
	if supports_facter_runtime?
	# Lazily instantiate FacterTestImpl here to optimize memory
	# allocation, as the proc will only be called if FacterImpl is unset
	set_facter_impl(proc { RSpec::Puppet::FacterTestImpl.new })
	Puppet.runtime[:facter] = FacterImpl
	else
	warn "Facter runtime implementations are not supported in Puppet #{Puppet.version}, continuing with facter_implementation 'facter'"
	RSpec.configuration.facter_implementation = :facter
	set_facter_impl(Facter)
	end
	when :facter
	set_facter_impl(Facter)
	else
	raise "Unsupported facter_implementation '#{RSpec.configuration.facter_implementation}'"
	end
	super
	end

I think one question we should ask ourselves is how to transition the setting from here to rspec-puppet-facts: do we explicitly call some method here in the adapter, or respect a setting on rspec-puppet inside rspec-puppet-facts.

Given we don't depend on rspec-puppet-facts here, I think calling something in the adapter is would be wrong. It's better to fix it in rspec-puppet-facts.

[alexjfisher]

rspec-puppet/lib/rspec-puppet/facter_impl.rb

Lines 10 to 12 in 612d8af

	def value(fact_name)
	@facts[fact_name.to_s]
	end

needs improving to work with the way puppetlabs/puppet@82cef23 was written.
ie it needs to support returning facts accessed with dot notation.

[alexjfisher]

@ekohl Should we add a task list to this issue description to try to figure out what needs to be done, where and in what order?

(or maybe somewhere under vox's namespace where we can both edit it??)

I dunno... I think we're currently looking at something like...

• Fix rspec-puppet facter implementation to support fetching facts with dot-notation [1]
• Add include_legacy_facts configuration setting to rspec-puppet [2]
• Update rspec-puppet-facts to honour the new rspec-puppet setting [3]

Later...

• Remove fact sets from facterdb that are missing structured facts
• Update rspec-puppet-facts code to use structured os fact when finding factset
• Backport Puppet 8's removal of legacy-facts in providers to Puppet 7???

Notes:
1 - #46
2 - The implementation should set a default based on Puppet.version. There is a closed PR that could be resurrected.
3 - rspec-puppet-facts should strip out the legacy facts returned from facterdb if the rspec-puppet setting exists (so we don't have to depend on it??) and is set to false

[ekohl]

Overall I think that's pretty much what I wanted to write down as a plan, so certainly 👍

• Backport Puppet 8's removal of legacy-facts in providers to Puppet 7???

What do you mean by this?

[alexjfisher]

The recent commit on main that changes all the provider confinement stuff to use structured facts hasn't been backported to Puppet 7, (so even though Puppet 7 has an include_legacy_facts configuration option that defaults to true), we won't be able to switch it to false (eg. in voxpupuli-test) without tests failing due to puppet not being able to find suitable service providers etc.

[alexjfisher]

In https://groups.google.com/g/puppet-dev/c/hFIjgvOa5J0 @joshcooper wrote...

For Puppet 7.x, I'd like to:
Add a configuration setting "include_legacy_facts" so Puppet can opt-into not sending legacy facts to puppetserver (PUP-11662) In a default agent configuration, this results in 25% less fact data. This is safe so long as legacy facts are not used during compilation (manifests, hiera configuration & data sources). There's already a legacy_facts puppet-lint plugin to help users migrate.
Provider confining/suitability won't be affected because "Facter.value()" will still resolve legacy facts on the agent.
Fix the block list slowness
For Puppet 8.0, I'd like to:
Change the default value of "include_legacy_facts" to false (PUP-11430)
Change Puppet's builtin type/providers to only reference structured facts (PUP-8543). I think this is only safe to do in Puppet 8, because of the way modules use rspec-puppet & rspec-puppet-facts to stub provider suitability.

Specifically, will the "I think this is only safe to do in Puppet 8, because of the way modules use rspec-puppet & rspec-puppet-facts to stub provider suitability." statement still be true if we make the changes being planned?

[joshcooper]

even though Puppet 7 has an include_legacy_facts configuration option that defaults to true), we won't be able to switch it to false

@alexjfisher the include_legacy_facts setting only controls whether legacy facts are included with the agent's catalog request. Puppet 7 agents should still be able to resolve legacy facts during catalog application.

[alexjfisher]

I'm no longer convinced that the pruning of facts should happen in rspec-puppet-facts as rspec-puppet itself sets some legacy facts by default and these need pruning too.

[alexjfisher]

Maybe this PR is the right direction instead? #53