nagios-charmers team mailing list archive

[Alvaro Uría <alvaro.uria@xxxxxxxxxxxxx>]

Please find some comments below.

Diff comments:

> diff --git a/Makefile b/Makefile
> index 20a6f9b..d99dca3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -9,26 +10,31 @@ help:
>  	@echo " make help - show this text"
>  	@echo " make lint - use pre-commit to ensure consistent layout"
>  	@echo " make test - run the functional test, unittests and lint"
> -	@echo " make unittest - run the tests defined in the unittest "\
> -		"subdirectory"
> -	@echo " make functionaltest - run the tests defined in the "\
> -		"functional subdirectory"
> +	@echo " make unittest - run the tests defined in the unittest subdirectory"
> +	@echo " make functional - run the tests defined in the functional subdirectory"
>  	@echo " make release - build the charm"
>  	@echo " make clean - remove unneeded files"
>  	@echo ""
> -	@echo " the following targets are meant to be used by the Makefile"
> -	@echo " make requirements - installs the requirements"
>  
>  lint:
>  	@echo "Running flake8"
>  	cd src && tox -e pep8
>  
> -
> -test: unittest lint
> +test: unittest functional lint

Same comment about passing lint target first.

>  
>  unittest:
>  	@cd src && tox -e unit
>  
> +functional: resource_check build
> +	@cd src && tox -e functional
> +
> +resource_check:
> +ifeq ("$(wildcard $(JUJU_REPOSITORY)/tools.zip)","")
> +	$(error ERROR: You must save the tools resource in ${JUJU_REPOSITORY}/tools.zip \
> +	  before running functional tests. \
> +	  Check the README for instructions on how to do so)
> +endif
> +
>  build:
>  	@echo "Building charm to base directory $(JUJU_REPOSITORY)"
>  	@git show -s --format="commit-sha-1: %H%ncommit-short: %h" > ./src/repo-info
> diff --git a/src/tests/functional/test_hwhealth.py b/src/tests/functional/test_hwhealth.py
> new file mode 100644
> index 0000000..e4c8a37
> --- /dev/null
> +++ b/src/tests/functional/test_hwhealth.py
> @@ -0,0 +1,189 @@
> +import os
> +import pytest
> +import sys
> +import subprocess
> +import asyncio
> +
> +sys.path.append('lib')
> +
> +from hwhealth import hwdiscovery  # noqa: E402
> +from hwhealth import classes      # noqa: E402
> +
> +
> +# Treat all tests as coroutines
> +pytestmark = pytest.mark.asyncio
> +SERIES = ['xenial', 'bionic']
> +JUJU_REPOSITORY = os.getenv('JUJU_REPOSITORY', '.')
> +NRPECFG_DIR = '/etc/nagios/nrpe.d'
> +
> +
> +###################
> +# Custom fixtures #
> +###################
> +
> +@pytest.fixture(scope='module',
> +                params=SERIES)
> +async def deploy_app(request, model):
> +    '''Deploys the hw-health charm as a subordinate of ubuntu'''
> +    # TODO: this might look nicer if we deployed a bundle instead. It could be
> +    # a jinja template to handle the parametrization
> +    release = request.param
> +    channel = 'stable'
> +    hw_health_app_name = 'hw-health-' + release
> +
> +    for principal_app in ['ubuntu', 'nagios']:
> +        await model.deploy(
> +            principal_app,
> +            application_name='{}-{}'.format(principal_app, release),
> +            series=release,
> +            channel=channel,
> +        )
> +    nrpe_app = await model.deploy(
> +        'nrpe',
> +        application_name='nrpe-' + release,

Minor suggestion: use 'nrpe-{}'.format(release)

> +        series=release,
> +        num_units=0,
> +        channel=channel,
> +    )
> +    await nrpe_app.add_relation(
> +        'general-info',
> +        'ubuntu-{}:juju-info'.format(release)
> +    )
> +    await nrpe_app.add_relation(
> +        'monitors',
> +        'nagios-{}:monitors'.format(release)
> +    )
> +
> +    # Attaching resources is not implemented yet in libjuju
> +    # see https://github.com/juju/python-libjuju/issues/294
> +    tools_res_path = os.path.join(JUJU_REPOSITORY, 'tools.zip')
> +    subprocess.check_call([
> +        'juju',
> +        'deploy',
> +        '-m',
> +        model.info.name,
> +        os.path.join(JUJU_REPOSITORY, 'builds', 'hw-health'),
> +        hw_health_app_name,
> +        '--resource',
> +        'tools={}'.format(tools_res_path),
> +    ])

