-
Notifications
You must be signed in to change notification settings - Fork 6
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
Adjust Puppet settings to match Puppet 8 defaults #92
base: master
Are you sure you want to change the base?
Conversation
Puppet 8 will ship without legacy facts and with strict mode enabled by default. Adjust CI settings to match this configuration for all versions of Puppet in order to match the most stict and less permissive configuration. This is expected to help detecting some issues even before Puppet 8.0.0 is released.
# We want to use these settings as a common denominator for all versions of | ||
# Puppet with test modules against. | ||
Puppet[:include_legacy_facts] = false | ||
Puppet[:strict_variables] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think strict_variables and strict is already set somewhere, maybe in puppetlabs_spec_helper. Setting it here probably won't hurt.
# | ||
# We want to use these settings as a common denominator for all versions of | ||
# Puppet with test modules against. | ||
Puppet[:include_legacy_facts] = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work on Puppet 6 as well? Can you maybe raise a PR against puppet-example that tests this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought as in the general plan, but I have doubts about the implementation.
I think we actually want to push this deeper in the test stack. rspec-puppet feels like the right place. Then we can configure rspec-puppet.
And rspec-puppet-facts needs to be taught about legacy vs non-legacy facts. My thought was that we should collect fact sets with and without, rather than filtering them out. That also means changes to FacterDB.
In addition to that: this can break a lot of modules so I think it's a major version bump. Then when we do a module sync we will find the issues and fix them.
# | ||
# We want to use these settings as a common denominator for all versions of | ||
# Puppet with test modules against. | ||
Puppet[:include_legacy_facts] = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works with rspec-puppet-facts. See puppetlabs/rspec-puppet#41 as well.
# Puppet with test modules against. | ||
Puppet[:include_legacy_facts] = false | ||
Puppet[:strict_variables] = true | ||
Puppet[:strict] = :error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting doesn't appear to exist in rspec-puppet.
# We want to use these settings as a common denominator for all versions of | ||
# Puppet with test modules against. | ||
Puppet[:include_legacy_facts] = false | ||
Puppet[:strict_variables] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should configure this via rspec-puppet: https://github.com/puppetlabs/rspec-puppet#strict_variables
Puppet 8 will ship without legacy facts and with strict mode enabled by
default. Adjust CI settings to match this configuration for all
versions of Puppet in order to match the most stict and less permissive
configuration. This is expected to help detecting some issues even
before Puppet 8.0.0 is released.