Merge lp://qastaging/~lazypower/charms/precise/nrpe/fix-lp-1287393 into lp://qastaging/charms/nrpe

Proposed by Charles Butler
Status: Merged
Approved by: Peter Petrakis
Approved revision: 30
Merged at revision: 28
Proposed branch: lp://qastaging/~lazypower/charms/precise/nrpe/fix-lp-1287393
Merge into: lp://qastaging/charms/nrpe
Diff against target: 206 lines (+189/-1)
2 files modified
hooks/nrpe-relation-joined (+1/-1)
lib/net.sh (+188/-0)
To merge this branch: bzr merge lp://qastaging/~lazypower/charms/precise/nrpe/fix-lp-1287393
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Matt Bruzek (community) Approve
Peter Petrakis Pending
Review via email: mp+209137@code.qastaging.launchpad.net

Description of the change

replaces charm-helpers-sh call with nslookup

https://codereview.appspot.com/69980048/

To post a comment you must log in.
Revision history for this message
Charles Butler (lazypower) wrote :

Reviewers: mp+209137_code.launchpad.net,

Message:
Please take a look.

Description:
replaces charm-helpers-sh call with nslookup

https://code.launchpad.net/~lazypower/charms/precise/nrpe/fix-lp-1287393/+merge/209137

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/69980048/

Affected files (+3, -1 lines):
   A [revision details]
   M hooks/nrpe-relation-joined

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: hooks/nrpe-relation-joined
=== modified file 'hooks/nrpe-relation-joined'
--- hooks/nrpe-relation-joined 2012-07-05 22:22:37 +0000
+++ hooks/nrpe-relation-joined 2014-03-03 22:05:22 +0000
@@ -7,7 +7,7 @@
  . hooks/nrpe.common.bash

  # NRPE requires IP
-address=$(ch_get_ip $(relation-get private-address))
+address=`nslookup $(unit-get private-address) | grep Add | grep -v '#' |
cut -f 2 -d ' '`

  juju-log "Adding $address to allowed_hosts."

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

I have a comment/question about the new command you are using.

https://codereview.appspot.com/69980048/diff/1/hooks/nrpe-relation-joined
File hooks/nrpe-relation-joined (right):

https://codereview.appspot.com/69980048/diff/1/hooks/nrpe-relation-joined#newcode10
hooks/nrpe-relation-joined:10: address=`nslookup $(unit-get
private-address) | grep Add | grep -v '#' | cut -f 2 -d ' '`
I ran this nslookup command on my own system and I believe the two greps
are cancelling each out.

$ nslookup 192.168.168.13
Server: 127.0.1.1
Address: 127.0.1.1#53

** server can't find 13.168.168.192.in-addr.arpa.: NXDOMAIN

$ nslookup 192.168.168.13 | grep Add
Address: 127.0.1.1#53

$ nslookup 192.168.168.13 | grep Add | grep -v '#'
(no match)

Are you sure this command works in all cases? Am I using the command
incorrectly here, or should we consider refining the command to work in
this case too?

https://codereview.appspot.com/69980048/

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

This change needs more information.

Add a comment to make it more clear what IP address the code is trying to get. The new code gets the unit address rather than the relation address (which I realize may be the same one).

Since there appears to be a possiblity of getting an empty address. Either fall back to the old way of getting the address or log a warning, or error if the address resolves to an empty string.

review: Needs Information
29. By Charles Butler

Embed lib/net.sh instead of rewriting the world

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

Embedded net.sh with an addition of returning $HOST if found.

Refactored out the cowboy nslookup that fails on local. Give it another go please

30. By Charles Butler

remove stray return 1 for debugging

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

+1 LGTM

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

Merged

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