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

Gentoo service fix #545

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Gentoo service fix #545

merged 2 commits into from
Oct 17, 2018

Conversation

lordievader
Copy link
Contributor

Pull Request (PR) description

This PR continues the Gentoo support of the puppet-zabbix module.
It fixes a few forgotten changes from zabbix-agent to $servicename. Which is sourced from params.pp.

In the second commit a copy is made of zabbix-agent-systemd.init.erb and named as zabbix-agentd-systemd.init.erb. I'm not sure if this is the wanted procedure, hence I left it as a separate commit. If this is not the correct procedure feel free to reject it ;)

The last commit adds $servicename like support to userparameters.pp.

@Dan33l
Copy link
Member

Dan33l commented Sep 27, 2018

Hey @lordievader , thank you for the PR.

Accordingly to our reviewing policy new template are prefered with epp and not with erb.
https://voxpupuli.org/docs/#reviewing-a-module-pr

And you need to fix tests, some of them failed.

@juniorsysadmin juniorsysadmin added the enhancement New feature or request label Sep 29, 2018
@baurmatt
Copy link
Contributor

baurmatt commented Oct 4, 2018

The file templates/zabbix-agentd-systemd.init.erb shouldn't be needed due it's content being exactly the same as zabbix-agent-systemd.init.erb

@Dan33l
Copy link
Member

Dan33l commented Oct 5, 2018

The file templates/zabbix-agentd-systemd.init.erb shouldn't be needed due it's content being exactly the same as zabbix-agent-systemd.init.erb

So can you modify your PR to remove its use and fix tests ?

@lordievader
Copy link
Contributor Author

@baurmatt but via commit 007b527 the module tries to find a template named 'zabbix-agentd-systemd.init'. This is the reason why I duplicated the service file.

@Dan33l I will try to fix the tests when I have time. Have been quite busy with other things the past few weeks, sorry.

@baurmatt
Copy link
Contributor

baurmatt commented Oct 9, 2018

@lordievader I'm aware :) But it's definitely not good style to just copy the file. This makes changes and maintenance way more complex. It could be enough to just add a parameter service_name to zabbix::startup and change this for each OS.

@lordievader
Copy link
Contributor Author

Oh, that does sound like a nicer fix. I'll implement that :)

@bastelfreak
Copy link
Member

@lordievader can you rebase against our master and take a look at the failing spec tests?

Since the Gentoo zabbix agent service is named different, it should be
reflected in the userparameters.pp too.
In the previous Gentoo fixes I have to use the params.pp zabbix-agent service
name it seems.

Source service name from params.pp
@lordievader
Copy link
Contributor Author

@bastelfreak After quite a few attemps, I fixed the failing tests. As Gentoo doesn't seem part of the unit tests (I didn't see an easy way of adding the OS), I tested that part on my own system.

@juniorsysadmin juniorsysadmin mentioned this pull request Oct 15, 2018
@bastelfreak
Copy link
Member

Thanks!

@bastelfreak bastelfreak merged commit d3dd9fa into voxpupuli:master Oct 17, 2018
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.

5 participants