-
Notifications
You must be signed in to change notification settings - Fork 50
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
Rocky 8 support #194
Rocky 8 support #194
Conversation
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.
LGTM I reckon!
manifests/init.pp
Outdated
systemd::dropin_file { 'freeradius remove bootstrap': | ||
ensure => present, | ||
filename => 'remove_bootstrap.conf', | ||
unit => 'radiusd.service', #@todo programmatically determine the service name |
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.
Let's remove this todo / create an issue in its place?
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.
Or maybe we don't need to as this is only on el8, so it's always going to be radius.service..
Want me to have a poke at the PDK tests failing, @djjudas21 ? I am deep in puppet brain right at the minute, and just finished something else, so now is a good time.. |
Yes please @nward! |
Huh, interesting. I’ve just finished for the night (it’s about 145am) so will have a look in the morning. |
6ccec74
to
59336a3
Compare
OK - fixed, removed the duplicate dependency. I also rebased on main rather than a merge (keeps the history tidier), hence the force push :) |
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.
LGTM - we should squash+merge as there's some reworking going on here which confuses the history a bit. Can we do that while keeping credit for the changes to @minorOffense? If GitHub doesn't let us I can do that locally and force push.
I suspect using the inbuilt squash + merge will attribute the resulting commit to the person who clicks the button |
c68f394
to
a828e87
Compare
a46fabd
to
8d5fcd5
Compare
8d5fcd5
to
564a719
Compare
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.
OK @djjudas21 I've added some tests - good to merge if you are happy with this.
If you're happy and it's passing tests, then I'm happy! Will merge now |
This PR includes the changes by @minorOffense from #178 which was incomplete, and some fixes by @nward from the
144-rocky8
branch.Supersedes #178
Fixes #144