bootstack-charmers team mailing list archive

[Zachary Zehring <zachary.zehring@xxxxxxxxxxxxx>]

WIP in reference to review. Will continue tomorrow.

Diff comments:

> diff --git a/config.yaml b/config.yaml
> index ba623bf..286fbfe 100644
> --- a/config.yaml
> +++ b/config.yaml
> @@ -60,3 +60,22 @@ options:
>      default: "ppa:juju-linters/ppa"
>    install_keys:
>      default: "null"
> +
> +  nagios_context:

You can remove these nagios config options from the config if you include "layer:nagios" in your layer.yaml. I'd recommend that to reduce config code and reuse applicable layer.

> +    type: string
> +    default: "juju"
> +    description: |
> +      Used by the nrpe-external-master subordinate charm.
> +      A string that will be prepended to instance name to set the host name
> +      in nagios. So for instance the hostname would be something like:
> +      .
> +          juju-myservice-0
> +      .
> +      If you're running multiple environments with the same services in them
> +      this allows you to differentiate between them.
> +  nagios_servicegroups:
> +    type: string
> +    default: ""
> +    description: |
> +      A comma-separated list of nagios servicegroups.
> +      If left empty, the nagios_context will be used as the servicegroup.
> diff --git a/reactive/charm_juju_lint.py b/reactive/charm_juju_lint.py
> index c3ad41c..23004eb 100644
> --- a/reactive/charm_juju_lint.py
> +++ b/reactive/charm_juju_lint.py
> @@ -34,35 +34,33 @@ from charms.reactive import (
>  
>  USR_LIB = "/usr/lib/juju-lint"
>  VAR_LIB = "/var/lib/juju-lint"
> +LINT_RESULTS_FILE = path.join(VAR_LIB, 'lint-results.txt')
> +NAGIOS_PLUGINS_DIR = '/usr/local/lib/nagios/plugins/'
>  
>  
>  @when_not("charm-juju-lint.installed")
>  def install_charm_juju_lint():
>      create_install_dirs()
> -    virtualenv.create_environment(USR_LIB)
> -    packages.pip_install("juju", venv=USR_LIB)
>      deploy_scripts()
>      hookenv.status_set("active", "Unit is ready")
>      set_flag("charm-juju-lint.installed")
>  
>  
>  def create_install_dirs():
> -    install_dirs = (USR_LIB, VAR_LIB)
> +    install_dirs = (USR_LIB, VAR_LIB, NAGIOS_PLUGINS_DIR)
>      for install_dir in install_dirs:
> -        if not path.isdir(install_dir):
> -            try:
> -                mkdir(install_dir)
> -            except OSError as error:
> -                print("{} directory creation failed: {}".format(
> -                    install_dir, error
> -                ))
> +        makedirs(install_dir, exist_ok=True)
>  
>  
>  def deploy_scripts():
> -    rsync(
> -        path.join(getenv("CHARM_DIR"), "scripts", "auto_lint.py"),
> -        path.join(USR_LIB, "auto_lint.py")
> -    )
> +    # install scripts

Seems like unnecessary comment. Maybe make docstring and flesh it out more.

> +    charm_dir = hookenv.charm_dir()
> +    # NOTE: the trailing slash(/) here is important for rsync
> +    # which means all content in dir other than dir itself.
> +    scripts_dir = path.join(charm_dir, 'scripts/')
> +    rsync(path.join(scripts_dir, "auto_lint.py"), path.join(USR_LIB, "auto_lint.py"))
> +    # install all scripts/check_xxx files into nagios plugins dir
> +    rsync(scripts_dir, NAGIOS_PLUGINS_DIR, options=["--include='check_*'"])
>  
>  
>  @when("charm-juju-lint.installed", "config.changed")
> @@ -95,3 +93,34 @@ def create_crontab():
>      )
>      with open("/etc/cron.d/juju-lint", "w") as fp:
>          fp.write(cron_job)
> +
> +    # call it now so we have a lint-results.txt

Personal preference, but I'd try to abstract the logic this comment is calling out (since it seems to not be totally clear) into a private function. Then, you can name the function something meaningful and even give more explanation in the docstring if need be.

> +    # for check_juju_lint_results.py to check against
> +    # it also acts as a smoke test for the cron job
> +    try:
> +        subprocess.check_call([path.join(USR_LIB, "auto_lint.py")])
> +    except subprocess.CalledProcessError as e:
> +        hookenv.log('fail to run auto_lint.py: {}'.format(e), 'ERROR')
> +        hookenv.status_set('blocked', 'fail to run auto_lint.py')
> +
> +
> +@when_not("juju-lint.lint-results-file.available")
> +def check_lint_results_file():

