Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(maint) replace legacy facts in service provider #8868

Closed
wants to merge 1 commit into from

Conversation

bastelfreak
Copy link
Contributor

This causes issues with spec testing puppet modules on a regular basis.
It's not guaranteed that new factsets from facterdb have legacy facts
because, well, Puppet Inc declared them legacy and in many places they
are opt-in. We (Vox Pupuli) spent alot of time adding some legacy facts
back to some factsets, but it's inconsistent. I think they shouldn't be
used anymore.

@bastelfreak bastelfreak requested a review from a team as a code owner January 28, 2022 07:35
@bastelfreak bastelfreak requested a review from a team January 28, 2022 07:35
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@bastelfreak
Copy link
Contributor Author

I'm not really sure how to mock the structured facts in rspec :(

This causes issues with spec testing puppet modules on a regular basis.
It's not guaranteed that new factsets from facterdb have legacy facts
because, well, Puppet Inc declared them legacy and in many places they
are opt-in. We (Vox Pupuli) spent alot of time adding some legacy facts
back to some factsets, but it's inconsistent. I think they shouldn't be
used anymore.
@b4ldr
Copy link
Contributor

b4ldr commented Mar 4, 2022

I'm not really sure how to mock the structured facts in rspec :(

not pretty but this is what i have done in the past

Also worth pointing out FACT-2913

@joshcooper
Copy link
Contributor

We tried doing this earlier and had to revert back because it broke tests that were only stubbing the legacy fact, but not the equivalent structured fact. So I don't think this is something that can land in a 7.x release.

Also Puppet recently added the ability to inject a facter implementation, see (PUP-11216). When using puppet as a library, such as when testing a module, it's now possible for rspec-puppet to do something like this:

facts = load fact fixtures, e.g. from https://raw.githubusercontent.com/voxpupuli/facterdb/master/facts/4.2/almalinux-8-x86_64.facts
facter_impl = TestFacterImpl.new(facts)
Puppet.initialize_settings(['arg1', 'arg2', ..], true, true, {facter: facter_impl})

This way the test can completely stub whatever facts are needed for the test. One exception is if a module's type/provider is calling Facter.value directly. If so, the module should be updated to call Puppet.runtime[:facter].value(:somefact) instead to ensure we can intercept the lookup.

I think the path forward would be something like:

  1. Ensure facterdb contains fixtures with both legacy and structured facts
  2. Update rspec-puppet to pass in a facter implementation that only uses facterdb to resolve facts (this works in 6.25.0/7.11.0 and up)
  3. Update rspec-puppet to issue deprecation warnings if a module tries to call Facter.value directly
  4. In Puppet 8, switch puppet to confine providers based on structured facts only.

Thoughts?

/cc @binford2k

@binford2k
Copy link
Contributor

At least the starts of the test implementation went in with puppetlabs/rspec-puppet#16

I did a quick scan and here's a list of all the Forge modules that invoke Facter.value directly. https://docs.google.com/spreadsheets/d/1O3sE8xsCFat2uYgb-4fAyu02ctYCZ78VoRXzDsAn4-k/edit#gid=565587507

If we wanted to, I could script out an "update the world" bot to propose PRs to the new API.

@bastelfreak
Copy link
Contributor Author

@joshcooper

We tried doing this earlier and had to revert back because it broke tests that were only stubbing the legacy fact, but not the equivalent structured fact. So I don't think this is something that can land in a 7.x release.

you say that "just" because it's time consuming to update the tests? It shouldn't be a breaking change, or am I missing something?

@binford2k if the bot can provide a working PR, I'm +100000 for that.

@joshcooper
Copy link
Contributor

Thanks @bastelfreak We ended up fixing this in 82cef23 so I'm going to close this.

@joshcooper joshcooper closed this Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants