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
> +
> +
> [email protected]_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)
> +
> +
> [email protected](scope='session')
> +async def controller():
> + """Connect to the current controller."""
> + controller = Controller()
> + await controller.connect_current()
> + yield controller
> + await controller.disconnect()
> +
> +
> [email protected](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)
> +
> +
> [email protected](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()
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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
> +
> +
> [email protected]
> +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 #
> +############
> [email protected](scope='session', params=SERIES)
> +def series(request):
> + """Return ubuntu version (i.e. xenial) in use in the test."""
> + return request.param
> +
> +
> [email protected](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}
> +
> +
> [email protected](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
> +
> +
> [email protected]()
> +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
> +
> +
> [email protected]()
> +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.
--
Mailing list: https://launchpad.net/~nagios-charmers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~nagios-charmers
More help : https://help.launchpad.net/ListHelp
