facterdb
facterdb copied to clipboard
voxpupuli
Remove Legacy Facts
Puppet 8 defaults to not providing Legacy Facts 1. In order for our tests to be relevant, we should not provide these facts. We also had special instructions to ask contributors to add these legacy facts which are now obsolete.
As a baseline, do not provide any legacy facts. Adjust references to legacy fact to use non deprecated and still relevant ones in CI, and provide a script that can asists us in updating the existing factsets.
I now see you have a commit that does that. It's a bit tricky to see.
Something I do miss is the change to facterdb/get_facts.sh to drop --show-legacy.
Something I do miss is the change to
facterdb/get_facts.shto drop--show-legacy.
Good catch, it looks like I git checkout -- . before committing it :facepalm: Fixed!
Would something in rspec-puppet-facts be better? Like some sort of configurable that would remove the legacy facts??
It's only a default in Puppet 8 to not have legacy facts. Presumably some people will be happy to turn them back on, so I think I'd prefer if this was done elsewhere if possible???
I'm thinking something like...
RSpec.configure do |c|
c.legacy_facts = false
end
(or we could default to false but people could set to true if they want legacy facts).
In here somewhere??? https://github.com/voxpupuli/rspec-puppet-facts/blob/1418bca958049060a77dd49f10a9a298d15dc438/lib/rspec-puppet-facts.rb#L172-L173
[RspecPuppetFacts.legacy_facts].each { |fact| facts.delete(fact) } unless opts[:legacy_facts])
When I was thinking about this problem I actually thought of creating 2 fact sets: one with and one without legacy facts. Then in RspecPuppetFacts you would create an option to select either one or the other.
My biggest worry was performance, given it's already rather slow due to its size. Nowadays it's mitigated by having a cache, but it's still slow.
So what I'd consider is making it a completely new fact set that's only available opt-in.
Any thoughts on that?
I thought it was probably easier to conditionally just drop the keys that are legacy facts? It's a published fixed list that isn't about to change.
While that's true, I'm a bit hesitant in generating data while in my head the aim of this is to represent what the real code does.
I thought it was probably easier to conditionally just drop the keys that are legacy facts? It's a published fixed list that isn't about to change.
Actually it's a little bit more involved. There are some legacy facts that are dynamically generated, eg. https://www.puppet.com/docs/puppet/7/core_facts.html#ipaddress-interface etc.
Also worth bearing in mind that rspec-puppet-facts currently searches facterdb based on legacy facts. https://github.com/voxpupuli/rspec-puppet-facts/blob/master/lib/rspec-puppet-facts.rb would need a complete overhaul before dropping legacy facts here.
Something like this perhaps?? https://github.com/voxpupuli/rspec-puppet-facts/pull/143
That's weird - I thought I pushed to my fork - how does it show up here?
yakatz:~/local_git/puppet/modules/facterdb (remove-legacy-facts $=)$ git remote -v
origin [email protected]:yakatz/facterdb.git (fetch)
origin [email protected]:yakatz/facterdb.git (push)
yakatz:~/local_git/puppet/modules/facterdb (remove-legacy-facts $=)$
EDIT: I figured out the git client "magic" that made this happen and I will make sure it doesn't happen again.
Due to its size this is hard to review. Would it make sense to have a commit that removes Facter < 3?
I also wonder if Facter 3 had all the non-legacy facts.
I'd even consider dropping all Facter < 3.14.
Since I accidentally rebased this, it should be easier to read now.
proposal: the next major release drops support for facter 3 and older and some EoL OS. For the major release after that should we update get_facts.sh to remove all the legacy facts? Or should we do this already in the next major? puppet-lint has a plugin for legacy facts since some time now that's rolled out by default in PDK and at Vox Pupuli. I'm fine with dropping the legacy facts already.
I think deprecating it in the next major and removing it the major after that can make sense, depending on the timeline.
Do we know when Puppet 7 is going EOL? While not strictly tied to it, it makes sense to me.
Puppet 7 lifecycle is related to the PE lifecycle: https://www.puppet.com/products/puppet-enterprise/support-lifecycle
PE 2021 contains Puppet 7:
Mainstream Support through August 31, 2024. Overlap Support through February 28, 2025.
I suggest wo do a major release now and mention in the README.md that legacy facts are deprecated and will be removed in the next major release. And when Puppet 7 is EoL we do another major release without legacy facts.
As someone who has the server infrastructure provided by another team in our org, it's to early for me now :( But I do agree on this proposal https://github.com/voxpupuli/facterdb/pull/266#issuecomment-2117152080
As someone who has the server infrastructure provided by another team in our org,
how does that relate to this change? Do they also provide puppet code with legacy facts?
sadly yes. But mostly I don't know when I will be able to switch to puppet 8
Keep in mind that the legacy facts are legacy since Puppet 5 or 6 and since that time the new ones are available. Just puppet 8 finally disables the legacy facts by default (but you can reenable them).
Keep in mind that the legacy facts are legacy since Puppet 5 or 6 and since that time the new ones are available. Just puppet 8 finally disables the legacy facts by default (but you can reenable them).
They were just called legacy facts. I don't think anyone actually deprecated them. It's a shame the change to rspec-puppet to configurably strip them never got over the line. I just ran out of time.
I don't think anyone actually deprecated them
We (Vox Pupuli) did that a long time ago and so does the pdk since some time.
The longer this sits, the more I think it should be two separate PRs - one to remove the code to generate the legacy facts and a separate one after that one is merged that actually removes the factsets. That will make this easier to merge. Any objections?
We made the 2.0.0 release that announced the legacy fact deprecation. I think we can now tackle this and work on the 3.0.0 release which removes the lagacy facts.
Just speaking from my own opinion, I think this is a good change to get in. Anyone that is still using Legacy Facts shouldn't be as they are no longer supported within the latest Puppet.
@smortex do you have a chance to rebase/redo this PR? I would like to then merge it.
@smortex do you have a chance to rebase/redo this PR? I would like to then merge it.
Luckily I had the original branch and could cherry-pick / adjust / regenerate / skip commits :grin:.