Merge lp://staging/~mbruzek/charms/trusty/apache2/nagios_extra_check_https into lp://staging/charms/trusty/apache2

thanks for improving on my branch, I was a bit clueless about the amulet tests, they were failing already

a typical params is something like this:

-S -H svc.example.com -I 127.0.0.1 -e '"200 OK"' --url=/

or so

if we want to give an example

Hey @mbruzek and @pedronis,

The new monitoring section of the README is fantastic. Excellent job!

However, I'm curious if we could implement this extra_http_checks differently. Rather than create a new config opt that takes yaml (which is difficult to type into the cli and juju-gui), could we make the existing nagios_check_http_params take multiple args? If so, it seems like the functionality from this MP could be achieved with:

juju set apache2 nagios_check_http_params="-H hostA -e '200 OK'; -H hostB -e '200 OK'"

The update_nrpe_checks() in hooks.py would then split nagios_check_http_params on a semicolon (or whatever delimiter makes sense) and call check_http on each of the splits. The README would also need to be updated to let users know that nagios_check_http_params could take multiple check params.

To me this is less confusing than having 2 config opts that are both passed on to check_http. Thoughts?

As an aside, when working through this review, I noticed 'make test' failed. I know you didn't introduce this failure, but I opened a bug in case you're interested in fixing :)

https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474

we went with a full new config because when you have many checks you really
care about being able to set sane descriptions for them, and finding a
format for that in one string I think would get unwieldy. Honestly we need
keep the original one for backward compatible/the simple case

On Thu, Oct 29, 2015 at 6:30 PM, Kevin W Monroe <email address hidden>
wrote:

> Hey @mbruzek and @pedronis,
>
> The new monitoring section of the README is fantastic. Excellent job!
>
> However, I'm curious if we could implement this extra_http_checks
> differently. Rather than create a new config opt that takes yaml (which is
> difficult to type into the cli and juju-gui), could we make the existing
> nagios_check_http_params take multiple args? If so, it seems like the
> functionality from this MP could be achieved with:
>
> juju set apache2 nagios_check_http_params="-H hostA -e '200 OK'; -H hostB
> -e '200 OK'"
>
> The update_nrpe_checks() in hooks.py would then split
> nagios_check_http_params on a semicolon (or whatever delimiter makes sense)
> and call check_http on each of the splits. The README would also need to be
> updated to let users know that nagios_check_http_params could take multiple
> check params.
>
> To me this is less confusing than having 2 config opts that are both
> passed on to check_http. Thoughts?
>
> As an aside, when working through this review, I noticed 'make test'
> failed. I know you didn't introduce this failure, but I opened a bug in
> case you're interested in fixing :)
>
> https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474
> --
>
> https://code.launchpad.net/~mbruzek/charms/trusty/apache2/nagios_extra_check_https/+merge/275095
> You are reviewing the proposed merge of
> lp:~mbruzek/charms/trusty/apache2/nagios_extra_check_https into
> lp:charms/trusty/apache2.
>

Hey Samuele, I get your point about multiple host checks becoming unwieldy in 1 string, but I'm still off-put by having 2 config opts for this. How do you feel about testing nagios_check_http_params for "yamlness", and looping over the yaml if true, or treating it as a string if false? Something like:

--- hooks/hooks.py 2015-04-15 20:23:01 +0000
+++ hooks/hooks.py 2015-10-29 18:27:20 +0000
@@ -432,11 +432,21 @@
     conf = nrpe_compat.config
     check_http_params = conf.get('nagios_check_http_params')
     if check_http_params:
- nrpe_compat.add_check(
- shortname='vhost',
- description='Check Virtual Host',
- check_cmd='check_http %s' % check_http_params
- )
+ for check_http in yaml.safe_load(check_http_params):
+ try:
+ name = check_http['name']
+ nrpe_compat.add_check(
+ shortname=name,
+ description=check_http.get('description', name),
+ check_cmd='check_http %s' % check_http['params']
+ )
+ except TypeError:
+ nrpe_compat.add_check(
+ shortname='vhost',
+ description='Check Virtual Host',
+ check_cmd='check_http %s' % check_http_params
+ )
+ break
     nrpe_compat.write()

Ouch on that formatting.. how about this: http://paste.ubuntu.com/13001287/

> Ouch on that formatting.. how about this: http://paste.ubuntu.com/13001287/

sorry been busy with other things, I'm not against if the ecosys team thinks is not too strange,

about that code, in the case of a simple string this works because the for check_http will be one char strings and so we get the TypeError, defensively I would put the try except just around

name = check_http['name']

or even be more explicit separating cases