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

Add support for Debian #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for Debian #46

wants to merge 3 commits into from

Conversation

davidc
Copy link

@davidc davidc commented Aug 15, 2022

Pull Request (PR) description

  • add Debian support
  • control_cmd was not used, now it is
  • add control_setup_cmd as Puppet now requires it to be an absolute path (I put in commands without paths for FreeBSD/OpenBSD that will need to be updated in relevant Puppet versions - but control_cmd needs to be updated for them anyway, so this is not worse than the current situation)

This Pull Request (PR) fixes the following issues

control_cmd was not used, now it is
add control_setup_cmd as Puppet now requires it to be an absolute path (I put in commands without paths for FreeBSD/OpenBSD that will need to be updated)
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Thanks! Better stick to one PR per issue for your future work: this allows more readable changelog (we generate them with the PR titles).

A few things to check: I added in-line comments.

nsd::service_name: 'nsd'
nsd::owner: 'nsd'
nsd::group: 'nsd'
nsd::control_cmd: '/usr/bin/echo /usr/sbin/nsd-control'
Copy link
Member

Choose a reason for hiding this comment

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

echo? Really?

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
nsd::control_cmd: '/usr/bin/echo /usr/sbin/nsd-control'
nsd::control_cmd: '/usr/sbin/nsd-control'

@@ -8,6 +8,7 @@
String $service_name,
Variant[String,Undef] $package_name,
String $control_cmd,
String $control_setup_cmd,
Copy link
Member

Choose a reason for hiding this comment

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

If an empty string is not valid (I guess it is not), better use:

Suggested change
String $control_setup_cmd,
String[1] $control_setup_cmd,

We can also use the type form stdlib if the full path is required:

Suggested change
String $control_setup_cmd,
Stdlib::Absolutepath $control_setup_cmd,

@smortex smortex changed the title add Debian support, add control_setup_cmd, actually use control_cmd Add support for Debian Aug 17, 2022
@smortex smortex added the enhancement New feature or request label Aug 17, 2022
@smortex
Copy link
Member

smortex commented Aug 17, 2022

Oh, and if it works on Debian, we can add it to metadata.json 😉

davidc and others added 2 commits August 17, 2022 12:21
Co-authored-by: Romain Tartière <[email protected]>
Co-authored-by: Romain Tartière <[email protected]>
@ekohl
Copy link
Member

ekohl commented Dec 21, 2022

@davidc any plans to revisit this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants