Merge lp:~nick-moffitt/charms/trusty/squid-reverseproxy/bikeshedding into lp:charms/trusty/squid-reverseproxy

[Nick Moffitt]

This is almost entirely cosmetic clean-ups I performed while trying to work out the flow of data in this charm. It shouldn't do too much, but I think it will help maintenance if this codebase is to be preserved.

I started from jacekn's nrpe fix branch, so that's listed as a prereq.

[Review Queue (review-queue)]

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

review: Needs Fixing (automated testing)

[Adam Israel (aisrael)]

Hi Nick,

Thanks for your work on this charm! I've reviewed this MP and everything looks good to me. +1

review: Approve

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-01-20 16:13:22 +0000
+++ hooks/hooks.py	2015-01-20 16:13:22 +0000
@@ -146,37 +146,31 @@
         dpkg.communicate(input=selections)
 
 
-###############################################################################
-# load_squid3_config:  Convenience function that loads (as a string) the
-#                       current squid3 configuration file.
-#                       Returns a string containing the squid3 config or
-#                       None
-###############################################################################
 def load_squid3_config(squid3_config_file="/etc/squid3/squid.conf"):
+    """Convenience function that loads (as a string) the current squid3
+    configuration file.
+
+    Returns a string containing the squid3 config or None"""
     if os.path.isfile(squid3_config_file):
         return open(squid3_config_file).read()
     return
 
 
-###############################################################################
-# get_service_ports:  Convenience function that scans the existing squid3
-#                     configuration file and returns a list of the existing
-#                     ports being used.  This is necessary to know which ports
-#                     to open and close when exposing/unexposing a service
-###############################################################################
 def get_service_ports(squid3_config_file="/etc/squid3/squid.conf"):
+    """Convenience function that scans the existing squid3 configuration
+    file and returns a list of the existing ports being used.
+
+    This is necessary to know which ports to open and close when
+    exposing/unexposing a service"""
     squid3_config = load_squid3_config(squid3_config_file)
     if squid3_config is None:
         return
     return re.findall("http_port ([0-9]+)", squid3_config)
 
 
-###############################################################################
-# get_sitenames:  Convenience function that scans the existing squid3
-#                 configuration file and returns a list of the sitenames
-#                 being served by the instance.
-###############################################################################
 def get_sitenames(squid3_config_file="/etc/squid3/squid.conf"):
+    """Convenience function that scans the existing squid3 configuration
+    file and returns a list of the sitenames being served by the instance."""
     squid3_config = load_squid3_config(squid3_config_file)
     if squid3_config is None:
         return
@@ -184,12 +178,9 @@
     return list(set(sitenames))
 
 
-###############################################################################
-# update_service_ports:  Convenience function that evaluate the old and new
-#                        service ports to decide which ports need to be
-#                        opened and which to close
-###############################################################################
 def update_service_ports(old_service_ports=None, new_service_ports=None):
+    """Convenience function that evaluate the old and new service ports to
+    decide which ports need to be opened and which to close"""
     if old_service_ports is None or new_service_ports is None:
         return
     for port in old_service_ports:
@@ -200,12 +191,10 @@
             open_port(port)
 
 
-###############################################################################
-# pwgen:  Generates a random password
-#         pwd_length:  Defines the length of the password to generate
-#                      default: 20
-###############################################################################
 def pwgen(pwd_length=20):
+    """Generates a random password
+    pwd_length:  Defines the length of the password to generate
+                 default: 20"""
     alphanumeric_chars = [l for l in (string.letters + string.digits)
                           if l not in 'Iil0oO1']
     random_chars = [random.choice(alphanumeric_chars)
@@ -222,10 +211,8 @@
     return template.render(vars)
 
 
-###############################################################################
-# construct_squid3_config:  Convenience function to write squid.conf
-###############################################################################
 def construct_squid3_config():
+    """Convenience function to write squid.conf"""
     config_data = config_get()
     reverse_sites = get_reverse_sites()
     only_direct = set()
@@ -355,34 +342,19 @@
         }))
 
 
-###############################################################################
-# service_squid3:  Convenience function to start/stop/restart/reload
-#                   the squid3 service
-###############################################################################
 def service_squid3(action=None, squid3_config=default_squid3_config):
+    """Convenience function to start/stop/restart/reload the squid3
+    service"""
     if action is None or squid3_config is None:
         return
     elif action == "check":
         check_cmd = ['/usr/sbin/squid3', '-f', squid3_config, '-k', 'parse']
-        retVal = subprocess.call(check_cmd)
-        if retVal == 1:
-            return False
-        elif retVal == 0:
-            return True
-        else:
-            return False
+        return not subprocess.call(check_cmd)
     elif action == 'status':
         status = subprocess.check_output(['status', 'squid3'])
-        if re.search('running', status) is not None:
-            return True
-        else:
-            return False
+        return re.search('running', status) is not None
     elif action in ('start', 'stop', 'reload', 'restart'):
-        retVal = subprocess.call([action, 'squid3'])
-        if retVal == 0:
-            return True
-        else:
-            return False
+        return not subprocess.call([action, 'squid3'])
 
 
 def install_nrpe_scripts():

=== modified file 'templates/main_config.template'
--- templates/main_config.template	2014-05-02 15:23:33 +0000
+++ templates/main_config.template	2015-01-20 16:13:22 +0000
@@ -15,12 +15,12 @@
 
 {% if config.snmp_community -%}
 acl snmp_access snmp_community {{ config.snmp_community }}
-{% for source in config.snmp_allowed_ips -%}
+  {% for source in config.snmp_allowed_ips -%}
 acl snmp_source src {{ source }}
-{% if loop.last -%}
+    {% if loop.last -%}
 snmp_access allow snmp_access snmp_source
-{% endif -%}
-{% endfor -%}
+    {% endif -%}
+  {% endfor -%}
 snmp_access allow localhost
 snmp_access deny all
 snmp_port {{ config.snmp_port }}
@@ -60,52 +60,52 @@
 
 # known services
 {% if sites -%}
-{% for sitename in sites.keys() -%}
-{% if sitename -%}
-{% set site_acl = "s_%s_acl" % loop.index %}
+  {% for sitename in sites.keys() -%}
+    {% if sitename -%}
+      {% set site_acl = "s_%s_acl" % loop.index %}
 acl {{ site_acl }} dstdomain {{ sitename }}
 http_access allow accel_ports {{ site_acl }}
 http_access allow CONNECT SSL_ports {{ site_acl }}
-{% if sitename in only_direct -%}
+      {% if sitename in only_direct -%}
 always_direct allow {{ site_acl }}
-{% else -%}
+      {% else -%}
 always_direct allow CONNECT SSL_ports {{ site_acl }}
 always_direct deny {{ site_acl }}
-{% endif -%}
-{% if config['x_balancer_name_allowed'] -%}
-{% set balancer_acl = "s_%s_balancer" % loop.index %}
+      {% endif -%}
+      {% if config['x_balancer_name_allowed'] -%}
+        {% set balancer_acl = "s_%s_balancer" % loop.index %}
 acl {{ balancer_acl }} req_header X-Balancer-Name {{ sitename.replace('.', '\.') }}
 http_access allow accel_ports {{ balancer_acl }}
 http_access allow CONNECT SSL_ports {{ balancer_acl }}
-{% if sitename in only_direct -%}
+        {% if sitename in only_direct -%}
 always_direct allow {{ balancer_acl }}
-{% else -%}
+        {% else -%}
 always_direct allow CONNECT SSL_ports {{ balancer_acl }}
 always_direct deny {{ balancer_acl }}
-{% endif -%}
-{% endif -%}
-{% else %}  # no sitename
+        {% endif -%}
+      {% endif -%}
+    {% else %}  # no sitename
 acl no_sitename_acl myport {{ config.port }}
-{% if config.enable_https -%}
+      {% if config.enable_https -%}
 acl no_sitename_acl myport {{ config.https_port }}
-{% endif -%}
+      {% endif -%}
 http_access allow accel_ports no_sitename_acl
 never_direct allow no_sitename_acl
-{% endif %}
-{% endfor -%}
+    {% endif %}
+  {% endfor -%}
 {% endif -%}
 
 {% if config.enable_forward_proxy -%}
 # no access retrictions
-{% for relation in forward_relations -%}
+  {% for relation in forward_relations -%}
 {# acl names are limited to 31 chars (!), so using short "fwd_" prefix #}
-{% set forward_acl = "fwd_%s" % relation['name'] -%}
+    {% set forward_acl = "fwd_%s" % relation['name'] -%}
 acl {{ forward_acl }} src {{ relation['private-address'] }}
 http_access allow {{ forward_acl }}
 http_access allow CONNECT SSL_ports {{ forward_acl }}
 always_direct allow {{ forward_acl }}
 always_direct allow CONNECT SSL_ports {{ forward_acl }}
-{% endfor -%}
+  {% endfor -%}
 {% endif -%}
 
 http_access allow manager localhost
@@ -120,21 +120,21 @@
 always_direct deny all
 
 {% if sites -%}
-{% for sitename in sites.keys() -%}
-{% set sites_loop = loop -%}
-{% for peer in sites[sitename] %}
-{% if sitename -%}
+  {% for sitename in sites.keys() -%}
+    {% set sites_loop = loop -%}
+    {% for peer in sites[sitename] %}
+      {% if sitename -%}
 cache_peer {{ peer.address }} parent {{ peer.port }} 0 name={{ peer.name }} no-query no-digest originserver round-robin login=PASS {{ peer.options }}
 cache_peer_access {{ peer.name }} allow s_{{ sites_loop.index }}_acl
-{% if config['x_balancer_name_allowed'] -%}
+        {% if config['x_balancer_name_allowed'] -%}
 cache_peer_access {{ peer.name }} allow s_{{ sites_loop.index }}_balancer
-{% endif -%}
+        {% endif -%}
 cache_peer_access {{ peer.name }} deny all
-{% else %}
+      {% else %}
 cache_peer {{ peer.address }} parent {{ peer.port }} 0 name={{ peer.name }} no-query no-digest originserver round-robin login=PASS
 cache_peer_access {{ peer.name }} allow no_sitename_acl
 cache_peer_access {{ peer.name }} deny all
-{% endif -%}
-{% endfor -%}
-{% endfor -%}
+      {% endif -%}
+    {% endfor -%}
+  {% endfor -%}
 {% endif -%}