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.
--
Mailing list: https://launchpad.net/~nagios-charmers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~nagios-charmers
More help : https://help.launchpad.net/ListHelp
