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 (community) Approve
Xav Paice (community) 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
diff --git a/config.yaml b/config.yaml
index a5dd578..e4755ad 100644
--- a/config.yaml
+++ b/config.yaml
@@ -222,3 +222,65 @@ options:
222 u - Unknown222 u - Unknown
223 w - Warning223 w - Warning
224 o - OK224 o - OK
225 admin_service_notification_period:
226 default: "24x7"
227 type: string
228 description: |
229 This directive is used to specify the short name of the time period during
230 which the default admin contact can be notified about service problems or recoveries.
231 You can think of this as an "on call" time for service notifications
232 for the contact. Read the documentation on time periods for more information
233 on how this works and potential problems that may result from improper use.
234 admin_host_notification_period:
235 default: "24x7"
236 type: string
237 description: |
238 This directive is used to specify the short name of the time period during
239 which the default admin contact can be notified about host problems or recoveries.
240 You can think of this as an "on call" time for host notifications
241 for the contact. Read the documentation on time periods for more information
242 on how this works and potential problems that may result from improper use.
243 admin_service_notification_options:
244 default: "w,u,c,r"
245 type: string
246 description: |
247 This directive is used to define the service states for which notifications
248 can be sent out to the default admin contact. Valid options are a combination of one
249 or more of the following:
250 w = notify on WARNING service states,
251 u = notify on UNKNOWN service states,
252 c = notify on CRITICAL service states,
253 r = notify on service recoveries (OK states),
254 f = notify when the service starts and stops flapping.
255 If you specify n (none) as an option, the contact will not receive any type of service notifications.
256 admin_host_notification_options:
257 default: "d,r"
258 type: string
259 description: |
260 This directive is used to define the host states for which notifications
261 can be sent out to the default admin contact. Valid options are a combination of one
262 or more of the following:
263 d = notify on DOWN host states,
264 u = notify on UNREACHABLE host states,
265 r = notify on host recoveries (UP states),
266 f = notify when the host starts and stops flapping,
267 s = send notifications when host or service scheduled downtime starts and ends.
268 If you specify n (none) as an option, the contact will not receive any type of host notifications.
269 admin_service_notification_commands:
270 default: "notify-service-by-email"
271 type: string
272 description: |
273 This directive is used to define a list of the short names of the commands
274 used to notify the default admin contact of a service problem or recovery.
275 Change this or add commands if you have custom notification plugins.
276 Multiple notification commands should be separated by commas.
277 All notification commands are executed when the contact needs to be notified.
278 admin_host_notification_commands:
279 default: "notify-host-by-email"
280 type: string
281 description: |
282 This directive is used to define a list of the short names of the commands
283 used to notify the default admin contact of a host problem or recovery.
284 Change this or add commands if you have custom notification plugins.
285 Multiple notification commands should be separated by commas.
286 All notification commands are executed when the contact needs to be notified.
diff --git a/hooks/templates/contacts-cfg.tmpl b/hooks/templates/contacts-cfg.tmpl
index 34cf6ff..8f28acf 100644
--- a/hooks/templates/contacts-cfg.tmpl
+++ b/hooks/templates/contacts-cfg.tmpl
@@ -21,12 +21,12 @@
21define contact{21define contact{
22 contact_name root22 contact_name root
23 alias Root23 alias Root
24 service_notification_period 24x724 service_notification_period {{ admin_service_notification_period }}
25 host_notification_period 24x725 host_notification_period {{ admin_host_notification_period }}
26 service_notification_options w,u,c,r26 service_notification_options {{ admin_service_notification_options }}
27 host_notification_options d,r27 host_notification_options {{ admin_host_notification_options }}
28 service_notification_commands notify-service-by-email28 service_notification_commands {{ admin_service_notification_commands }}
29 host_notification_commands notify-host-by-email29 host_notification_commands {{ admin_host_notification_commands }}
30 email {{ admin_email }}30 email {{ admin_email }}
31 }31 }
3232
diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
index 167225c..a80743b 100755
--- a/hooks/upgrade-charm
+++ b/hooks/upgrade-charm
@@ -167,7 +167,13 @@ def enable_pagerduty_config():
167 if "pagerduty" not in contactgroup_members:167 if "pagerduty" not in contactgroup_members:
168 contactgroup_members += ", pagerduty"168 contactgroup_members += ", pagerduty"
169169
170 template_values = {'admin_email': hookenv.config('admin_email'),170 template_values = {'admin_service_notification_period': hookenv.config('admin_service_notification_period'),
171 'admin_host_notification_period': hookenv.config('admin_host_notification_period'),
172 'admin_service_notification_options': hookenv.config('admin_service_notification_options'),
173 'admin_host_notification_options': hookenv.config('admin_host_notification_options'),
174 'admin_service_notification_commands': hookenv.config('admin_service_notification_commands'),
175 'admin_host_notification_commands': hookenv.config('admin_host_notification_commands'),
176 'admin_email': hookenv.config('admin_email'),
171 'contactgroup_members': contactgroup_members}177 'contactgroup_members': contactgroup_members}
172178
173 with open('hooks/templates/contacts-cfg.tmpl', 'r') as f:179 with open('hooks/templates/contacts-cfg.tmpl', 'r') as f:

Subscribers

People subscribed via source and target branches