-
Notifications
You must be signed in to change notification settings - Fork 582
Add a type for ensure on service resources #750
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 a type for ensure on service resources #750
Conversation
Would ServiceEnsure or EnsureService be easier to read? |
Only first character uppercase, for consistency with existing types (f.e:
|
A third option is |
types/serviceensure.pp
Outdated
@@ -0,0 +1 @@ | |||
type Stdlib::Serviceensure = Enum['stopped', 'running'] |
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.
should this not be
type Stdlib::Serviceensure = Varient[Boolean,Enum['stopped', 'running']]
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.
IMO, using ensure => true
or ensure => false
on a service is an anti-pattern. It works but it's just not as clear to new users what is going on. As a convention I don't support it.
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 tend to agree with you however if this is going into stdlib then i think it should conform to the documentation
https://puppet.com/docs/puppet/5.3/type.html#service-attribute-ensure
Add the needed docs change and I will merge. |
@npwalker Moving towards a clean release on this module to allow a full rubocop run, so would love to get this in before that. Please add the docs so I can do o, otherwise a full-rebase will be needed. |
bb1a971
to
ec2c517
Compare
@david22swan okay updated. |
No description provided.