add multiple execstarts by converting $execstart to array,

[Dwarfex]

and check if $type is "oneshot"
change some settings to default values.

[jordiprats]

Hi,
Thanks fot you contribution, but I cannot merge it

• multiple ExecStart sounds good

but:

• umask is valid parameter
• default values cannot be changed for compatibility reasons

regards,

[Dwarfex]

I'll check both your points and will come back to it :-) - the third commit was because I added an additional umask parameter - i overlooked that it was already implemented - so umask is still part of the PR - for the default values: i looked them up on the documentation because i had some issues regarding the default behaviour of the services and noticed that the defaults used in this lib differ from the systemd defaults if i should remove the changing of the default values for you to accept the PR, i would do so - but hope you would add them in a new major version. These are the defaults of systemd: Restart= the valid default for "restart" is "no" (https://www.freedesktop.org/software/systemd/man/systemd.service.html#Restart=) WantedBy= is part of the [Install] Section wich - per definits is completely optional and shouldn't have values set as default (https://www.freedesktop.org/software/systemd/man/systemd.unit.html#%5BInstall%5D%20Section%20Options) StandardOutput= , StandardError= The default values of systemd are "inherit" for StandardError and "journal" for StandardOutput (https://www.freedesktop.org/software/systemd/man/systemd.exec.html#StandardOutput= https://www.freedesktop.org/software/systemd/man/systemd.exec.html#StandardError= )

[func0der]

Hey! Great thought. I would have done it myself, but I had no time.

The new major thing sounds great.
I think, if you would bump this to 0.2.0, this would be totally okay.
Also, if there is no major version of a plugin (every version < 1), breaking changes are most likely and expected.
No major version means that this is not a stable version, which is equal to an alpha or beta version.

For further information about versioning for puppet modules, you can have a read here:

https://docs.puppet.com/puppet/latest/bgtm.html#versioning-your-module
or here
http://semver.org/spec/v1.0.0.html
which describes the situation above with this paragraph/rule.

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Thanks for you awesome plugin and your development, by the way. It really helps me out at this point :)

---

I forgot about the metadata.json, in which people can also freeze their versions of the module until the have adapted the code. ;)

[jordiprats]

Hi,
I've merged this change partially in this commit:

I think here we are intending to provide "safe defaults" instead of just use systemd's defaults. I don't want to force people to add "wantedby => [ 'multi-user.target' ]," specially since this was the default in this module since day 1. I've updated the documentation

I'm closing this PR since it's partially merged

thanks,

[func0der]

hey. Thanks for merging, even though it was only partially.
I would have been nice, if you would have checked backed with the PR requester first, to make sure, you are both satisfied with the solution. I know, you do not have to, because this is your module, but it is just kind of nicer.

To you comment:

I think here we are intending to provide "safe defaults" instead of just use systemd's defaults

Yes and no. If I use a module, I do not want it to make a configuration that is any different from the defaults that the tool I am using, defines. Great that you mentioned it in the documentation, though. Now people will know about it, if they read the docs.
BUT: consider the following. I do NOT want to have wantedby in my service configuration and explicitly set it to 'undef' or whatever. Your plugin forces this directive into the service file in the current configuration as it also does with the [install] section.
I find this part of the PR (0a6e8b9#diff-2f31c46d567c1cc049875d41d88a144fR134) escpecially useful in such cases. It would be nice, if you'd reconsider merging it.

I'd would also appreciated if you reconsider this structure in your commit: 4bd7f3d#diff-2f31c46d567c1cc049875d41d88a144fR26

You are pressing extensive logic into a TEMPLATE file here. A template is a mere string in which you should only search and replace some variables. It that includes a loop, it is okay, but it should not include extensive conditional structures.
The type casting solution in the PR is a great solution and it happening where it is supposed to happen.
Also you supply us with functions like any2array, so, why not use them?

Regarding these two changes: https://github.com/NTTCom-MS/eyp-systemd/pull/32/files#diff-2f31c46d567c1cc049875d41d88a144fL42

You did not even address them in your commit. Do not hard code anything in the template, please. You are taking away the change for the users of your plugin, to modify it at will.
You can put the values for those into the parameters for your systemd::server class.

I hope you reconsider these changes.

specially since this was the default in this module since day 1

To this I can only point back to my last comment, in which I have linked to the versioning page. It includes the rules every puppet module should follow, as stated by the puppet labs guidelines.
Your module can behave differently at any time, especially in the development phase (version < 1). Later also, with major updates, that come with breaking changes.

Change in general is not a bad thing. We all fear them sometimes, but I we look at them subjectively, they might be more practical and useful that we first thought. Or maybe they are not.
Things like it always has been this way, so we do not change it, are not the best approach ;)

I know, that there is a pretty good chance of pushing the needs of oneself into such modules.
But I think, if you'd leave those aside, your module could reach a much broader crowd.

TL;DR: Read the above :P No shortcuts.