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.

Sign up for GitHub

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

Jump to bottom

Debian(-ish) split sysconfig and systemd support #156

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

Debian(-ish) split sysconfig and systemd support #156

wants to merge 11 commits into from

Conversation

vStone
Copy link
Contributor

@vStone vStone commented Nov 2, 2018 •
edited
Loading

Remove support for systems that still use one sysconfig file for both daemons.
Systemd support for debian systems.
Tests cleanup / reorder.
Reorder parameters

@vStone
Copy link
Contributor Author

vStone commented Nov 2, 2018 •
edited
Loading

This should also fix #126, #114, #125, #110, #63

@vStone vStone force-pushed the feature/split-debian branch from fa1ef17 to 12f9a59 Compare November 2, 2018 09:54
@vStone vStone changed the title Feature/split debian Debian split sysconfig and systemd support Nov 2, 2018
@vStone vStone changed the title Debian split sysconfig and systemd support Debian(-ish) split sysconfig and systemd support Nov 2, 2018
@vStone vStone force-pushed the feature/split-debian branch 2 times, most recently from 4d82eda to b4798d6 Compare November 2, 2018 10:13
bastelfreak
bastelfreak reviewed Nov 2, 2018
View reviewed changes
manifests/init.pp Outdated Show resolved Hide resolved
bastelfreak
bastelfreak reviewed Nov 2, 2018
View reviewed changes
manifests/init.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Nov 2, 2018
@bastelfreak
Copy link
Member

Hey @vStone, the PR looks pretty good! I just made some little inline comments.

@vStone
Copy link
Contributor Author

vStone commented Nov 2, 2018

Looks like I'll have to refactor the daemon options too. systemd does not like snmp*d without the -f flag

@vStone vStone force-pushed the feature/split-debian branch 2 times, most recently from 0fce9ab to a5f3a1c Compare November 2, 2018 13:49
@vStone
Copy link
Contributor Author

vStone commented Nov 2, 2018

@bastelfreak since the parameter (and alignments) were a horrible mess (imho), i reordered/re-aligned the lot of them. Made it into a separate commit so the other diffs are not too big

@ekohl
Copy link
Member

ekohl commented Nov 2, 2018

@vStone personally I'm in favor of dropping alignments. Without data types they were annoying but with data types they do more harm than good IMHO.

@vStone vStone force-pushed the feature/split-debian branch from a5f3a1c to 6857b05 Compare November 2, 2018 14:43
@vStone
Copy link
Contributor Author

vStone commented Nov 2, 2018

@vStone personally I'm in favor of dropping alignments. Without data types they were annoying but with data types they do more harm than good IMHO.

Could we postpone further changes to the alignment of the parameters to a separate pull request? :) I already feel bad for including this big alignment change in this one

@ekohl
Copy link
Member

ekohl commented Nov 2, 2018

Could we postpone further changes to the alignment of the parameters to a separate pull request? :) I already feel bad for including this big alignment change in this one

IMHO yes since I don't care about alignment so don't put too much effort into it :)

@vStone vStone force-pushed the feature/split-debian branch from 6857b05 to 41b8986 Compare November 5, 2018 08:14
@vStone vStone force-pushed the feature/split-debian branch from 41b8986 to 718efb9 Compare November 5, 2018 08:21
@vStone vStone added needs-work not ready to merge just yet and removed needs-work not ready to merge just yet labels Nov 5, 2018
@vStone
Copy link
Contributor Author

vStone commented Nov 7, 2018 •
edited
Loading

Its getting worse. Ubuntu requires the pid to be in fixed value with its mixed init/systemd scripts wheep

@vStone vStone removed the needs-work not ready to merge just yet label Nov 8, 2018
@vStone
Copy link
Contributor Author

vStone commented Nov 8, 2018

@bastelfreak @ekohl @Dan33l: I finally got this working on all debian/ubuntu flavors. I created some acceptance tests too because it was a horrible nightmare to figure out how sh*t works on each os. Please re-review ;)

