nagios-charmers team mailing list archive

[Joe Guo <joe.guo@xxxxxxxxxxxxx>]

Review: Needs Fixing

2 inline comments, only the 2nd one matters.

Diff comments:

> diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
> index 4152e53..264e40a 100755
> --- a/hooks/upgrade_charm.py
> +++ b/hooks/upgrade_charm.py
> @@ -171,32 +171,33 @@ def fixpath(path):
>  
>  
>  def enable_livestatus_config():
> -    if enable_livestatus:
> -        hookenv.log("Livestatus is enabled")
> -        fetch.apt_update()
> -        fetch.apt_install("check-mk-livestatus")
> -
> -        # Make the directory and fix perms on it
> -        hookenv.log("Fixing perms on livestatus_path")
> -        livestatus_dir = os.path.dirname(livestatus_path)
> -
> -        if not os.path.isdir(livestatus_dir):
> -            hookenv.log("Making path for livestatus_dir")
> -            mkdir_p(livestatus_dir)
> -        fixpath(livestatus_dir)
> -
> -        # Fix the perms on the socket
> -        hookenv.log("Fixing perms on the socket")
> -        uid = pwd.getpwnam(nagios_user).pw_uid
> -        gid = grp.getgrnam("www-data").gr_gid
> -        os.chown(livestatus_path, uid, gid)
> -        os.chown(livestatus_dir, uid, gid)
> -        st = os.stat(livestatus_path)
> -        os.chmod(livestatus_path, st.st_mode | stat.S_IRGRP)
> -        os.chmod(
> -            livestatus_dir,
> -            st.st_mode | stat.S_IRGRP | stat.S_ISGID | stat.S_IXUSR | stat.S_IXGRP,
> -        )
> +    livestatus_dir = os.path.dirname(livestatus_path)
> +    hookenv.log("Livestatus is enabled")
> +    if not os.path.isdir(livestatus_dir):
> +        hookenv.log("Making path for livestatus_dir")
> +        mkdir_p(livestatus_dir)
> +    # fix perms on livestatus dir
> +    hookenv.log("Fixing perms on livestatus_path")
> +    fixpath(livestatus_dir)
> +    fetch.apt_update()
> +    install_packages = fetch.filter_installed_packages(["check-mk-livestatus"])

This line is correct, but it does confuse me.

`filter_installed_packages` is meant to *exclude* installed packages in the list , and return missing packages.

But `filter` can mean opposite in python, e.g.:

list(filter(str.islower, 'ABCxyz')) -> ['x', 'y', 'z']

`filter` here means *include* the lower letters, and return them.

To avoid this confusion, maybe change this line to:

missing_packages = fetch.filter_installed_packages(["check-mk-livestatus"]) or
packages_to_install = fetch.filter_installed_packages(["check-mk-livestatus"])

`install_packages` could easily be misread as `installed_packages` returned by `filter_installed_packages`.

> +    if install_packages:
> +        fetch.apt_install()

packages arg is missing for apt_install.

> +        # This needs a nagios restart to actually make the socket
> +        host.service_reload("nagios3")
> +
> +    # Fix the perms on the socket
> +    hookenv.log("Fixing perms on the socket")
> +    uid = pwd.getpwnam(nagios_user).pw_uid
> +    gid = grp.getgrnam("www-data").gr_gid
> +    os.chown(livestatus_path, uid, gid)
> +    os.chown(livestatus_dir, uid, gid)
> +    st = os.stat(livestatus_path)
> +    os.chmod(livestatus_path, st.st_mode | stat.S_IRGRP)
> +    os.chmod(
> +        livestatus_dir,
> +        st.st_mode | stat.S_IRGRP | stat.S_ISGID | stat.S_IXUSR | stat.S_IXGRP,
> +    )
>  
>  
>  def enable_pagerduty_config():


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