Why not: await model.deploy(os.path.join(JUJU_REPOSITORY, 'builds', 'hw-health'), series=series, resources={'tools': tools_res_path}, application_name='hw-health-{}'.format(series))
According to the following link, the above is supported:
https://github.com/juju/python-libjuju/blob/master/juju/model.py#L1214

> +    # This is pretty horrible, but we can't deploy via libjuju
> +    while True:
> +        try:
> +            hw_health_app = model.applications[hw_health_app_name]
> +            break
> +        except KeyError:
> +            await asyncio.sleep(5)
> +
> +    await hw_health_app.add_relation(
> +        'general-info',
> +        'ubuntu-{}:juju-info'.format(release)
> +    )
> +    await hw_health_app.add_relation(
> +        'nrpe-external-master',
> +        nrpe_app.name + ':nrpe-external-master'
> +    )
> +    # The app will initially be in blocked state because it's running in a
> +    # container
> +    await model.block_until(
> +        lambda: hw_health_app.status == 'blocked',
> +        timeout=600
> +    )
> +    yield hw_health_app
> +
> +
> +@pytest.fixture(scope='module')
> +async def deployed_unit(deploy_app):
> +    '''Returns the hw-health unit we've deployed'''
> +    return deploy_app.units[0]
> +
> +
> +@pytest.fixture(scope='function')
> +async def tools(monkeypatch):
> +    # All tool classes know which files should be installed and how, so we can
> +    # use them to read the expected stat results. Monkeypatching is however
> +    # required as the classes code is not expected to be run outside of a
> +    # deployed charm
> +    with monkeypatch.context() as m:
> +        m.setattr('charmhelpers.core.hookenv.charm_dir',
> +                  lambda: JUJU_REPOSITORY)
> +        m.setattr('charmhelpers.core.hookenv.config',
> +                  lambda x=None: dict())
> +        m.setattr('charmhelpers.contrib.charmsupport.nrpe.get_nagios_hostname',
> +                  lambda: 'pytest')
> +        return hwdiscovery.get_tools('test')
> +
> +#########
> +# Tests #
> +#########
> +
> +
> +async def test_cannot_run_in_container(deploy_app):
> +    assert deploy_app.status == 'blocked'
> +
> +
> +async def test_forced_deploy(deploy_app, model):
> +    await deploy_app.set_config({'manufacturer': 'test'})
> +    await model.block_until(
> +        lambda: deploy_app.status == 'active',
> +        timeout=600
> +    )
> +    assert deploy_app.status == 'active'
> +
> +
> +async def test_stats(tools, deployed_unit, file_stat):
> +    # This should really be a parametrized test, but fixtures cannot be used as
> +    # params value as if they were iterators
> +    # It should also check for other installed files and differentiate between
> +    # tool types (e.g. IpmiTool does not use a vendor binary)
> +    for tool in tools:
> +        # Have we rendered the nrpe check cfg?
> +        nrpecfg_path = os.path.join(NRPECFG_DIR, 'check_{}.cfg'.format(tool._shortname))
> +        print('Checking {}'.format(nrpecfg_path))
> +        test_stat = await file_stat(nrpecfg_path, deployed_unit)
> +        assert test_stat['size'] > 0
> +
> +        # Have we installed the nrpe check script?
> +        nrpescript_path = os.path.join(tool.NRPE_PLUGINS_DIR, tool._nrpe_script)
> +        print('Checking {}'.format(nrpescript_path))
> +        test_stat = await file_stat(nrpescript_path, deployed_unit)
> +        assert test_stat['size'] > 0
> +        assert test_stat['gid'] == tool.NRPE_PLUGINS_GID
> +        assert test_stat['uid'] == tool.NRPE_PLUGINS_UID
> +        assert test_stat['mode'] == oct(tool.NRPE_PLUGINS_MODE)
> +        if isinstance(tool, classes.IpmiTool):
> +            # Have we added sudo rights for running freeipmi commands?
> +            sudoer_path = os.path.join(tool.SUDOERS_DIR, tool._sudoer_file)
> +            print('Checking {}'.format(sudoer_path))
> +            test_stat = await file_stat(sudoer_path, deployed_unit)
> +            assert test_stat['size'] > 0
> +            assert test_stat['gid'] == tool.SUDOERS_GID
> +            assert test_stat['uid'] == tool.SUDOERS_UID
> +            assert test_stat['mode'] == oct(tool.SUDOERS_MODE)
> +        elif isinstance(tool, classes.VendorTool):
> +            # Have we installed the cronjob script helper?
> +            cronjob_script_path = os.path.join(tool.NRPE_PLUGINS_DIR, tool._cronjob_script)
> +            print('Checking {}'.format(cronjob_script_path))
> +            test_stat = await file_stat(cronjob_script_path, deployed_unit)
> +            assert test_stat['size'] > 0
> +            assert test_stat['gid'] == tool.CRONJOB_SCRIPT_GID
> +            assert test_stat['uid'] == tool.CRONJOB_SCRIPT_UID
> +            assert test_stat['mode'] == oct(tool.CRONJOB_SCRIPT_MODE)
> +
> +            # Have we installed the cronjob itself?
> +            cronjob_path = os.path.join(tool.CROND_DIR, tool._shortname)
> +            print('Checking {}'.format(cronjob_path))
> +            test_stat = await file_stat(cronjob_path, deployed_unit)
> +            assert test_stat['size'] > 0
> +
> +            # Have we installed the vendor binary?
> +            if isinstance(tool, classes.MdadmTool):
> +                bin_path = os.path.join('/sbin', tool._shortname)
> +            else:
> +                bin_path = os.path.join(tool.TOOLS_DIR, tool._shortname)
> +            print('Checking {}'.format(bin_path))
> +            test_stat = await file_stat(bin_path, deployed_unit)
> +            assert test_stat['size'] > 0
> +            assert test_stat['gid'] == tool.TOOLS_GID
> +            assert test_stat['uid'] == tool.TOOLS_UID
> +            assert test_stat['mode'] == oct(tool.TOOLS_MODE)
> diff --git a/src/tox.ini b/src/tox.ini
> index 04b6de8..77597a0 100644
> --- a/src/tox.ini
> +++ b/src/tox.ini
> @@ -12,9 +12,17 @@ commands =
>    {toxworkdir}/../tests/download_nagios_plugin3.py
>    nosetests tests/unit
>  
> +[testenv:functional]
> +passenv =
> +  HOME
> +  JUJU_REPOSITORY
> +  PATH
> +commands = pytest -v --ignore {toxinidir}/tests/unit
> +deps = -r{toxinidir}/tests/functional/requirements.txt
> +       -r{toxinidir}/requirements.txt
> +
>  [testenv:pep8]
>  basepython = python3
>  deps =
>    flake8
>  commands = flake8 {posargs} --max-complexity=20 --max-line-length=120 .

Same comment about changing the complexity to 10.

> -


-- 
https://code.launchpad.net/~aieri/hw-health-charm/+git/hw-health-charm/+merge/364758
Your team Nagios Charm developers is subscribed to branch hw-health-charm:oo-rewrite-integration.