Reduce configuration on agent service #711
Conversation
2693b2e to
afaf24c
Compare
|
@bjschafer you added AIX support in #665. Could you verify I don't break anything with this? |
afaf24c to
e575e25
Compare
manifests/agent.pp
Outdated
| # Override the service provider on AIX | ||
| # Doing it this way allows overriding it on other platforms | ||
| if $facts['os']['name'] == 'AIX' { | ||
| Service[$servicename] { service_provider => 'init' } |
There was a problem hiding this comment.
given this documentation: https://puppet.com/docs/puppet/6.18/types/service.html#service-provider-init
true == begin
os = Facter.value(:operatingsystem).downcase
family = Facter.value(:osfamily).downcase
!(os == 'debian' || os == 'ubuntu' || family == 'redhat')
endi'm sceptical that puppet will know the correct $service_path, so maybe let's bring this back.
There was a problem hiding this comment.
I still dont get why we need to set the provider explicitly :(
There was a problem hiding this comment.
that's why I'm hoping @bjschafer can answer that.
There was a problem hiding this comment.
because the default service provider is src, but the zabbix package, i assume, provides an init style service file
There was a problem hiding this comment.
I did see that, but I don't know if src can run init style service files.
021fae4 to
c473b30
Compare
|
do we have anything to fix the test failures? https://travis-ci.org/github/voxpupuli/puppet-zabbix/jobs/727040802 |
This avoids setting parameters as much as possible. hasstatus and hasrestart have defaulted to true since Puppet 2.7 so there's no need to set those. The systemd fact is defined as $fact['service_provider'] == 'systemd' so there's no need to set that either. Then there's AIX left. It sets it this was because it allows users to override it in site.pp via service defaults. It now only sets the service_provider and not the path. This path is set in the provider since Puppet 4.6.0[1]. puppetlabs/puppet@34e45ab
c473b30 to
73e6da0
Compare
|
That was bad code from my side. I tried to properly use |
| end | ||
|
|
||
| it { is_expected.to contain_file(include_dir).with_ensure('directory') } | ||
| it { is_expected.to contain_zabbix__startup(service_name).with(require: "Package[#{package_name}]") } |
There was a problem hiding this comment.
This with_require style happens more often in this module. I just fixed it for this file
There was a problem hiding this comment.
let's open an issue as reminder to do it in a next iteration
There was a problem hiding this comment.
Consider this code:
class myclass {
package { 'mypackage':
}
file { '/etc/mypackage.conf':
require => Package['mypackage'],
}
service { 'mypackage':
subscribe => File['/etc/mypackage.conf'],
}
}Then the test code:
it { is_expected.not_to contain_service('mypackage').with_require('Package[mypackage]') }
it { is_expected.to contain_service('mypackage').that_requires('Package[mypackage]') }What I'm trying to show is that one tests for a parameter on a resource and the other the relationship exists in the catalog. That takes transitive requirements into account.
|
can we merge this? |
This avoids setting parameters as much as possible.
hasstatus and hasrestart have defaulted to true since Puppet 2.7 so there's no need to set those.
The systemd fact is defined as $fact['service_provider'] == 'systemd' so there's no need to set that either.
Then there's AIX left. It sets it this was because it allows users to override it in site.pp via service defaults. It now only sets the service_provider and not the path. This path is set in the provider since Puppet 4.6.0.
I didn't verify this and only did it by theory so review carefully.