nagios-charmers team mailing list archive

[Xav Paice <xav.paice@xxxxxxxxxxxxx>]

Review: Needs Fixing

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.

Diff comments:

> diff --git a/config.yaml b/config.yaml
> index a5dd578..5920142 100644
> --- a/config.yaml
> +++ b/config.yaml
> @@ -222,3 +222,67 @@ options:
>              u - Unknown
>              w - Warning
>              o - OK
> +    service_notification_period:
> +        default: "24x7"
> +        type: string
> +        description: |
> +            This directive is used to specify the short name of the time period during
> +            which the contact can be notified about service problems or recoveries.
> +            You can think of this as an "on call" time for service notifications
> +            for the contact.  Read the documentation on time periods for more information

For this, and similar references below, it would be helpful to be specific about which contact this applies to - pagerduty, admin_email, etc.

> +            on how this works and potential problems that may result from improper use.
> +    host_notification_period:
> +        default: "24x7"
> +        type: string
> +        description: |
> +            This directive is used to specify the short name of the time period during
> +            which the contact can be notified about host problems or recoveries.
> +            You can think of this as an "on call" time for host notifications
> +            for the contact.  Read the documentation on time periods for more information
> +            on how this works and potential problems that may result from improper use.
> +    service_notification_options:
> +        default: "w,u,c,r"
> +        type: string
> +        description: |
> +            This directive is used to define the service states for which notifications
> +            can be sent out to this contact.  Valid options are a combination of one
> +            or more of the following:
> +            w = notify on WARNING service states,
> +            u = notify on UNKNOWN service states,
> +            c = notify on CRITICAL service states,
> +            r = notify on service recoveries (OK states),
> +            f = notify when the service starts and stops flapping.
> +            If you specify n (none) as an option, the contact will not receive any type of service notifications.
> +    host_notification_options:
> +        default: "d,r"
> +        type: string
> +        description: |
> +            This directive is used to define the host states for which notifications
> +            can be sent out to this contact.  Valid options are a combination of one
> +            or more of the following:
> +            d = notify on DOWN host states,
> +            u = notify on UNREACHABLE host states,
> +            r = notify on host recoveries (UP states),
> +            f = notify when the host starts and stops flapping,
> +            s = send notifications when host or service scheduled downtime starts and ends.
> +            If you specify n (none) as an option, the contact will not receive any type of host notifications.
> +    service_notification_commands:

For this, do you suggest using the extraconfig option to define alternate commands?  If so, it would be helpful to mention that in the documentation here, since it's not immediately obvious how to use this option.

> +        default: "notify-service-by-email"
> +        type: string
> +        description: |
> +            This directive is used to define a list of the short names of the commands
> +            used to notify the contact of a service problem or recovery.
> +            Multiple notification commands should be separated by commas.
> +            All notification commands are executed when the contact needs to be notified.
> +            The maximum amount of time that a notification command can run is controlled
> +            by the notification_timeout option.

notification_timeout isn't a charm option

> +    host_notification_commands:

same comments as for service_notification_commands

> +        default: "notify-host-by-email"
> +        type: string
> +        description: |
> +            This directive is used to define a list of the short names of the commands
> +            used to notify the contact of a host problem or recovery.
> +            Multiple notification commands should be separated by commas.
> +            All notification commands are executed when the contact needs to be notified.
> +            The maximum amount of time that a notification command can run is controlled
> +            by the notification_timeout option.


-- 
https://code.launchpad.net/~szeestraten/charm-nagios/+git/nagios-charm/+merge/379943
Your team Nagios Charm developers is subscribed to branch charm-nagios:master.