nagios-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Seems fine. Some inline comments and minor issues that should be fixed before landing.

Diff comments:

> diff --git a/Makefile b/Makefile
> index 20a6f9b..a5668bc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -9,26 +9,29 @@ 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

Personally, I put lint first, followed by unittest, because they run fastest. I understand that this comes from the template, so might be worth fixing there if you agree.

>  
>  unittest:
>  	@cd src && tox -e unit
>  
> +functional: build
> +ifeq ("$(wildcard $(JUJU_REPOSITORY)/tools.zip)","")
> +	$(error ERROR: You must save the tools resource in ${JUJU_REPOSITORY}/tools.zip before running functional tests)

Is there a download link for tools.zip, or a wiki page or doc on how to assemble it? The failure message needs to help a new charm contributor resolve the problem, even if it just tells the to look in the README.

I like how you did this in Makefile syntax rather than my usual approach of putting the if/then in the rule shell code.

> +else
> +	@cd src && tox -e functional

The latest release of the template moves things back out of src/ (and has a more complex command here). No need to change though, as updating things to match the latest version of that particular moving target should wait until later unless it solves some other problem.

> +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/conftest.py b/src/tests/functional/conftest.py
> new file mode 100644
> index 0000000..ea22194
> --- /dev/null
> +++ b/src/tests/functional/conftest.py
> @@ -0,0 +1,171 @@
> +#!/usr/bin/python3
> +'''
> +Reusable pytest fixtures for functional testing

This is all from the template. Not a problem for this branch, but we need to work out a better way of distributing this shared code (such as a pypi package, embedded via wheelhouse.txt), to avoid remaking the past mistake of embedded charm-helpers.

> +
> +Environment variables
> +---------------------
> +
> +test_preserve_model:
> +if set, the testing model won't be torn down at the end of the testing session
> +'''
> +
> +import asyncio
> +import json
> +import os
> +import uuid
> +import pytest
> +import juju
> +from juju.controller import Controller
> +from juju.errors import JujuError
> +
> +STAT_CMD = '''python3 - <<EOF
> +import json
> +import os
> +
> +s = os.stat('%s')
> +stat_hash = {
> +    'uid': s.st_uid,
> +    'gid': s.st_gid,
> +    'mode': oct(s.st_mode),
> +    'size': s.st_size
> +}
> +stat_json = json.dumps(stat_hash)
> +print(stat_json)
> +
> +EOF
> +'''
> +
> +
> +@pytest.yield_fixture(scope='module')
> +def event_loop():
> +    '''Override the default pytest event loop to allow for fixtures using a
> +    broader scope'''
> +    loop = asyncio.get_event_loop_policy().new_event_loop()
> +    asyncio.set_event_loop(loop)
> +    loop.set_debug(True)
> +    yield loop
> +    loop.close()
> +    asyncio.set_event_loop(None)
> +
> +
> +@pytest.fixture(scope='module')
> +async def controller():
> +    '''Connect to the current controller'''
> +    _controller = Controller()
> +    await _controller.connect_current()
> +    yield _controller
> +    await _controller.disconnect()
> +
> +
> +@pytest.fixture(scope='module')
> +async def model(controller):  # pylint: disable=redefined-outer-name
> +    '''This model lives only for the duration of the test'''
> +    model_name = "functest-{}".format(uuid.uuid4())
> +    _model = await controller.add_model(model_name)
> +    yield _model
> +    await _model.disconnect()
> +    if not os.getenv('test_preserve_model'):
> +        await controller.destroy_model(model_name)
> +        while model_name in await controller.list_models():
> +            await asyncio.sleep(1)
> +
> +
> +@pytest.fixture()
> +async def get_app(model):  # pylint: disable=redefined-outer-name
> +    '''Returns the application requested'''
> +    async def _get_app(name):
> +        try:
> +            return model.applications[name]
> +        except KeyError:
> +            raise JujuError("Cannot find application {}".format(name))
> +    return _get_app
> +
> +
> +@pytest.fixture()
> +async def get_unit(model):  # pylint: disable=redefined-outer-name
> +    '''Returns the requested <app_name>/<unit_number> unit'''
> +    async def _get_unit(name):
> +        try:
> +            (app_name, unit_number) = name.split('/')
> +            return model.applications[app_name].units[unit_number]
> +        except (KeyError, ValueError):
> +            raise JujuError("Cannot find unit {}".format(name))
> +    return _get_unit
> +
> +
> +@pytest.fixture()
> +async def get_entity(get_unit, get_app):  # pylint: disable=redefined-outer-name
> +    '''Returns a unit or an application'''
> +    async def _get_entity(name):
> +        try:
> +            return await get_unit(name)
> +        except JujuError:
> +            try:
> +                return await get_app(name)
> +            except JujuError:
> +                raise JujuError("Cannot find entity {}".format(name))
> +    return _get_entity
> +
> +
> +@pytest.fixture
> +async def run_command(get_unit):  # pylint: disable=redefined-outer-name
> +    '''
> +    Runs a command on a unit.
> +
> +    :param cmd: Command to be run
> +    :param target: Unit object or unit name string
> +    '''
> +    async def _run_command(cmd, target):
> +        unit = (
> +            target
> +            if isinstance(target, juju.unit.Unit)
> +            else await get_unit(target)
> +        )
> +        action = await unit.run(cmd)
> +        return action.results
> +    return _run_command
> +
> +
> +@pytest.fixture
> +async def file_stat(run_command):  # pylint: disable=redefined-outer-name
> +    '''
> +    Runs stat on a file
> +
> +    :param path: File path
> +    :param target: Unit object or unit name string
> +    '''
> +    async def _file_stat(path, target):
> +        cmd = STAT_CMD % path
> +        results = await run_command(cmd, target)
> +        return json.loads(results['Stdout'])
> +    return _file_stat
> +
> +
> +@pytest.fixture
> +async def file_contents(run_command):  # pylint: disable=redefined-outer-name
> +    '''
> +    Returns the contents of a file
> +
> +    :param path: File path
> +    :param target: Unit object or unit name string
> +    '''
> +    async def _file_contents(path, target):
> +        cmd = 'cat {}'.format(path)
> +        results = await run_command(cmd, target)
> +        return results['Stdout']
> +    return _file_contents
> +
> +
> +@pytest.fixture
> +async def reconfigure_app(get_app, model):  # pylint: disable=redefined-outer-name
> +    '''Applies a different config to the requested app'''
> +    async def _reconfigure_app(cfg, target):
> +        application = (
> +            target
> +            if isinstance(target, juju.application.Application)
> +            else await get_app(target)
> +        )
> +        await application.set_config(cfg)
> +        await application.get_config()
> +        await model.block_until(lambda: application.status == 'active')
> +    return _reconfigure_app
> diff --git a/src/tests/functional/test_hwhealth.py b/src/tests/functional/test_hwhealth.py
> new file mode 100644
> index 0000000..bf905c1
> --- /dev/null
> +++ b/src/tests/functional/test_hwhealth.py
> @@ -0,0 +1,183 @@
> +import os
> +import pytest
> +import sys
> +import subprocess
> +import asyncio
> +sys.path.append('lib')