ekohl
ekohl reviewed Nov 8, 2018
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

We usually also run beaker tests on Travis if we can. Think we can do so here as well?

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/params.pp Outdated Show resolved Hide resolved
spec/acceptance/nodesets/docker-debian-8-systemd.yml Outdated Show resolved Hide resolved
spec/acceptance/support/shared_examples_processes.rb Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
@vStone vStone force-pushed the feature/split-debian branch from 2e91cf1 to 7d1a32f Compare November 9, 2018 08:24
@vStone
Copy link
Contributor Author

vStone commented Nov 9, 2018

I removed the acceptance test things and I'll make a pr for them later on. Need to figure out the beaker hostgenerator part :) other remarks should be fixed.

@vStone vStone force-pushed the feature/split-debian branch from 7d1a32f to 09d714e Compare November 9, 2018 08:28
@vStone
Add more data types to parameters
9c921a4
@vStone
Stop supporting old versions of debian-ish systems with a combined pa…
693a08a 
…ckage

Ubuntu 14.04 is the only os left that uses the weird single
/etc/default/snmpd config to control wether or not to start
snmpd and/or snmptrapd with *RUN variables. By removing support
for this abomination, the module logic can be greatly reduced.

On a side note: Ubuntu 14.04 will be EOL 2019/04.
@vStone
All supported debian versions have a split-out snmptrapd package
c034d43
@vStone
Debian split snmpd/snmptrapd sysconfig support
2f24f8f 
snmpd and snmptrapd now each have their own package and own config file
in /etc/default.
@vStone
Support systemd on the debian family
bb4cf15 
Most systemd systems allow using a sysconfig style environments file,
but not debian. If we do not want to adjust daemon options, we need to use
an systemd drop-in service file.
@vStone
Re-order and adjust debian tests
8fdc95b 
Remove and re-order the old tests for the combined sysconfig file.
Add tests for the systemd configuration.
@vStone
Fix spacing (or scratching that itch)
539f37a
@vStone
Reorder and realign all parameters
7551a86 
Attempt to group parameters that belong together.
Add data types to a couple of parameters (Booleans)
@vStone
Fix ubuntu 16.04 mixed systemd/sysvinit style configuration
82164e2
@vStone vStone force-pushed the feature/split-debian branch from 09d714e to 82164e2 Compare November 12, 2018 05:39
@vStone vStone mentioned this pull request Nov 22, 2018
Merged
ghoneycutt
ghoneycutt reviewed Jun 13, 2019
View reviewed changes
manifests/init.pp
@@ -139,6 +139,11 @@
# @param package_name
# Name of the package. Only set this if your platform is not supported or you know what you are doing.
#
# @param snmptrapd_package_name
# Name of the snmptrapd package, if there is one.
# Only set thi sif your platform is not supported or you know what you are doing.
Copy link
Member

Choose a reason for hiding this comment

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

typo /thi sif/this if

@vox-pupuli-tasks
Copy link

Dear @vStone, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

1 similar comment
@vox-pupuli-tasks
Copy link

Dear @vStone, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @vStone, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @vStone, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @vStone, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

jonasdemoor and others added 2 commits March 16, 2021 13:16
@jonasdemoor
Drop systemd daemon-reload for snmp services
ee27e00 
This is not needed anymore since Puppet 6.

voxpupuli/puppet-systemd#171
@vStone
Merge pull request #1 from jonasdemoor/feature/split-debian
f068cac 
Drop systemd daemon-reload for snmp services
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@ghoneycutt ghoneycutt ghoneycutt left review comments

@ekohl ekohl ekohl left review comments

Assignees
No one assigned
Labels
enhancement New feature or request merge-conflicts tests-fail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vStone @bastelfreak @ekohl @ghoneycutt @dhoppe @Dan33l @jonasdemoor