Juju Charms Collection
squid-reverseproxy package

Merge lp:~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix into lp:charms/trusty/squid-reverseproxy

  1. Trusty Tahr (14.04)
  2. squid-reverseproxy-nrpe-fix
  3. Merge into trunk
Proposed by Jacek Nykis
Status: Merged
Merged at revision: 51
Proposed branch: lp:~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix
Merge into: lp:charms/trusty/squid-reverseproxy
Diff against target: 183 lines (+35/-5)
3 files modified
files/check_squidpeers (+6/-1)
hooks/hooks.py (+1/-1)
hooks/tests/test_nrpe_hooks.py (+28/-3)
To merge this branch: bzr merge lp:~jacekn/charms/trusty/squid-reverseproxy/squid-reverseproxy-nrpe-fix
Reviewer Review Type Date Requested Status
Charles Butler (community) Approve
Jacek Nykis (community) Needs Resubmitting
Tim Van Steenburgh (community) Needs Fixing
Review Queue (community) automated testing Needs Fixing
Review via email: mp+245312@code.launchpad.net

Description of the change

This change fixes check_squidpeers nrpe check which does not work with non default port

To post a comment you must log in.
Revision history for this message
Review Queue (review-queue) wrote :

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10823-results

review: Needs Fixing (automated testing)
Revision history for this message
Jacek Nykis (jacekn) wrote :

Sorry I am not sure what the problem is here. I have not touched the line that is failing at all.

All tests pass on my laptop. I am also running the charm in my environment and it's completely fine.

review: Needs Resubmitting
Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote :

The problem is that the tests depend on the python-apt package, but the `make test` target does not ensure that it's installed.

review: Needs Fixing
Revision history for this message
Jacek Nykis (jacekn) wrote :

Hi Tim,

Understood but how does this relate to my change? I have not touched the piece of code that fails. I am almost certain this is pre existing problem and my change is irrelevant here.

If this MP needs to depend on the Makefile fix so be it but I believe it should be separate piece of work.
When I have time I may look at fixing the Makefile but at this point I have different priorities.

review: Needs Resubmitting
Revision history for this message
Jacek Nykis (jacekn) wrote :

Hello,

It's been 2 weeks since my last comment. Is there anything else you need from me to merge this bugfix into the charmstore charm?

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

Hi Jacek,

Thanks for the improvement to the squid-reverse-proxy charm! It appears that there was some back and forth regarding dependency management in the charm regarding CI. I've reviewed the charm based on the merits of the MP, as we are tracking that issue independently:

https://bugs.launchpad.net/charms/+source/squid-reverseproxy/+bug/1401323

If you happen to have the time to address this as another branch that would be brilliant!

The changes contained herein look good to me and will be merged upstream. Thanks for your patience and dedication during the charm review process.

