Navigation Menu

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

Add support for EL8 #39

Merged
merged 1 commit into from Oct 14, 2020
Merged

Add support for EL8 #39

merged 1 commit into from Oct 14, 2020

Conversation

treydock
Copy link
Collaborator

Pull Request Checklist

Description

Add support for EL8 by installing EL7 packages.

@treydock treydock added the enhancement New feature or request label Oct 13, 2020
@@ -8,12 +8,16 @@
if $sensuclassic::repo_source {
$url = $sensuclassic::repo_source
} else {
$major = $facts.dig('os', 'release', 'major')
Copy link
Collaborator

Choose a reason for hiding this comment

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

why dig? why not just address the facts by name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because it would require more complete unit tests because if you do $facts['os']['family'] and $facts['os'] is not defined, you get errors. Dig is much safer when traversing deep hash in case facts are missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also ever since Puppet 5 dig is the better way to walk a hash, just safer and less verbose than checking each intermediate key exists. Might have been around since Puppet 4, but I know it's built-in to Puppet 5.

Copy link
Collaborator

@ghoneycutt ghoneycutt left a comment

Choose a reason for hiding this comment

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

These facts always exist and if they didn't something would be terribly wrong. This makes sense to me if we are digging into data structures that are more dynamic. Please just access the facts as that makes it easier to read.

@treydock
Copy link
Collaborator Author

I tried using without dig and that results is a majority of the unit tests failing because the unit tests do not use rspec-puppet-facts and instead rely on specifying facts manually and many of the facts we need in this change are not defined. So using dig is a sort of soft fail approach vs a hard fail and right now the unit tests are getting a hard fail even though in the real world the change would not cause failures. Do we want to go through the effort of updating the numerous unit tests or just stick with the "soft fail" approach of using dig that does not require lots of unit test updates?

Looks like to remove the use of dig would require updating nearly every single unit test which is a big change:

Finished in 1 minute 8.15 seconds (files took 2.11 seconds to load)
963 examples, 606 failures

@ghoneycutt ghoneycutt merged commit 14a256a into master Oct 14, 2020
@ghoneycutt ghoneycutt deleted the el8 branch October 14, 2020 20:57
@ghoneycutt
Copy link
Collaborator

Released in v3.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants