-
-
Notifications
You must be signed in to change notification settings - Fork 59
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 unnecessary test on $service_ensure #88
Conversation
$service_ensure is required by init.pp to have type Stdlib::Ensure::Service, so there is no need for this test.
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.
🙋🏻♀️
@@ -7,10 +7,6 @@ | |||
$service_manage = $chrony::service_manage, | |||
$service_name = $chrony::service_name, | |||
) inherits chrony { |
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.
why does this class inherit from chrony
that's really bad form for anything other than the params class
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 have no idea. It has been that way since commit 74c0dc8 when the the file was added.
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.
okay
let's remove those in a future PR
i'm pretty, and sure, there's a standard puppet lint that should be complaining about this
have we gotten module/pdk sync yet for this module?
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.
If you want, you can also remove parameters from name
, hasstatus
and hasrestart
since they match defaults.
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.
@igalic let's keep the inherits for a separate PR indeed. Any objections to merging this now?
Oops. I will have to change the tests also. Don't merge until I have fixed that. |
The name defaults to the resource title, and both hasstatus ans hasrestart is default true.
$service_ensure is required by init.pp to have type Stdlib::Ensure::Service, so there is
no need for this test.