Remove Legacy Facts #266

smortex · 2023-03-13T22:01:32Z

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.

ekohl

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.

ekohl · 2023-03-13T23:36:47Z

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.

smortex · 2023-03-14T03:07:52Z

Something I do miss is the change to facterdb/get_facts.sh to drop --show-legacy.

Good catch, it looks like I git checkout -- . before committing it 🤦 Fixed!

alexjfisher · 2023-03-14T09:09:43Z

Would something in rspec-puppet-facts be better? Like some sort of configurable that would remove the legacy facts??

alexjfisher · 2023-03-14T09:14:22Z

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???

alexjfisher · 2023-03-14T09:18:04Z

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).

alexjfisher · 2023-03-14T09:26:49Z

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])

ekohl · 2023-03-14T09:52:14Z

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?

alexjfisher · 2023-03-14T10:01:53Z

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.

ekohl · 2023-03-14T10:20:22Z

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.

alexjfisher · 2023-03-14T10:20:38Z

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.

alexjfisher · 2023-03-14T10:34:17Z

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.

alexjfisher · 2023-03-14T14:53:47Z

Something like this perhaps?? voxpupuli/rspec-puppet-facts#143

yakatz · 2024-05-16T21:51:46Z

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.

yakatz · 2024-05-16T22:03:35Z

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.

bastelfreak · 2024-05-16T22:39:59Z

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.

ekohl · 2024-05-17T08:39:02Z

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.

bastelfreak · 2024-05-17T09:37:08Z

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.

bastelfreak · 2024-05-17T09:38:07Z

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.

TheMeier · 2024-05-17T13:52:32Z

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 #266 (comment)

bastelfreak · 2024-05-17T13:56:46Z

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?

TheMeier · 2024-05-17T14:00:07Z

sadly yes. But mostly I don't know when I will be able to switch to puppet 8

bastelfreak · 2024-05-17T14:16:54Z

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).

bastelfreak · 2024-05-17T14:50:20Z

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.

yakatz · 2024-05-17T15:03:43Z

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?

bastelfreak · 2024-05-28T09:16:02Z

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.

david22swan · 2024-06-06T13:27:06Z

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.

bastelfreak · 2024-06-06T19:31:09Z

@smortex do you have a chance to rebase/redo this PR? I would like to then merge it.

smortex · 2024-06-09T20:21:20Z

@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 😁.

bastelfreak · 2024-06-09T20:24:24Z

bin/legacy-facts-striper

+  selinux_enforced
+  selinux_policyversion
+  serialnumber
+  sp_boot_mode


okay I've never seen those sp_* facts before.

Hehe, me neither! Looks like sp stand for system_profiler and is something related to MaxOSX 🤷

jep, its macOS stuff... its now collected as system_profiler structured fact.

bastelfreak · 2024-06-09T20:26:51Z

spec/facts_spec.rb

-      # those are used in newer rspec-puppet-facts versions
-      # by ensuring those facts exists, we can drop a huge amount of logic in rspec-puppet-facts
-      it 'contains the mandatory OS facts' do
-        expect(content['os']['release']['major']).to not_be_nil.and not_be_empty
-        expect(content['os']['name']).to not_be_nil.and not_be_empty
-        expect(content['os']['hardware']).to not_be_nil.and not_be_empty


can you maybe keep those checks? we rely on those in rspec-puppet-facts and assume they exist. And there were situations in the past where factsets were provided for operating systems that aren't 100% supported and those facts were missing or null.

Good catch, I think that the initial code was looking for the legacy facts, and after rebasing I misread the conflicts that changed them to os.xxx facts and removed them by mistake.

Reintroduced, but I kept the check that ensure legacy facts are not found.

rwaffen · 2024-06-10T09:14:07Z

bin/legacy-facts-striper

+  selinux_enforced
+  selinux_policyversion
+  serialnumber
+  sp_boot_mode


jep, its macOS stuff... its now collected as system_profiler structured fact.

facts/4.0/almalinux-8-x86_64.facts

bin/legacy-facts-striper

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. [1]: https://github.com/puppetlabs/puppet/wiki/Puppet-8-Compatibility#legacy-facts