Think we talked about this in scrum, where charm won't be checking for this file. So won't look much into here.

> +    if path.isfile(LINT_RESULTS_FILE):
> +        set_flag("juju-lint.lint-results-file.available")
> +        hookenv.status_set('active', 'Unit is ready')
> +
> +
> +@when("juju-lint.lint-results-file.available", "nrpe-external-master.available")

Also from scrum, rework flags so this doesn't run on every update-status.

> +def update_nrpe_checks():
> +    nrpe_client = nrpe.NRPE()
> +
> +    shortname = 'juju_lint_results'
> +    nrpe_client.add_check(
> +        shortname=shortname,
> +        description='check juju lint results file genrated by juju-lint',
> +        # class nrpe.Check will convert cmd to full path.

I'd take this comment out. But mostly personal preference.

> +        check_cmd='check_juju_lint_results.py --lint-results-file {}'.format(LINT_RESULTS_FILE)
> +    )
> +    hookenv.log('check_{} added'.format(shortname))
> +    nrpe_client.write()
> diff --git a/scripts/check_juju_lint_results.py b/scripts/check_juju_lint_results.py
> new file mode 100755
> index 0000000..7d9caab
> --- /dev/null
> +++ b/scripts/check_juju_lint_results.py
> @@ -0,0 +1,79 @@
> +#!/usr/bin/env python3
> +
> +"""
> +# check_juju_lint_results.py
> +
> +Nagios plugin script to check juju lint results.
> +
> +Copyright (C) 2020 Canonical Ltd.
> +Copyright (C) 2020 Joe Guo <joe.guo@xxxxxxxxxxxxx>
> +
> +This program is free software: you can redistribute it and/or modify it under
> +the terms of the GNU General Public License version 3, as published by the Free
> +Software Foundation.
> +
> +This program is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
> +PARTICULAR PURPOSE.  See the GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License along with
> +this program.  If not, see <https://www.gnu.org/licenses/>.
> +"""
> +
> +import argparse
> +import sys
> +from os.path import join
> +
> +VAR_LIB = "/var/lib/juju-lint"
> +LINT_RESULTS_FILE = join(VAR_LIB, 'lint-results.txt')
> +
> +NAGIOS_STATUS_OK = 0
> +NAGIOS_STATUS_WARNING = 1
> +NAGIOS_STATUS_CRITICAL = 2
> +NAGIOS_STATUS_UNKNOWN = 3
> +
> +NAGIOS_STATUS = {
> +    NAGIOS_STATUS_OK: 'OK',
> +    NAGIOS_STATUS_WARNING: 'WARNING',
> +    NAGIOS_STATUS_CRITICAL: 'CRITICAL',
> +    NAGIOS_STATUS_UNKNOWN: 'UNKNOWN',
> +}
> +
> +
> +def nagios_exit(status, message):
> +    assert status in NAGIOS_STATUS, "Invalid Nagios status code"
> +    # prefix status name to message

Unnecessary comment.

> +    output = '{}: {}'.format(NAGIOS_STATUS[status], message)
> +    print(output)  # nagios requires print to stdout, no stderr
> +    sys.exit(status)
> +
> +
> +def main():
> +    parser = argparse.ArgumentParser(
> +        description='check juju lint results',
> +        # show default in help

Unnecessary comment.

> +        formatter_class=argparse.ArgumentDefaultsHelpFormatter)
> +
> +    parser.add_argument(
> +        '-f', '--lint-results-file',
> +        type=argparse.FileType(mode='r'),
> +        dest='lint_results_file',
> +        default=LINT_RESULTS_FILE,
> +        help='juju lint results file to check, use `-` for stdin')
> +
> +    args = parser.parse_args()
> +    lint_results = args.lint_results_file.read()
> +
> +    if 'ERROR' in lint_results or 'CRITICAL' in lint_results:
> +        message = 'juju-lint errors\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_CRITICAL, message)
> +    elif 'WARNING' in lint_results:
> +        message = 'juju-lint warnings\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_WARNING, message)
> +    else:
> +        message = 'juju lint is happy\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_OK, message)
> +
> +
> +if __name__ == "__main__":
> +    main()


-- 
https://code.launchpad.net/~guoqiao/charm-juju-lint/+git/charm-juju-lint/+merge/377959
Your team Canonical BootStack Charmers is subscribed to branch charm-juju-lint:master.