Code review comment for lp:~brad-marshall/charms/trusty/nagios/add-extra-config-options

Revision history for this message
Cory Johns (johnsca) wrote :

Brad,

Thank you for your addition to this charm.

Overall, these changes look good. I particularly think that the switch to a template for the config files makes it easier to follow. However, I did run into some issues when running the tests.

I had a few tests fail that were unrelated to this change, and the mysql charm seems to error when removing the nagios relation. However both of those seem to be outside the scope of this review, and there is an existing bug (https://bugs.launchpad.net/charms/+source/nagios/+bug/1403574) related to test failures in this charm, so I'm just going to note it here and move on.

Unfortunately, I also got an error on the newly added test: http://pastebin.ubuntu.com/12014504/ When I re-ran the test to attempt to triage it, it passed without error, so I think this is a race condition. You already have a sentry.wait() in the test, so I'm really not sure why I ran into that error. Perhaps, though, a for-range loop + sleep could be wrapped around the directory check to give it a bit of leeway to account for the race?

« Back to merge proposal