This is the result of: ``` bundle exec bin/legacy-facts-striper facts/*/*.facts ```

Remove the `--show-legacy` option and prefer long options to the short ones.

smortex marked this pull request as ready for review March 13, 2023 22:48

ekohl requested changes Mar 13, 2023

View reviewed changes

This was referenced Mar 19, 2024

Increase test coverage #246

Open

Modernized Windows Support and documentation update #306

Merged

yakatz force-pushed the remove-legacy-facts branch 2 times, most recently from 24d6302 to 1e2cc2a Compare May 16, 2024 21:49

yakatz force-pushed the remove-legacy-facts branch 2 times, most recently from e43e75e to b7d2e17 Compare May 16, 2024 22:01

bastelfreak added the backwards-incompatible label May 18, 2024

bastelfreak mentioned this pull request May 18, 2024

deprecate legacy facts #350

Merged

bastelfreak mentioned this pull request Jun 8, 2024

work with symbolized strings voxpupuli/rspec-puppet-facts#175

Merged

smortex force-pushed the remove-legacy-facts branch 2 times, most recently from 8877cb0 to 6184373 Compare June 9, 2024 20:19

bastelfreak reviewed Jun 9, 2024

View reviewed changes

smortex force-pushed the remove-legacy-facts branch from 6184373 to 513e70e Compare June 9, 2024 20:54

bastelfreak approved these changes Jun 9, 2024

View reviewed changes

smortex requested a review from ekohl June 9, 2024 21:37

This was referenced Jun 10, 2024

Remove EoL CentOS 8 factsets #361

Merged

Remove deprecated RedHat 7 facts #359

Merged

Remove EoL CentOS 7 factsets #360

Merged

rwaffen reviewed Jun 10, 2024

View reviewed changes

bastelfreak reviewed Jun 10, 2024

View reviewed changes

bin/legacy-facts-striper Outdated Show resolved Hide resolved

smortex added 3 commits June 10, 2024 03:24

Update factsets

24a2dbd

This is the result of: ``` bundle exec bin/legacy-facts-striper facts/*/*.facts ```

Update facter options

f38c29a

Remove the `--show-legacy` option and prefer long options to the short ones.

smortex force-pushed the remove-legacy-facts branch from 513e70e to f38c29a Compare June 10, 2024 13:25

smortex requested review from rwaffen and bastelfreak June 10, 2024 13:28

bastelfreak approved these changes Jun 10, 2024

View reviewed changes

bastelfreak merged commit 1a3dad5 into master Jun 10, 2024
7 checks passed

bastelfreak deleted the remove-legacy-facts branch June 10, 2024 15:33

Remove Legacy Facts #266

Remove Legacy Facts #266

Conversation

smortex commented Mar 13, 2023

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Mar 13, 2023

smortex commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

ekohl commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

ekohl commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

alexjfisher commented Mar 14, 2023

yakatz commented May 16, 2024 • edited Loading

yakatz commented May 16, 2024

bastelfreak commented May 16, 2024

ekohl commented May 17, 2024

bastelfreak commented May 17, 2024

bastelfreak commented May 17, 2024

TheMeier commented May 17, 2024

bastelfreak commented May 17, 2024

TheMeier commented May 17, 2024

bastelfreak commented May 17, 2024

bastelfreak commented May 17, 2024

yakatz commented May 17, 2024

bastelfreak commented May 28, 2024

david22swan commented Jun 6, 2024

bastelfreak commented Jun 6, 2024

smortex commented Jun 9, 2024 • edited Loading

bastelfreak Jun 9, 2024

Choose a reason for hiding this comment

smortex Jun 9, 2024

Choose a reason for hiding this comment

rwaffen Jun 10, 2024

Choose a reason for hiding this comment

bastelfreak Jun 9, 2024

Choose a reason for hiding this comment

smortex Jun 9, 2024

Choose a reason for hiding this comment

rwaffen Jun 10, 2024

Choose a reason for hiding this comment

yakatz commented May 16, 2024 •

edited

Loading

smortex commented Jun 9, 2024 •

edited

Loading