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

Remove dnssec-enable option for EL >= 9 #218

Merged
merged 6 commits into from
Sep 25, 2022
Merged

Remove dnssec-enable option for EL >= 9 #218

merged 6 commits into from
Sep 25, 2022

Conversation

ikonia
Copy link
Contributor

@ikonia ikonia commented Sep 13, 2022

validated against EL (Redhat / CentOS / Rocky) 7 - 8 - 9 functions correctly.
Basic test against Fedora 35, which also works

Adds check of EL version greater than 7 and sets dnssec_enable parameter to undef if greater to allow the removal of the parameter from options.conf, which is a deprecated parameter in EL8 and later. This removes any warning in 9.15 and any breakage in 9.18 or later. Any version EL 7 or earlier includes the parameter allowing the user to set it on and off depending on if they want to use it, but the parameter is included as a valid parameter

Comment on lines 57 to 60
$dnssec_enable = $facts['os']['Family'] ? {
'RedHat' => if versioncmp($facts['os']['release']['major'], '8') >= 0 { undef } else { 'yes' },
default => undef,
}
Copy link
Member

Choose a reason for hiding this comment

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

You're already in the RedHat os family branch.

Suggested change
$dnssec_enable = $facts['os']['Family'] ? {
'RedHat' => if versioncmp($facts['os']['release']['major'], '8') >= 0 { undef } else { 'yes' },
default => undef,
}
$dnssec_enable = if versioncmp($facts['os']['release']['major'], '8') >= 0 { undef } else { 'yes' }

However, I do think that on EL8 it's still relevant. That has 9.11 and only 9.15 made it obsolete. So should it be this?

Suggested change
$dnssec_enable = $facts['os']['Family'] ? {
'RedHat' => if versioncmp($facts['os']['release']['major'], '8') >= 0 { undef } else { 'yes' },
default => undef,
}
$dnssec_enable = if versioncmp($facts['os']['release']['major'], '9') >= 0 { undef } else { 'yes' }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd not fully realised that higher up I'd joined the RedHat branch, this was a great spot and helpful, thank you

@ikonia
Copy link
Contributor Author

ikonia commented Sep 13, 2022

excellent feedback, happy to make the changes, I got the warning on my CentOS 8 bind host though, so I will go back and double check the version on that.

@@ -54,7 +54,8 @@
# This option is not relevant for RedHat
$sysconfig_resolvconf_integration = undef

$dnssec_enable = 'yes'
$dnssec_enable = if versioncmp($facts['os']['release']['major'], '9') >= 0 { undef } else { 'yes' }
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}

@ikonia
Copy link
Contributor Author

ikonia commented Sep 22, 2022

do I need to look at the rspec test suites on this as while the module works fine on EL9 (which is where the test suite is failing) I'm not sure if these tests are known to fail, or if I should dig into them and look to update them.
If they are expected passes I can look into updating them

@ekohl
Copy link
Member

ekohl commented Sep 22, 2022

Yes, the failures are caused by your change.

@ikonia
Copy link
Contributor Author

ikonia commented Sep 22, 2022

thank you, I'll look at the tests now.

@ikonia
Copy link
Contributor Author

ikonia commented Sep 22, 2022

tests updated, thanks for the help

@ekohl ekohl changed the title enabled removal of dnssec-enable option for EL versions greater than 7 Remove dnssec-enable option for EL >= 9 Sep 25, 2022
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl merged commit 9db91f1 into theforeman:master Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants