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

[Jacek Nykis]

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

[Review Queue (review-queue)]

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)

[Jacek Nykis (jacekn)]

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: Resubmit

[Tim Van Steenburgh (tvansteenburgh)]

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

[Jacek Nykis (jacekn)]

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: Resubmit

[Jacek Nykis (jacekn)]

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?

[Charles Butler (lazypower)]

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

=== 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 @@
 #!/usr/bin/python
 
+import argparse
 from operator import itemgetter
 import re
 import subprocess
@@ -59,7 +60,11 @@
 
 
 def main():
-    proc = subprocess.Popen(['squidclient', 'mgr:server_list'],
+    parser = argparse.ArgumentParser(description='check_squidpeers')
+    parser.add_argument('-p', '--port', default=3128,
+                        help='Port number squid listens on')
+    args = parser.parse_args()
+    proc = subprocess.Popen(['squidclient', '-p', str(args.port), 'mgr:server_list'],
             stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     stdout, stderr = proc.communicate()
     if proc.returncode != 0:

=== 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 @@
     nrpe_compat.add_check(
         shortname='squidpeers',
         description='Check Squid Peers',
-        check_cmd='check_squidpeers'
+        check_cmd='check_squidpeers -p {port}'.format(**config_data)
     )
     check_http_params = conf.get('nagios_check_http_params')
     if check_http_params:

=== 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 @@
     """
     patches = {
         'config': {'object': nrpe},
+        'config_get': {'object': hooks},
         'log': {'object': nrpe},
         'getpwnam': {'object': pwd},
         'getgrnam': {'object': grp},
@@ -56,8 +57,12 @@
             self.assertEqual(expected, patcher.call_count, attr)
 
     def test_update_nrpe_no_nagios_bails(self):
-        config = {'nagios_context': 'test'}
+        config = {
+            'nagios_context': 'test',
+            'port': 8080,
+        }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['getpwnam'].side_effect = KeyError
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -70,8 +75,10 @@
         config = {
             'nagios_context': 'test',
             'nagios_check_http_params': '-u http://example.com/url',
+            'port': 8080,
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
         self.patched['listdir'].return_value = [
             'foo', 'bar.cfg', 'check_squidrp.cfg']
@@ -86,8 +93,10 @@
     def test_update_nrpe_uses_check_squidpeers(self):
         config = {
             'nagios_context': 'test',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
         self.patched['isfile'].return_value = False
 
@@ -115,7 +124,7 @@
              call().__enter__().write('# check squidpeers\n'),
              call().__enter__().write(
                  'command[check_squidpeers]='
-                 '/check_squidpeers\n')],
+                 '/check_squidpeers -p 8080\n')],
             any_order=True)
 
         self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
@@ -125,8 +134,10 @@
         config = {
             'nagios_context': 'test',
             'nagios_check_http_params': '-u foo -H bar',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
         self.patched['isfile'].return_value = False
 
@@ -163,8 +174,10 @@
         config = {
             'nagios_context': 'test',
             'services': '- {service_name: i_ytimg_com}',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -176,8 +189,10 @@
         config = {
             'nagios_context': 'test',
             'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -196,8 +211,10 @@
         config = {
             'nagios_context': 'test',
             'services': '- {service_name: i.ytimg.com, nrpe_check_path: /}',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -217,8 +234,10 @@
             'nagios_context': 'test',
             'x_balancer_name_allowed': True,
             'services': '- {service_name: i_ytimg_com, nrpe_check_path: /}',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -239,8 +258,10 @@
         config = {
             'nagios_context': 'test',
             'services': services,
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -261,8 +282,10 @@
         config = {
             'nagios_context': 'test',
             'services': services,
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())
@@ -280,9 +303,11 @@
     def test_update_nrpe_restarts_service(self):
         config = {
             'nagios_context': 'test',
-            'nagios_check_http_params': '-u foo -p 3128'
+            'nagios_check_http_params': '-u foo -p 3128',
+            'port': '8080',
         }
         self.patched['config'].return_value = Serializable(config)
+        self.patched['config_get'].return_value = Serializable(config)
         self.patched['exists'].return_value = True
 
         self.assertEqual(None, hooks.update_nrpe_checks())