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.

Sign up for GitHub

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

Jump to bottom

Add include_legacy_facts setting #41

Closed
wants to merge 1 commit into from
Closed

Add include_legacy_facts setting #41

wants to merge 1 commit into from

Conversation

crazymind1337
Copy link

@crazymind1337 crazymind1337 commented Feb 20, 2023 •
edited
Loading

Well it is my first PR here. I hope this is correct. Please tell if I need to change anything.

I would like to enable the setting include_legacy_facts. We would like to be able to test the code for legacy facts since we already would like to make our code puppet 8 compatible.

Default value would be true and could be changed via:

RSpec.configure do |config|
  ##
  ## disable legacy facts
  ##
  config.include_legacy_facts = false
end

@crazymind1337 crazymind1337 requested a review from a team as a code owner February 20, 2023 09:25
@chelnak
Copy link

chelnak commented Feb 20, 2023

Hey thank you for this.

I think you might need to rebase as your PR contains a commit from @binford2k (unless I'm missing something).

Also could you add a bit more of a description to the PR as it helps with triage.

Thank you 😊

@crazymind1337 crazymind1337 force-pushed the add_include_legacy_facts_setting branch from e422745 to 57620ec Compare February 20, 2023 09:32
@crazymind1337
Copy link
Author

rebased - this might have come from the old repository. I have just migrate to puppetlabs/rpsec-puppet.

@crazymind1337
Copy link
Author

Added some more description

@crazymind1337
Copy link
Author

Sadly it doesn't really with. I will convert to draft.

@crazymind1337 crazymind1337 marked this pull request as draft February 20, 2023 09:48
@crazymind1337
Copy link
Author

Stupid me is using

  config.facter_implementation = 'rspec'

and not

  config.facter_implementation = 'facter'

There this change would have no affect.

@crazymind1337 crazymind1337 deleted the add_include_legacy_facts_setting branch February 20, 2023 10:05
@ekohl
Copy link

ekohl commented Feb 21, 2023

This may still be a good idea, but we should teach rspec-puppet-facts and FacterDB about non-legacy-factsets. If rspec-puppet-facts looks at the setting, that would make it nicely integrated.

This was referenced Mar 13, 2023
Open
Open
alexjfisher
alexjfisher reviewed Mar 14, 2023
View reviewed changes
lib/rspec-puppet.rb
@@ -60,6 +60,7 @@ def self.current_example
c.add_setting :fixture_hiera_configs, :default => {}
c.add_setting :use_fixture_spec_hiera, :default => false
c.add_setting :fallback_to_default_hiera, :default => true
c.add_setting :include_legacy_facts, :default => true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default could/should depend on Puppet.version.

alexjfisher
alexjfisher reviewed Mar 14, 2023
View reviewed changes
spec/unit/adapters_spec.rb
@@ -204,6 +204,11 @@ def context_double(options = {})

let(:test_context) { double :environment => 'rp_env' }

it 'sets Puppet[:strict_variables] to false by default' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy and paste error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjfisher alexjfisher alexjfisher left review comments

@bastelfreak bastelfreak Awaiting requested review from bastelfreak bastelfreak will be requested when the pull request is marked ready for review bastelfreak is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crazymind1337 @chelnak @ekohl @alexjfisher