Juju Charms Collection
nagios package

Merge lp://qastaging/~brad-marshall/charms/trusty/nagios/add-livestatus-support into lp://qastaging/charms/trusty/nagios

  1. Trusty Tahr (14.04)
  2. add-livestatus-support
  3. Merge into trunk
Proposed by Brad Marshall
Status: Merged
Merged at revision: 31
Proposed branch: lp://qastaging/~brad-marshall/charms/trusty/nagios/add-livestatus-support
Merge into: lp://qastaging/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://qastaging/~brad-marshall/charms/trusty/nagios/add-livestatus-support
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Review via email: mp+257992@code.qastaging.launchpad.net

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.

To post a comment you must log in.
lp://qastaging/~brad-marshall/charms/trusty/nagios/add-livestatus-support updated
21. By Brad Marshall

[bradm] Merge from upstream, had started with lp:charms/nagios instead of lp:charms/trusty/nagios

22. By Brad Marshall

[bradm] Add test for livestatus config variable

Revision history for this message
Brad Marshall (brad-marshall) wrote :

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.

Revision history for this message
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!

lp://qastaging/~brad-marshall/charms/trusty/nagios/add-livestatus-support updated
23. By Brad Marshall

[bradm] Add documentation about livestatus config options, tidy up comments

Revision history for this message
Brad Marshall (brad-marshall) wrote :

I've added some basic documentation about the config variables.

Revision history for this message
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://bugs.launchpad.net/charms/+source/nagios/+bug/1403574

+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/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: