nagios-charmers team mailing list archive

[Chris Sanders <chris.sanders@xxxxxxxxxxxxx>]

Review: Needs Fixing

Comment in-line, I wouldn't suggest reviewing the whole charm for issues like this but since this change is directly reworking this section of code it seems in-scope to address errors in the subprocess which could arrise.

Diff comments:

> diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
> index 05bb384..5d95e77 100755
> --- a/hooks/upgrade-charm
> +++ b/hooks/upgrade-charm
> @@ -361,31 +368,44 @@ def update_apache():
>      with open('hooks/templates/default-ssl.tmpl', 'r') as f:
>          templateDef = f.read()
>  
> +    t = Template(templateDef)
> +    ssl_conf = '/etc/apache2/sites-available/default-ssl.conf'
> +    with open(ssl_conf, 'w') as f:
> +        f.write(t.render(template_values))
> +
>      # Create directory for extra *.include files installed by subordinates
>      try:
>          os.makedirs('/etc/apache2/vhost.d/')
>      except OSError:
>          pass
>  
> -    t = Template(templateDef)
> -    with open('/etc/apache2/sites-available/default-ssl.conf', 'w') as f:
> -        f.write(t.render(template_values))
> -    print("Value of ssl is %s" % ssl)
> -    if ssl_config == "only":
> -        subprocess.call(['a2dissite', 'default'])
> -        hookenv.close_port(80)
> -        subprocess.call(['a2ensite', 'default-ssl'])
> -        subprocess.call(['a2enmod', 'ssl'])
> -    elif ssl_config == "on":
> +    # Configure the behavior of http sites
> +    sites = glob.glob("/etc/apache2/sites-available/*.conf")
> +    non_ssl = set(sites) - {ssl_conf}
> +    for each in non_ssl:
> +        site = os.path.basename(each).strip('.conf')
> +        if HTTP_ENABLED:
> +            # this default vhost config must be disabled
> +            hookenv.log("Disabling %s..." % site, "INFO")
> +            subprocess.call(['a2dissite', site])

If we need to subprocess, we should be checking the return codes and handling failures. I'm aware this was happening before this change but while you're already reworking this part of the code we should check for errors on the call.

> +            hookenv.close_port(80)
> +        else:
> +            # this default vhost config must be enabled
> +            hookenv.log("Enabling %s..." % site, "INFO")
> +            subprocess.call(['a2ensite', site])

If we need to subprocess, we should be checking the return codes and handling failures. I'm aware this was happening before this change but while you're already reworking this part of the code we should check for errors on the call.

> +            hookenv.open_port(80)
> +
> +    # Configure the behavior of https site
> +    if SSL_CONFIGURED:
> +        hookenv.log("Enabling default-ssl...", "INFO")
>          subprocess.call(['a2ensite', 'default-ssl'])
>          subprocess.call(['a2enmod', 'ssl'])

Might as well handle this one the same as above while you're here.

>          hookenv.open_port(443)
>      else:
>          subprocess.call(['a2dissite', 'default-ssl'])
>          hookenv.close_port(443)
> -        subprocess.call(['a2ensite', 'default'])
> -        hookenv.open_port(80)
>  
> +    # Finally, restart apache2
>      host.service_reload('apache2')
>  
>  


-- 
https://code.launchpad.net/~addyess/charm-nagios/+git/charm-nagios/+merge/387305
Your team Nagios Charm developers is subscribed to branch charm-nagios:master.