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
1=== modified file 'files/check_squidpeers'
2--- files/check_squidpeers 2013-08-21 21:18:15 +0000
3+++ files/check_squidpeers 2014-12-22 15:11:21 +0000
4@@ -1,5 +1,6 @@
5 #!/usr/bin/python
6
7+import argparse
8 from operator import itemgetter
9 import re
10 import subprocess
11@@ -59,7 +60,11 @@
12
13
14 def main():
15- proc = subprocess.Popen(['squidclient', 'mgr:server_list'],
16+ parser = argparse.ArgumentParser(description='check_squidpeers')
17+ parser.add_argument('-p', '--port', default=3128,
18+ help='Port number squid listens on')
19+ args = parser.parse_args()
20+ proc = subprocess.Popen(['squidclient', '-p', str(args.port), 'mgr:server_list'],
21 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
22 stdout, stderr = proc.communicate()
23 if proc.returncode != 0:
24
25=== modified file 'hooks/hooks.py'
26--- hooks/hooks.py 2014-10-30 16:07:54 +0000
27+++ hooks/hooks.py 2014-12-22 15:11:21 +0000
28@@ -401,7 +401,7 @@
29 nrpe_compat.add_check(
30 shortname='squidpeers',
31 description='Check Squid Peers',
32- check_cmd='check_squidpeers'
33+ check_cmd='check_squidpeers -p {port}'.format(**config_data)
34 )
35 check_http_params = conf.get('nagios_check_http_params')
36 if check_http_params:
37
38=== modified file 'hooks/tests/test_nrpe_hooks.py'
39--- hooks/tests/test_nrpe_hooks.py 2014-10-30 16:07:54 +0000
40+++ hooks/tests/test_nrpe_hooks.py 2014-12-22 15:11:21 +0000
41@@ -22,6 +22,7 @@
42 """
43 patches = {
44 'config': {'object': nrpe},
45+ 'config_get': {'object': hooks},
46 'log': {'object': nrpe},
47 'getpwnam': {'object': pwd},
48 'getgrnam': {'object': grp},
49@@ -56,8 +57,12 @@
50 self.assertEqual(expected, patcher.call_count, attr)
51
52 def test_update_nrpe_no_nagios_bails(self):
53- config = {'nagios_context': 'test'}
54+ config = {
55+ 'nagios_context': 'test',
56+ 'port': 8080,
57+ }
58 self.patched['config'].return_value = Serializable(config)
59+ self.patched['config_get'].return_value = Serializable(config)
60 self.patched['getpwnam'].side_effect = KeyError
61
62 self.assertEqual(None, hooks.update_nrpe_checks())
63@@ -70,8 +75,10 @@
64 config = {
65 'nagios_context': 'test',
66 'nagios_check_http_params': '-u http://example.com/url',
67+ 'port': 8080,
68 }
69 self.patched['config'].return_value = Serializable(config)
70+ self.patched['config_get'].return_value = Serializable(config)
71 self.patched['exists'].return_value = True
72 self.patched['listdir'].return_value = [
73 'foo', 'bar.cfg', 'check_squidrp.cfg']
74@@ -86,8 +93,10 @@
75 def test_update_nrpe_uses_check_squidpeers(self):
76 config = {
77 'nagios_context': 'test',
78+ 'port': '8080',
79 }
80 self.patched['config'].return_value = Serializable(config)
81+ self.patched['config_get'].return_value = Serializable(config)
82 self.patched['exists'].return_value = True
83 self.patched['isfile'].return_value = False
84
85@@ -115,7 +124,7 @@
86 call().__enter__().write('# check squidpeers\n'),
87 call().__enter__().write(
88 'command[check_squidpeers]='
89- '/check_squidpeers\n')],
90+ '/check_squidpeers -p 8080\n')],
91 any_order=True)
92
93 self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
94@@ -125,8 +134,10 @@
95 config = {
96 'nagios_context': 'test',
97 'nagios_check_http_params': '-u foo -H bar',
98+ 'port': '8080',
99 }
100 self.patched['config'].return_value = Serializable(config)
101+ self.patched['config_get'].return_value = Serializable(config)
102 self.patched['exists'].return_value = True
103 self.patched['isfile'].return_value = False
104
105@@ -163,8 +174,10 @@
106 config = {
107 'nagios_context': 'test',
108 'services': '- {service_name: i_ytimg_com}',
109+ 'port': '8080',
110 }
111 self.patched['config'].return_value = Serializable(config)
112+ self.patched['config_get'].return_value = Serializable(config)
113 self.patched['exists'].return_value = True
114
115 self.assertEqual(None, hooks.update_nrpe_checks())
116@@ -176,8 +189,10 @@
117 config = {
118 'nagios_context': 'test',
119 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
120+ 'port': '8080',
121 }
122 self.patched['config'].return_value = Serializable(config)
123+ self.patched['config_get'].return_value = Serializable(config)
124 self.patched['exists'].return_value = True
125
126 self.assertEqual(None, hooks.update_nrpe_checks())
127@@ -196,8 +211,10 @@
128 config = {
129 'nagios_context': 'test',
130 'services': '- {service_name: i.ytimg.com, nrpe_check_path: /}',
131+ 'port': '8080',
132 }
133 self.patched['config'].return_value = Serializable(config)
134+ self.patched['config_get'].return_value = Serializable(config)
135 self.patched['exists'].return_value = True
136
137 self.assertEqual(None, hooks.update_nrpe_checks())
138@@ -217,8 +234,10 @@
139 'nagios_context': 'test',
140 'x_balancer_name_allowed': True,
141 'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
142+ 'port': '8080',
143 }
144 self.patched['config'].return_value = Serializable(config)
145+ self.patched['config_get'].return_value = Serializable(config)
146 self.patched['exists'].return_value = True
147
148 self.assertEqual(None, hooks.update_nrpe_checks())
149@@ -239,8 +258,10 @@
150 config = {
151 'nagios_context': 'test',
152 'services': services,
153+ 'port': '8080',
154 }
155 self.patched['config'].return_value = Serializable(config)
156+ self.patched['config_get'].return_value = Serializable(config)
157 self.patched['exists'].return_value = True
158
159 self.assertEqual(None, hooks.update_nrpe_checks())
160@@ -261,8 +282,10 @@
161 config = {
162 'nagios_context': 'test',
163 'services': services,
164+ 'port': '8080',
165 }
166 self.patched['config'].return_value = Serializable(config)
167+ self.patched['config_get'].return_value = Serializable(config)
168 self.patched['exists'].return_value = True
169
170 self.assertEqual(None, hooks.update_nrpe_checks())
171@@ -280,9 +303,11 @@
172 def test_update_nrpe_restarts_service(self):
173 config = {
174 'nagios_context': 'test',
175- 'nagios_check_http_params': '-u foo -p 3128'
176+ 'nagios_check_http_params': '-u foo -p 3128',
177+ 'port': '8080',
178 }
179 self.patched['config'].return_value = Serializable(config)
180+ self.patched['config_get'].return_value = Serializable(config)
181 self.patched['exists'].return_value = True
182
183 self.assertEqual(None, hooks.update_nrpe_checks())

Subscribers

People subscribed via source and target branches

to all changes: