Change - Update requirements for the snmp::client class #98
Conversation
manifests/client.pp
Outdated
| } | ||
|
|
||
| $req = $::osfamily ? { | ||
| 'RedHat' => [Package[$snmp::params::package_name], Package['snmp-client']], |
There was a problem hiding this comment.
Package[$snmp::params::package_name] will work so long as no one does include ::snmp::client without include ::snmp (which is completely valid).
There was a problem hiding this comment.
snmp::client inherits snmp::params so it shouldn't be necessary to include ::snmp at all.
spec/classes/snmp_client_spec.rb
Outdated
| )} | ||
|
|
||
| it { should contain_package('snmp-client').that_comes_before('File[snmp.conf]') } | ||
|
|
There was a problem hiding this comment.
This removes some testing functionality. It may be better to just add to the :require below.
There was a problem hiding this comment.
Thanks. I wanted to use $snmp::params::package_name because the package name may change in future RedHat releases but I could also just set it statically to 'net-snmp'.
I also tried updating the :require => line but that causes the automated tests to fail. If there's a way to make the spec file accept an array value then I can fix the tests to make it work.
To avoid missing directory errors on Redhat based systems the net-snmp package needs to be installed before the snmp.conf file is created.
|
I've rebased on the develop branch and all tests are passing now. |
|
@blackknight36 Will be merging this week once I page back in previous state. |
| } | ||
|
|
||
| $req = $::osfamily ? { | ||
| 'RedHat' => [Package['snmp-client'], Package[$snmp::params::package_name]], |
There was a problem hiding this comment.
I think you can reference this as Package['snmpd'] since it already contains the logic that you're replicating
There was a problem hiding this comment.
@elisiano I considered that but I wanted to avoid hard coding the package name since it may change in the future and it's a lot easier update everything in params.pp (or hiera) when needed. IMO the first requirement should also be changed to Package[$snmp::params::client_package however I wanted to make as few changes as possible with this commit.
There was a problem hiding this comment.
it's not hardcoded. 'snmpd' is just the name of the resource, the real package name given with the name parameter:
package { 'snmpd':
ensure => $package_ensure,
name => $package_name,
}
There was a problem hiding this comment.
Ah, I see. I guess you could submit a PR to fix it.
There was a problem hiding this comment.
Package[$snmp::params::client_package_name] is not defined and should throw an error.
Package['snmpd'] is not defined in Class['::snmp::client'] and cannot be depended upon to exist.
Package['snmp-client'] is the only one directly declared in Class['::snmp::client']. Which makes me wonder if I am completely wrong or if the code won't work. @blackknight36, have you tested this on real systems with only class {'::snmp::client'} ?
There was a problem hiding this comment.
@razorsedge Looks like you are correct. I've made a couple changes and submitted a new PR which should fix these issues. In the end it may be simpler to just create a file resource that ensures /etc/snmp exists. Installing the net-snmp package on systems that don't really need it is a bit of a waste.
|
Disclaimer: I did not test this patch. |
|
I did some testing today and there were errors when a node only includes snmp::client and not the main snmp class. The snmpd package must be defined or the catalog will fail. |
To avoid missing directory errors on Redhat based systems the net-snmp
package needs to be installed before the snmp.conf file is created.