Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code
Issues
All Issues

Issue 29587724: #4612 - Enable External Commands in Nagios (Closed)

Created:
Oct. 24, 2017, 9 p.m. by f.nicolaisen
Modified:
Oct. 26, 2017, 7:47 a.m.
Reviewers:
mathias
CC:
f.lopez
Base URL:
https://hg1/infrastructure
Visibility:
Public.

Description

#4612 - Enable External Commands in Nagios

Patch Set 1 #

Total comments: 4

Patch Set 2 : nicer dpkg_statoverride #

Total comments: 2

Patch Set 3 : address acks from #6 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -1 line) Patch
M modules/nagios/files/nagios.cfg View 1 chunk +1 line, -1 line 0 comments Download
M modules/nagios/manifests/server.pp View 1 2 2 chunks +27 lines, -0 lines 2 comments Download

Messages

Total messages: 9
f.nicolaisen
Oct. 24, 2017, 9 p.m. (2017-10-24 21:00:03 UTC) #1
f.nicolaisen
https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp#newcode71 modules/nagios/manifests/server.pp:71: ], Hm, I accidentally included this change. I was ...
Oct. 25, 2017, 10:01 a.m. (2017-10-25 10:01:13 UTC) #2
mathias
https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp#newcode56 modules/nagios/manifests/server.pp:56: command => 'dpkg-statoverride --update --add nagios www-data 2710 /var/lib/nagios3/rw ...
Oct. 25, 2017, 10:31 a.m. (2017-10-25 10:31:48 UTC) #3
f.nicolaisen
PS2 addresses the comments so far. https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29587725/modules/nagios/manifests/server.pp#newcode56 modules/nagios/manifests/server.pp:56: command => 'dpkg-statoverride ...
Oct. 25, 2017, 2:36 p.m. (2017-10-25 14:36:09 UTC) #4
mathias
https://codereview.adblockplus.org/29587724/diff/29588952/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29588952/modules/nagios/manifests/server.pp#newcode55 modules/nagios/manifests/server.pp:55: if $::operatingsystem == 'Debian' { The condition should use ...
Oct. 25, 2017, 2:40 p.m. (2017-10-25 14:40:55 UTC) #5
f.nicolaisen
https://codereview.adblockplus.org/29587724/diff/29588952/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29588952/modules/nagios/manifests/server.pp#newcode55 modules/nagios/manifests/server.pp:55: if $::operatingsystem == 'Debian' { On 2017/10/25 14:40:55, mathias ...
Oct. 25, 2017, 3:10 p.m. (2017-10-25 15:10:29 UTC) #6
f.nicolaisen
PS3 addresses the latest review.
Oct. 25, 2017, 3:33 p.m. (2017-10-25 15:33:24 UTC) #7
mathias
LGTM. https://codereview.adblockplus.org/29587724/diff/29588955/modules/nagios/manifests/server.pp File modules/nagios/manifests/server.pp (right): https://codereview.adblockplus.org/29587724/diff/29588955/modules/nagios/manifests/server.pp#newcode2 modules/nagios/manifests/server.pp:2: $directory = hiera('nagios::server::directory', '/var/lib/nagios3'), This technique is not ...
Oct. 25, 2017, 4:53 p.m. (2017-10-25 16:53:42 UTC) #8
f.nicolaisen
Oct. 26, 2017, 7:45 a.m. (2017-10-26 07:45:05 UTC) #9 
https://codereview.adblockplus.org/29587724/diff/29588955/modules/nagios/mani...
File modules/nagios/manifests/server.pp (right):

https://codereview.adblockplus.org/29587724/diff/29588955/modules/nagios/mani...
modules/nagios/manifests/server.pp:2: $directory =
hiera('nagios::server::directory', '/var/lib/nagios3'),
On 2017/10/25 16:53:42, mathias wrote:
> This technique is not necessary any more, we've been using it to emulate
Puppet
> 3 behavior in Puppet 2. Today you can assign the default values directly, and
> Puppet will worry about the Hiera lookup.

Noted for next time!

Powered by Google App Engine
This is Rietveld