Merge ~szeestraten/charm-nagios:bug/1864968 into ~nagios-charmers/charm-nagios:master

Proposed by Sandor Zeestraten
Status: Merged
Merged at revision: 73961bbffd6735574cc63a525a96ad26c58d0e38
Proposed branch: ~szeestraten/charm-nagios:bug/1864968
Merge into: ~nagios-charmers/charm-nagios:master
Diff against target: 112 lines (+75/-7)
3 files modified
config.yaml (+62/-0)
hooks/templates/contacts-cfg.tmpl (+6/-6)
hooks/upgrade-charm (+7/-1)
Reviewer Review Type Date Requested Status
Alvaro Uria Approve
Xav Paice Needs Fixing
Review via email: mp+379943@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Xav Paice (xavpaice) wrote :

Many thanks for this patch.

I've made a few comments regarding the config options in config.yaml, around documentation. The change itself seems good, and has tested OK via a manual deployment test.

review: Needs Fixing
Revision history for this message
Sandor Zeestraten (szeestraten) wrote :

Thanks for the review Xav!

I prepended `_admin` and added comment in description to clarify it is for the default admin contact.

I considered adding similar config options for the pagerduty, but unfortunately it seems that the config option name `pagerduty_notification_levels` was chosen for the `service_notification_options` which breaks the naming convention and changing it for existing deployments is probably not the scope for this bug/patch.

Also commented that the notification_commands is for changing or adding custom notification plugins which is the intended use case for those commands.

Revision history for this message
Xav Paice (xavpaice) wrote :

There's some merge conflict markers in hooks/upgrade-charm which need sorting, and likely a rebase against master to ensure there's no further conflicts.

Other than that, lgtm.

review: Needs Fixing
Revision history for this message
Sandor Zeestraten (szeestraten) wrote :

Resolved merge conflict and squashed commits.

Revision history for this message
Alvaro Uria (aluria) wrote :

Thank you Sandor for your change. I'm going to approve it and merge.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/config.yaml b/config.yaml
2index a5dd578..e4755ad 100644
3--- a/config.yaml
4+++ b/config.yaml
5@@ -222,3 +222,65 @@ options:
6 u - Unknown
7 w - Warning
8 o - OK
9+ admin_service_notification_period:
10+ default: "24x7"
11+ type: string
12+ description: |
13+ This directive is used to specify the short name of the time period during
14+ which the default admin contact can be notified about service problems or recoveries.
15+ You can think of this as an "on call" time for service notifications
16+ for the contact. Read the documentation on time periods for more information
17+ on how this works and potential problems that may result from improper use.
18+ admin_host_notification_period:
19+ default: "24x7"
20+ type: string
21+ description: |
22+ This directive is used to specify the short name of the time period during
23+ which the default admin contact can be notified about host problems or recoveries.
24+ You can think of this as an "on call" time for host notifications
25+ for the contact. Read the documentation on time periods for more information
26+ on how this works and potential problems that may result from improper use.
27+ admin_service_notification_options:
28+ default: "w,u,c,r"
29+ type: string
30+ description: |
31+ This directive is used to define the service states for which notifications
32+ can be sent out to the default admin contact. Valid options are a combination of one
33+ or more of the following:
34+ w = notify on WARNING service states,
35+ u = notify on UNKNOWN service states,
36+ c = notify on CRITICAL service states,
37+ r = notify on service recoveries (OK states),
38+ f = notify when the service starts and stops flapping.
39+ If you specify n (none) as an option, the contact will not receive any type of service notifications.
40+ admin_host_notification_options:
41+ default: "d,r"
42+ type: string
43+ description: |
44+ This directive is used to define the host states for which notifications
45+ can be sent out to the default admin contact. Valid options are a combination of one
46+ or more of the following:
47+ d = notify on DOWN host states,
48+ u = notify on UNREACHABLE host states,
49+ r = notify on host recoveries (UP states),
50+ f = notify when the host starts and stops flapping,
51+ s = send notifications when host or service scheduled downtime starts and ends.
52+ If you specify n (none) as an option, the contact will not receive any type of host notifications.
53+ admin_service_notification_commands:
54+ default: "notify-service-by-email"
55+ type: string
56+ description: |
57+ This directive is used to define a list of the short names of the commands
58+ used to notify the default admin contact of a service problem or recovery.
59+ Change this or add commands if you have custom notification plugins.
60+ Multiple notification commands should be separated by commas.
61+ All notification commands are executed when the contact needs to be notified.
62+ admin_host_notification_commands:
63+ default: "notify-host-by-email"
64+ type: string
65+ description: |
66+ This directive is used to define a list of the short names of the commands
67+ used to notify the default admin contact of a host problem or recovery.
68+ Change this or add commands if you have custom notification plugins.
69+ Multiple notification commands should be separated by commas.
70+ All notification commands are executed when the contact needs to be notified.
71diff --git a/hooks/templates/contacts-cfg.tmpl b/hooks/templates/contacts-cfg.tmpl
72index 34cf6ff..8f28acf 100644
73--- a/hooks/templates/contacts-cfg.tmpl
74+++ b/hooks/templates/contacts-cfg.tmpl
75@@ -21,12 +21,12 @@
76 define contact{
77 contact_name root
78 alias Root
79- service_notification_period 24x7
80- host_notification_period 24x7
81- service_notification_options w,u,c,r
82- host_notification_options d,r
83- service_notification_commands notify-service-by-email
84- host_notification_commands notify-host-by-email
85+ service_notification_period {{ admin_service_notification_period }}
86+ host_notification_period {{ admin_host_notification_period }}
87+ service_notification_options {{ admin_service_notification_options }}
88+ host_notification_options {{ admin_host_notification_options }}
89+ service_notification_commands {{ admin_service_notification_commands }}
90+ host_notification_commands {{ admin_host_notification_commands }}
91 email {{ admin_email }}
92 }
93
94diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
95index 167225c..a80743b 100755
96--- a/hooks/upgrade-charm
97+++ b/hooks/upgrade-charm
98@@ -167,7 +167,13 @@ def enable_pagerduty_config():
99 if "pagerduty" not in contactgroup_members:
100 contactgroup_members += ", pagerduty"
101
102- template_values = {'admin_email': hookenv.config('admin_email'),
103+ template_values = {'admin_service_notification_period': hookenv.config('admin_service_notification_period'),
104+ 'admin_host_notification_period': hookenv.config('admin_host_notification_period'),
105+ 'admin_service_notification_options': hookenv.config('admin_service_notification_options'),
106+ 'admin_host_notification_options': hookenv.config('admin_host_notification_options'),
107+ 'admin_service_notification_commands': hookenv.config('admin_service_notification_commands'),
108+ 'admin_host_notification_commands': hookenv.config('admin_host_notification_commands'),
109+ 'admin_email': hookenv.config('admin_email'),
110 'contactgroup_members': contactgroup_members}
111
112 with open('hooks/templates/contacts-cfg.tmpl', 'r') as f:

Subscribers

People subscribed via source and target branches