Merge lp:~brad-marshall/charms/trusty/nagios/add-livestatus-support into lp:charms/trusty/nagios
| Status: | Merged |
|---|---|
| Merged at revision: | 31 |
| Proposed branch: | lp:~brad-marshall/charms/trusty/nagios/add-livestatus-support |
| Merge into: | lp:charms/trusty/nagios |
| Diff against target: |
227 lines (+147/-2) 5 files modified
README.md (+4/-0) config.yaml (+10/-0) hooks/install (+18/-0) hooks/upgrade-charm (+72/-2) tests/23-livestatus-test (+43/-0) |
| To merge this branch: | bzr merge lp:~brad-marshall/charms/trusty/nagios/add-livestatus-support |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Charles Butler (community) | 2015-05-01 | Approve on 2015-05-15 | |
|
Review via email:
|
|||
Description of the Change
Add livestatus support, which can't be done via the extraconfig since the broker_module needs to be in the main nagios.cfg file.
- 21. By Brad Marshall on 2015-05-01
-
[bradm] Merge from upstream, had started with lp:charms/nagios instead of lp:charms/trusty/nagios
- 22. By Brad Marshall on 2015-05-04
-
[bradm] Add test for livestatus config variable
| Brad Marshall (brad-marshall) wrote : | # |
| Adam Israel (aisrael) wrote : | # |
Hi!
I'm doing a triage of reviews in the queue, in the spirit of fail-fast. This is a cursory check of the proposed merge to see if there are any "obvious" things that may prevent the review from being accepted.
The goal is to get you feedback sooner, before an in-depth review has been done. I'm happy to see the addition of a unit test for the new functionality. Could you also add something to the README to explains the usage of this new feature?
Thanks!
- 23. By Brad Marshall on 2015-05-07
-
[bradm] Add documentation about livestatus config options, tidy up comments
| Brad Marshall (brad-marshall) wrote : | # |
I've added some basic documentation about the config variables.
| Charles Butler (lazypower) wrote : | # |
Greetings Brad,
I took some time to review this and would like to thank you for the contribution. The test coverage + documentation updates.
In response to your earlier question about running config-get, that is one way of doing it or you can read directly from the config dictionary you supply to the charm in the test. However if you did not supply one - your method was correct. To obtain the data from juju-run.
I found some test failures that seem unrelated to your merge. I filed them under this existing bug about tests failing in CI here: https:/
+1 LGTM. Thank you for taking the time to submit this feature for the nagios charm. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion.
If you have any questions/

Added tests for the livestatus path existing, although I'm not sure I did the config-get correctly - it feels like there should be a better way in amulet than a unit.run( 'config- get <var>') - please let me know if there is.