All the best!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'files/check_squidpeers'
--- files/check_squidpeers 2013-08-21 21:18:15 +0000
+++ files/check_squidpeers 2014-12-22 15:11:21 +0000
@@ -1,5 +1,6 @@
1#!/usr/bin/python1#!/usr/bin/python
22
3import argparse
3from operator import itemgetter4from operator import itemgetter
4import re5import re
5import subprocess6import subprocess
@@ -59,7 +60,11 @@
5960
6061
61def main():62def main():
62 proc = subprocess.Popen(['squidclient', 'mgr:server_list'],63 parser = argparse.ArgumentParser(description='check_squidpeers')
64 parser.add_argument('-p', '--port', default=3128,
65 help='Port number squid listens on')
66 args = parser.parse_args()
67 proc = subprocess.Popen(['squidclient', '-p', str(args.port), 'mgr:server_list'],
63 stdout=subprocess.PIPE, stderr=subprocess.PIPE)68 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
64 stdout, stderr = proc.communicate()69 stdout, stderr = proc.communicate()
65 if proc.returncode != 0:70 if proc.returncode != 0:
6671
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py 2014-10-30 16:07:54 +0000
+++ hooks/hooks.py 2014-12-22 15:11:21 +0000
@@ -401,7 +401,7 @@
401 nrpe_compat.add_check(401 nrpe_compat.add_check(
402 shortname='squidpeers',402 shortname='squidpeers',
403 description='Check Squid Peers',403 description='Check Squid Peers',
404 check_cmd='check_squidpeers'404 check_cmd='check_squidpeers -p {port}'.format(**config_data)
405 )405 )
406 check_http_params = conf.get('nagios_check_http_params')406 check_http_params = conf.get('nagios_check_http_params')
407 if check_http_params:407 if check_http_params:
408408
=== modified file 'hooks/tests/test_nrpe_hooks.py'
--- hooks/tests/test_nrpe_hooks.py 2014-10-30 16:07:54 +0000
+++ hooks/tests/test_nrpe_hooks.py 2014-12-22 15:11:21 +0000
@@ -22,6 +22,7 @@
22 """22 """
23 patches = {23 patches = {
24 'config': {'object': nrpe},24 'config': {'object': nrpe},
25 'config_get': {'object': hooks},
25 'log': {'object': nrpe},26 'log': {'object': nrpe},
26 'getpwnam': {'object': pwd},27 'getpwnam': {'object': pwd},
27 'getgrnam': {'object': grp},28 'getgrnam': {'object': grp},
@@ -56,8 +57,12 @@
56 self.assertEqual(expected, patcher.call_count, attr)57 self.assertEqual(expected, patcher.call_count, attr)
5758
58 def test_update_nrpe_no_nagios_bails(self):59 def test_update_nrpe_no_nagios_bails(self):
59 config = {'nagios_context': 'test'}60 config = {
61 'nagios_context': 'test',
62 'port': 8080,
63 }
60 self.patched['config'].return_value = Serializable(config)64 self.patched['config'].return_value = Serializable(config)
65 self.patched['config_get'].return_value = Serializable(config)
61 self.patched['getpwnam'].side_effect = KeyError66 self.patched['getpwnam'].side_effect = KeyError
6267
63 self.assertEqual(None, hooks.update_nrpe_checks())68 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -70,8 +75,10 @@
70 config = {75 config = {
71 'nagios_context': 'test',76 'nagios_context': 'test',
72 'nagios_check_http_params': '-u http://example.com/url',77 'nagios_check_http_params': '-u http://example.com/url',
78 'port': 8080,
73 }79 }
74 self.patched['config'].return_value = Serializable(config)80 self.patched['config'].return_value = Serializable(config)
81 self.patched['config_get'].return_value = Serializable(config)
75 self.patched['exists'].return_value = True82 self.patched['exists'].return_value = True
76 self.patched['listdir'].return_value = [83 self.patched['listdir'].return_value = [
77 'foo', 'bar.cfg', 'check_squidrp.cfg']84 'foo', 'bar.cfg', 'check_squidrp.cfg']
@@ -86,8 +93,10 @@
86 def test_update_nrpe_uses_check_squidpeers(self):93 def test_update_nrpe_uses_check_squidpeers(self):
87 config = {94 config = {
88 'nagios_context': 'test',95 'nagios_context': 'test',
96 'port': '8080',
89 }97 }
90 self.patched['config'].return_value = Serializable(config)98 self.patched['config'].return_value = Serializable(config)
99 self.patched['config_get'].return_value = Serializable(config)
91 self.patched['exists'].return_value = True100 self.patched['exists'].return_value = True
92 self.patched['isfile'].return_value = False101 self.patched['isfile'].return_value = False
93102
@@ -115,7 +124,7 @@
115 call().__enter__().write('# check squidpeers\n'),124 call().__enter__().write('# check squidpeers\n'),
116 call().__enter__().write(125 call().__enter__().write(
117 'command[check_squidpeers]='126 'command[check_squidpeers]='
118 '/check_squidpeers\n')],127 '/check_squidpeers -p 8080\n')],
119 any_order=True)128 any_order=True)
120129
121 self.check_call_counts(config=1, getpwnam=1, getgrnam=1,130 self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
@@ -125,8 +134,10 @@
125 config = {134 config = {
126 'nagios_context': 'test',135 'nagios_context': 'test',
127 'nagios_check_http_params': '-u foo -H bar',136 'nagios_check_http_params': '-u foo -H bar',
137 'port': '8080',
128 }138 }
129 self.patched['config'].return_value = Serializable(config)139 self.patched['config'].return_value = Serializable(config)
140 self.patched['config_get'].return_value = Serializable(config)
130 self.patched['exists'].return_value = True141 self.patched['exists'].return_value = True
131 self.patched['isfile'].return_value = False142 self.patched['isfile'].return_value = False
132143
@@ -163,8 +174,10 @@
163 config = {174 config = {
164 'nagios_context': 'test',175 'nagios_context': 'test',
165 'services': '- {service_name: i_ytimg_com}',176 'services': '- {service_name: i_ytimg_com}',
177 'port': '8080',
166 }178 }
167 self.patched['config'].return_value = Serializable(config)179 self.patched['config'].return_value = Serializable(config)
180 self.patched['config_get'].return_value = Serializable(config)
168 self.patched['exists'].return_value = True181 self.patched['exists'].return_value = True
169182
170 self.assertEqual(None, hooks.update_nrpe_checks())183 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -176,8 +189,10 @@
176 config = {189 config = {
177 'nagios_context': 'test',190 'nagios_context': 'test',
178 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',191 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
192 'port': '8080',
179 }193 }
180 self.patched['config'].return_value = Serializable(config)194 self.patched['config'].return_value = Serializable(config)
195 self.patched['config_get'].return_value = Serializable(config)
181 self.patched['exists'].return_value = True196 self.patched['exists'].return_value = True
182197
183 self.assertEqual(None, hooks.update_nrpe_checks())198 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -196,8 +211,10 @@
196 config = {211 config = {
197 'nagios_context': 'test',212 'nagios_context': 'test',
198 'services': '- {service_name: i.ytimg.com, nrpe_check_path: /}',213 'services': '- {service_name: i.ytimg.com, nrpe_check_path: /}',
214 'port': '8080',
199 }215 }
200 self.patched['config'].return_value = Serializable(config)216 self.patched['config'].return_value = Serializable(config)
217 self.patched['config_get'].return_value = Serializable(config)
201 self.patched['exists'].return_value = True218 self.patched['exists'].return_value = True
202219
203 self.assertEqual(None, hooks.update_nrpe_checks())220 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -217,8 +234,10 @@
217 'nagios_context': 'test',234 'nagios_context': 'test',
218 'x_balancer_name_allowed': True,235 'x_balancer_name_allowed': True,
219 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',236 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
237 'port': '8080',
220 }238 }
221 self.patched['config'].return_value = Serializable(config)239 self.patched['config'].return_value = Serializable(config)
240 self.patched['config_get'].return_value = Serializable(config)
222 self.patched['exists'].return_value = True241 self.patched['exists'].return_value = True
223242
224 self.assertEqual(None, hooks.update_nrpe_checks())243 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -239,8 +258,10 @@
239 config = {258 config = {
240 'nagios_context': 'test',259 'nagios_context': 'test',
241 'services': services,260 'services': services,
261 'port': '8080',
242 }262 }
243 self.patched['config'].return_value = Serializable(config)263 self.patched['config'].return_value = Serializable(config)
264 self.patched['config_get'].return_value = Serializable(config)
244 self.patched['exists'].return_value = True265 self.patched['exists'].return_value = True
245266
246 self.assertEqual(None, hooks.update_nrpe_checks())267 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -261,8 +282,10 @@
261 config = {282 config = {
262 'nagios_context': 'test',283 'nagios_context': 'test',
263 'services': services,284 'services': services,
285 'port': '8080',
264 }286 }
265 self.patched['config'].return_value = Serializable(config)287 self.patched['config'].return_value = Serializable(config)
288 self.patched['config_get'].return_value = Serializable(config)
266 self.patched['exists'].return_value = True289 self.patched['exists'].return_value = True
267290
268 self.assertEqual(None, hooks.update_nrpe_checks())291 self.assertEqual(None, hooks.update_nrpe_checks())
@@ -280,9 +303,11 @@
280 def test_update_nrpe_restarts_service(self):303 def test_update_nrpe_restarts_service(self):
281 config = {304 config = {
282 'nagios_context': 'test',305 'nagios_context': 'test',
283 'nagios_check_http_params': '-u foo -p 3128'306 'nagios_check_http_params': '-u foo -p 3128',
307 'port': '8080',
284 }308 }
285 self.patched['config'].return_value = Serializable(config)309 self.patched['config'].return_value = Serializable(config)
310 self.patched['config_get'].return_value = Serializable(config)
286 self.patched['exists'].return_value = True311 self.patched['exists'].return_value = True
287312
288 self.assertEqual(None, hooks.update_nrpe_checks())313 self.assertEqual(None, hooks.update_nrpe_checks())

Subscribers

People subscribed via source and target branches

to all changes: