Juju Charms Collection
nrpe-external-master package

Merge lp://qastaging/~macgreagoir/charms/precise/nrpe-external-master/load into lp://qastaging/charms/nrpe-external-master

  1. Precise Pangolin (12.04)
  2. load
  3. Merge into trunk
Proposed by Mick Gregg
Status: Merged
Merged at revision: 41
Proposed branch: lp://qastaging/~macgreagoir/charms/precise/nrpe-external-master/load
Merge into: lp://qastaging/charms/nrpe-external-master
Diff against target: 50 lines (+18/-3)
2 files modified
config.yaml (+6/-2)
hooks/config-changed (+12/-1)
To merge this branch: bzr merge lp://qastaging/~macgreagoir/charms/precise/nrpe-external-master/load
Reviewer Review Type Date Requested Status
Matt Bruzek (community) Approve
Charles Butler (community) Needs Information
Alvaro Uria (community) Approve
Review via email: mp+258034@code.qastaging.launchpad.net

Commit message

[macgreagoir] Make it possible to autogenerate check_load parameters.

To post a comment you must log in.
Revision history for this message
Alvaro Uria (aluria) wrote :

lgtm

review: Approve
Revision history for this message
Charles Butler (lazypower) wrote :

Greetings Mick Gregg

I took some time to review this MP, and I really like the thought of the NRPE master generating the proper config for checks. While this looks great, there appears to be a slight issue with the code.

When testing, I got some errors on the config-changed hook that surface around line:

 56 let "LOAD_WARN = $PROC_COUNT / 2"

When the unit only has a single processor. I've proposed a branch for merging into yours. If you feel this is a good patch to resolve the issue, feel free to merge it and ping me for a follow up express review/merge.

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: Needs Information
Revision history for this message
Charles Butler (lazypower) wrote :

Ah, and I realized I didn't link to that branch, which is here: lp:~lazypower/charms/precise/nrpe-external-master/patch-auto-config

Revision history for this message
Mick Gregg (macgreagoir) wrote :

Charles, thanks for the review. I could only see an empty diff in your MP, sorry. I've committed a new change that I hope does something similar to what you meant to propose.

Revision history for this message
Matt Bruzek (mbruzek) wrote :

Hello Mick,

Thanks for submitting this fix to the nrpe-external-master charm. The LOAD_WARN now evalutes to 1 when PROC_COUNT = 1.

I ran your branch on our CI system, and the tests pass. The CI system runs instances with 2 processors. The tests for the nrpe-external-master charm are very basic stand up charm test. As a user of NRPE it would be much appreciated if you could add better tests to the amulet test on your next commit. I would suggest writing tests that at least test the value of the load were set correctly

This change looks good to me, so I am going to merge it with the recommended version of the charm in the charm store. Thanks for the contribution!

review: Approve
Revision history for this message
Mick Gregg (macgreagoir) wrote :

Mark,

Thanks for the review. It didn't appear to merge, so I've set the status back to 'Needs review'. Do you mind taking a look at the merge again, please.

Cheers,

Mick

Revision history for this message
Matt Bruzek (mbruzek) wrote :

Mick,

Sorry about the mistake, I thought the code was merged. The code is merged now. Thanks for the follow up. Many apologies for my mistake.

Revision history for this message
Mick Gregg (macgreagoir) wrote :

Matt, no worries at all. Thanks again.

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: