nagios-charmers team mailing list archive

[Chris Sanders <chris.sanders@xxxxxxxxxxxxx>]

Review: Approve

Adding tests / lint / etc is a clear improvement. A few nit comments that you can take or leave I wouldn't block the merge on it.

Additionally, and I believe this is out of scope for this change, but you've got several fixtures and generic test support stuff built in here that should really be considered for an external module. That would make it easier for future test and would help maintain a consistent testing setup so that each charm isn't defining it's own slightly different versions of things like setting/resetting config for a specific test.

Diff comments:

> diff --git a/hooks/monitors-relation-changed b/hooks/monitors_relation_changed.py
> similarity index 86%
> rename from hooks/monitors-relation-changed
> rename to hooks/monitors_relation_changed.py
> index 13cb96c..bd37812 100755
> --- a/hooks/monitors-relation-changed
> +++ b/hooks/monitors_relation_changed.py
> @@ -103,21 +108,22 @@ def main(argv):
>      os.system('service nagios3 reload')
>  
>  
> -def apply_relation_config(relid, units, all_hosts):
> +def apply_relation_config(relid, units, all_hosts):   # noqa: C901
>      for unit, relation_settings in units.iteritems():
>          monitors = relation_settings['monitors']
>          target_id = relation_settings['target-id']
>          machine_id = relation_settings.get('machine_id', None)
>          parent_host = None
>          if machine_id:
> -            container_regex = re.compile("(\d*)/lx[cd]/\d*")
> +            container_regex = re.compile(r"(\d+)/lx[cd]/\d+")
>              if container_regex.search(machine_id):
>                  parent_machine = container_regex.search(machine_id).group(1)
>                  if parent_machine in all_hosts:
>                      parent_host = all_hosts[parent_machine]
>  
>          # If not set, we don't mess with it, as multiple services may feed
> -        # monitors in for a particular address. Generally a primary will set this
> +        # monitors in for a particular address. Generally a primary will set
> +        # this
>          # to its own private-address

Nit: After the line wrap you can move 209 up into 208 as well.

>          target_address = relation_settings.get('target-address', None)
>  
> diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
> new file mode 100644
> index 0000000..fafe7f0
> --- /dev/null
> +++ b/tests/functional/conftest.py
> @@ -0,0 +1,290 @@
> +#!/usr/bin/python3
> +
> +import asyncio
> +import json
> +import os
> +import uuid
> +
> +import juju
> +from juju.controller import Controller
> +from juju.errors import JujuError
> +from juju.model import Model
> +
> +import pytest
> +
> +
> +STAT_FILE = "python3 -c \"import json; import os; s=os.stat('%s'); print(json.dumps({'uid': s.st_uid, 'gid': s.st_gid, 'mode': oct(s.st_mode), 'size': s.st_size}))\""  # noqa: E501
> +
> +
> +@pytest.yield_fixture(scope='session')
> +def event_loop(request):
> +    """Override the default pytest event loop to allow for broaded scopedv fixtures."""
> +    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='session')
> +async def controller():
> +    """Connect to the current controller."""
> +    controller = Controller()
> +    await controller.connect_current()
> +    yield controller
> +    await controller.disconnect()
> +
> +
> +@pytest.fixture(scope='session')
> +async def model(controller):
> +    """Create a model that 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 os.getenv('PYTEST_KEEP_MODEL'):
> +        return
> +    await controller.destroy_model(model_name)
> +    while model_name in await controller.list_models():
> +        await asyncio.sleep(1)
> +
> +
> +@pytest.fixture(scope='session')
> +async def current_model():
> +    """Return the current model, does not create or destroy it."""
> +    model = Model()
> +    await model.connect_current()
> +    yield model
> +    await model.disconnect()
> +
> +
> +@pytest.fixture
> +async def get_app(model):
> +    """Return 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):
> +    """Return 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(model, get_unit, get_app):
> +    """Return 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):
> +    """
> +    Run 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 type(target) is 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):
> +    """
> +    Run stat on a file.
> +
> +    :param path: File path
> +    :param target: Unit object or unit name string
> +    """
> +    async def _file_stat(path, target):
> +        cmd = STAT_FILE % path
> +        results = await run_command(cmd, target)
> +        return json.loads(results['Stdout'])
> +    return _file_stat
> +
> +
> +@pytest.fixture
> +async def file_contents(run_command):
> +    """
> +    Return 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):
> +    """Apply a different config to the requested app."""
> +    async def _reconfigure_app(cfg, target):
> +        application = (
> +            target
> +            if type(target) is 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
> +
> +
> +@pytest.fixture
> +async def create_group(run_command):
> +    """Create the UNIX group specified."""
> +    async def _create_group(group_name, target):
> +        cmd = "sudo groupadd %s" % group_name
> +        await run_command(cmd, target)
> +    return _create_group
> +
> +
> +pytestmark = pytest.mark.asyncio
> +
> +CHARM_BUILD_DIR = os.getenv("CHARM_BUILD_DIR", "..").rstrip("/")
> +
> +SERIES = [
> +    "trusty",
> +    "xenial",
> +    "bionic",
> +]
> +
> +
> +############
> +# FIXTURES #
> +############
> +@pytest.fixture(scope='session', params=SERIES)
> +def series(request):
> +    """Return ubuntu version (i.e. xenial) in use in the test."""
> +    return request.param
> +
> +
> +@pytest.fixture(scope='session')
> +async def relatives(model):
> +    nrpe = "nrpe"
> +    nrpe_app = await model.deploy(
> +        'cs:' + nrpe, application_name=nrpe,
> +        series='trusty', config={},
> +        num_units=0
> +    )
> +
> +    mysql = "mysql"
> +    mysql_app = await model.deploy(
> +        'cs:' + mysql, application_name=mysql,
> +        series='trusty', config={}
> +    )
> +
> +    mediawiki = "mediawiki"
> +    mediawiki_app = await model.deploy(
> +        'cs:' + mediawiki, application_name=mediawiki,
> +        series='trusty', config={}
> +    )
> +
> +    await model.add_relation('mysql:db', 'mediawiki:db')
> +    await model.add_relation('mysql:juju-info', 'nrpe:general-info')
> +    await model.add_relation('mediawiki:juju-info', 'nrpe:general-info')
> +    await model.block_until(
> +        lambda: all(_.status == "active" for _ in (mysql_app, mediawiki_app))
> +    )
> +
> +    yield {mediawiki: mediawiki_app, mysql: mysql_app, nrpe: nrpe_app}
> +
> +
> +@pytest.fixture(scope='session')
> +async def deploy_app(relatives, model, series):
> +    """Return application of the charm under test."""
> +    app_name = "nagios-{}".format(series)
> +
> +    """Deploy the nagios app."""
> +    nagios_app = await model.deploy(
> +        os.path.join(CHARM_BUILD_DIR, 'nagios'),
> +        application_name=app_name,
> +        series=series,
> +        config={
> +            'enable_livestatus': False,
> +            'ssl': False,
> +            'extraconfig': '',
> +            'enable_pagerduty': False
> +        }
> +    )
> +    await model.add_relation('{}:monitors'.format(app_name), 'mysql:monitors')
> +    await model.add_relation('{}:nagios'.format(app_name), 'mediawiki:juju-info')
> +    await model.add_relation('nrpe:monitors', '{}:monitors'.format(app_name))
> +    await model.block_until(lambda: nagios_app.status == "active")
> +    await model.block_until(lambda: all(
> +            _.status == "active"
> +            for _ in list(relatives.values()) + [nagios_app]
> +    ))
> +    yield nagios_app
> +    if os.getenv('PYTEST_KEEP_MODEL'):
> +        return
> +    await nagios_app.destroy()

I don't know that this is buying you anything. The model on destruction will delete all applications no need to delete them individually.

> +
> +
> +class Agent:
> +    def __init__(self, unit, application):
> +        self.u = unit
> +        self.application = application
> +        self.model = unit.model
> +
> +    def is_active(self, status):
> +        u = self.u
> +        return u.agent_status == status and u.workload_status == "active"
> +
> +    async def block_until_or_timeout(self, lambda_f, **kwargs):
> +        await self.block_until(lambda_f, ignore_timeout=True, **kwargs)
> +
> +    async def block_until(self, lambda_f, timeout=120, wait_period=5,
> +                          ignore_timeout=False):
> +        try:
> +            await self.model.block_until(
> +                lambda_f, timeout=timeout, wait_period=wait_period
> +            )
> +        except asyncio.TimeoutError:
> +            if not ignore_timeout:
> +                raise
> +
> +
> +@pytest.fixture()
> +async def unit(model, deploy_app):
> +    """Return the unit we've deployed."""
> +    unit = Agent(deploy_app.units[0], deploy_app)
> +    await unit.block_until(lambda: unit.is_active('idle'))
> +    return unit
> +
> +
> +@pytest.fixture()
> +async def auth(file_contents, unit):
> +    """Return the basic auth credentials."""
> +    nagiospwd = await file_contents("/var/lib/juju/nagios.passwd", unit.u)
> +    return 'nagiosadmin', nagiospwd.strip()


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