Needs a blank line before and after the sys.path.append.

> +from hwhealth import hwdiscovery  # noqa: E402
> +from hwhealth import classes      # noqa: E402
> +

Two blank lines after the imports per pep8

> +# Treat all tests as coroutines
> +pytestmark = pytest.mark.asyncio
> +SERIES = ['xenial', 'bionic']
> +juju_repository = os.getenv('JUJU_REPOSITORY', '.').rstrip('/')
> +nrpecfg_dir = '/etc/nagios/nrpe.d'

juju_repository and nrpecfg_dir are module level constants and per pep8 should be ALL_CAPS.

> +
> +
> +###################
> +# Custom fixtures #
> +###################
> +
> +@pytest.fixture(scope='module',
> +                params=SERIES)

No need for this to be split over two lines; it would read better as a single line.

> +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

Or just manipulate the machine readable YAML file with Python, rather than continue the terrible habit of using HTML or text templating to generate structured YAML/JSON :-P

> +    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,
> +        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(os.getenv('JUJU_REPOSITORY'), 'tools.zip')

You setup the JUJU_REPOSITORY variable at the top of the module, so you probably should actually make use of it here, and again half a dozen lines down.

> +    subprocess.check_call([
> +        'juju',
> +        'deploy',
> +        '-m',
> +        model.info.name,
> +        os.path.join(os.getenv('JUJU_REPOSITORY'), 'builds', 'hw-health'),
> +        hw_health_app_name,
> +        '--resource',
> +        'tools={}'.format(tools_res_path),
> +    ])
> +    # 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')
> +    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.pop()

I don't understand when these fixtures get invoked. Are you sure .pop() is the right thing to do here, mutating deploy_app.units list?

> +
> +
> +@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'''

This isn't a well formed docstring. It reads more like a comment to me, so maybe it should be one. Docstring is just something like 'Return a tool class for use by tests'.

> +    with monkeypatch.context() as m:
> +        m.setattr('charmhelpers.core.hookenv.charm_dir',
> +                  lambda: os.getenv('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')
> +    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))

This would read better on a single line.

> +        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)

Single line

> +        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)

Single line.

> +            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 .

Can you ratchet down the complexity yet? 20 is pretty high, with 10 being what I see most people targetting.

> -


-- 
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.