cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Nice work.  Couple of questions inline.

Diff comments:

> diff --git a/tests/unittests/test_handler/test_handler_landscape.py b/tests/unittests/test_handler/test_handler_landscape.py
> new file mode 100644
> index 0000000..d35ba60
> --- /dev/null
> +++ b/tests/unittests/test_handler/test_handler_landscape.py
> @@ -0,0 +1,129 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from cloudinit.config import cc_landscape
> +from cloudinit.sources import DataSourceNone
> +from cloudinit import (distros, helpers, cloud, util)
> +from ..helpers import FilesystemMockingTestCase, mock
> +
> +from configobj import ConfigObj
> +import logging
> +import os
> +
> +
> +LOG = logging.getLogger(__name__)
> +
> +
> +class TestLandscape(FilesystemMockingTestCase):
> +
> +    with_logs = True
> +
> +    def setUp(self):
> +        super(TestLandscape, self).setUp()
> +        self.new_root = self.tmp_dir()
> +        self.conf = os.path.join(self.new_root, 'client.conf')
> +        self.default_file = os.path.join(self.new_root, 'default/landscape')

If new_root is meant to be a '/' then I would expect conf to be: os.path.join(self.new_root, 'etc', 'landscape', 'client.conf')
and default_file to be: os.path.join(self.new_root, 'etc', 'default', 'landscape')

> +
> +    def _get_cloud(self, distro):
> +        self.patchUtils(self.new_root)
> +        paths = helpers.Paths({'templates_dir': self.new_root})
> +        cls = distros.fetch(distro)
> +        mydist = cls(distro, {}, paths)
> +        myds = DataSourceNone.DataSourceNone({}, mydist, paths)
> +        return cloud.Cloud(myds, paths, {}, mydist, None)
> +
> +    def test_handler_skips_empty_landscape_cloudconfig(self):
> +        """Empty landscape cloud-config section does no work."""
> +        mycloud = self._get_cloud('ubuntu')
> +        mycloud.distro = mock.MagicMock()
> +        cfg = {'landscape': {}}
> +        cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertFalse(mycloud.distro.install_packages.called)
> +
> +    def test_handler_error_on_invalid_landscape_type(self):
> +        """Raise an error when landscape configuraiton option is invalid."""
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'landscape': 'wrongtype'}
> +        with self.assertRaises(RuntimeError) as context_manager:
> +            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertIn(
> +            "'landscape' key existed in config, but not a dict",
> +            str(context_manager.exception))
> +
> +    @mock.patch('cloudinit.config.cc_landscape.util')
> +    def test_handler_restarts_landscape_client(self, m_util):
> +        """handler restarts lansdscape-client after install."""
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'landscape': {'client': {}}}
> +        with mock.patch('cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE',
> +                        self.conf):
> +                cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(
> +            [mock.call(['service', 'landscape-client', 'restart'])],
> +            m_util.subp.call_args_list)
> +
> +    def test_handler_installs_client_and_creates_config_file(self):
> +        """Write landscape client.conf and install landscape-client."""
> +        self.assertFalse(os.path.exists(self.conf))

Are we not tearing down the tmp_dir between tests?  This seems unnecessary.

> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'landscape': {'client': {}}}
> +        expected = {'client': {
> +            'log_level': 'info',
> +            'url': 'https://landscape.canonical.com/message-system',
> +            'ping_url': 'http://landscape.canonical.com/ping',
> +            'data_path': '/var/lib/landscape/client'}}
> +        mycloud.distro = mock.MagicMock()
> +        with mock.patch('cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE',
> +                        self.conf):
> +            with mock.patch('cloudinit.config.cc_landscape.LS_DEFAULT_FILE',
> +                            self.default_file):
> +                cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(
> +            [mock.call('landscape-client')],
> +            mycloud.distro.install_packages.call_args)
> +        self.assertEqual(expected, dict(ConfigObj(self.conf)))
> +        self.assertIn(
> +            'Wrote landscape config file to {0}'.format(self.conf),
> +            self.logs.getvalue())
> +        with open(self.default_file) as stream:
> +            default_content = stream.read()
> +        self.assertEqual('RUN=1\n', default_content)
> +
> +    def test_handler_writes_merged_client_config_file_with_defaults(self):
> +        """Merge and write options from LSC_CLIENT_CFG_FILE with defaults."""
> +        # Write existing sparse client.conf file
> +        util.write_file(self.conf, '[client]\ncomputer_title = My PC\n')
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'landscape': {'client': {}}}
> +        expected = {'client': {
> +            'log_level': 'info',
> +            'url': 'https://landscape.canonical.com/message-system',
> +            'ping_url': 'http://landscape.canonical.com/ping',
> +            'data_path': '/var/lib/landscape/client',
> +            'computer_title': 'My PC'}}
> +        with mock.patch("cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE",
> +                        self.conf):
> +            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(expected, dict(ConfigObj(self.conf)))
> +        self.assertIn(
> +            'Wrote landscape config file to {0}'.format(self.conf),
> +            self.logs.getvalue())
> +
> +    def test_handler_writes_merged_provided_cloudconfig_with_defaults(self):
> +        """Merge and write options from cloud-config options with defaults."""
> +        # Write empty sparse client.conf file
> +        util.write_file(self.conf, '')
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'landscape': {'client': {'computer_title': 'My PC'}}}
> +        expected = {'client': {
> +            'log_level': 'info',
> +            'url': 'https://landscape.canonical.com/message-system',
> +            'ping_url': 'http://landscape.canonical.com/ping',
> +            'data_path': '/var/lib/landscape/client',
> +            'computer_title': 'My PC'}}
> +        with mock.patch("cloudinit.config.cc_landscape.LSC_CLIENT_CFG_FILE",
> +                        self.conf):
> +            cc_landscape.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(expected, dict(ConfigObj(self.conf)))
> +        self.assertIn(
> +            'Wrote landscape config file to {0}'.format(self.conf),
> +            self.logs.getvalue())
> diff --git a/tests/unittests/test_handler/test_handler_puppet.py b/tests/unittests/test_handler/test_handler_puppet.py
> new file mode 100644
> index 0000000..805c76b
> --- /dev/null
> +++ b/tests/unittests/test_handler/test_handler_puppet.py
> @@ -0,0 +1,142 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from cloudinit.config import cc_puppet
> +from cloudinit.sources import DataSourceNone
> +from cloudinit import (distros, helpers, cloud, util)
> +from ..helpers import CiTestCase, mock
> +
> +import logging
> +
> +
> +LOG = logging.getLogger(__name__)
> +
> +
> +@mock.patch('cloudinit.config.cc_puppet.util')
> +@mock.patch('cloudinit.config.cc_puppet.os')

neat, I didn't know you could decorate a class with mock.patch;  seems like that might cause issue for non test_ methods, but maybe mock is smart?

> +class TestAutostartPuppet(CiTestCase):
> +
> +    with_logs = True
> +
> +    def test_wb_autostart_puppet_updates_puppet_default(self, m_os, m_util):
> +        """Update /etc/default/puppet to autostart if it exists."""
> +
> +        def _fake_exists(path):
> +            return path == '/etc/default/puppet'
> +
> +        m_os.path.exists.side_effect = _fake_exists
> +        cc_puppet._autostart_puppet(LOG)
> +        self.assertEqual(
> +            [mock.call(['sed', '-i', '-e', 's/^START=.*/START=yes/',
> +                        '/etc/default/puppet'], capture=False)],
> +            m_util.subp.call_args_list)
> +
> +    def test_wb_autostart_pupppet_enables_puppet_systemctl(self, m_os, m_util):
> +        """If systemctl is present, enable puppet via systemctl."""
> +
> +        def _fake_exists(path):
> +            return path == '/bin/systemctl'
> +
> +        m_os.path.exists.side_effect = _fake_exists
> +        cc_puppet._autostart_puppet(LOG)
> +        expected_calls = [mock.call(
> +            ['/bin/systemctl', 'enable', 'puppet.service'], capture=False)]
> +        self.assertEqual(expected_calls, m_util.subp.call_args_list)
> +
> +    def test_wb_autostart_pupppet_enables_puppet_chkconfig(self, m_os, m_util):
> +        """If chkconfig is present, enable puppet via checkcfg."""
> +
> +        def _fake_exists(path):
> +            return path == '/sbin/chkconfig'
> +
> +        m_os.path.exists.side_effect = _fake_exists
> +        cc_puppet._autostart_puppet(LOG)
> +        expected_calls = [mock.call(
> +            ['/sbin/chkconfig', 'puppet', 'on'], capture=False)]
> +        self.assertEqual(expected_calls, m_util.subp.call_args_list)
> +
> +
> +@mock.patch('cloudinit.config.cc_puppet._autostart_puppet')
> +class TestPuppetHandle(CiTestCase):
> +
> +    with_logs = True
> +
> +    def setUp(self):
> +        super(TestPuppetHandle, self).setUp()
> +        self.new_root = self.tmp_dir()
> +        self.conf = self.tmp_path('puppet.conf')
> +
> +    def _get_cloud(self, distro):
> +        paths = helpers.Paths({'templates_dir': self.new_root})
> +        cls = distros.fetch(distro)
> +        mydist = cls(distro, {}, paths)
> +        myds = DataSourceNone.DataSourceNone({}, mydist, paths)
> +        return cloud.Cloud(myds, paths, {}, mydist, None)
> +
> +    def test_handler_skips_missing_puppet_key_in_cloudconfig(self, m_auto):
> +        """Cloud-config containing no 'puppet' key is skipped."""
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {}
> +        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertIn(
> +            "no 'puppet' configuration found", self.logs.getvalue())
> +        self.assertEqual(0, m_auto.call_count)
> +
> +    @mock.patch('cloudinit.config.cc_puppet.util.subp')
> +    def test_handler_puppet_config_starts_puppet_service(self, m_subp, m_auto):
> +        """Cloud-config 'puppet' configuration starts puppet."""
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {'puppet': {'install': False}}
> +        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(1, m_auto.call_count)
> +        self.assertEqual(
> +            [mock.call(['service', 'puppet', 'start'], capture=False)],
> +            m_subp.call_args_list)
> +
> +    @mock.patch('cloudinit.config.cc_puppet.util.subp')
> +    def test_handler_empty_puppet_config_installs_puppet(self, m_subp, m_auto):
> +        """Cloud-config empty 'puppet' configuration installs latest puppet."""
> +        mycloud = self._get_cloud('ubuntu')
> +        mycloud.distro = mock.MagicMock()
> +        cfg = {'puppet': {}}
> +        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(
> +            [mock.call(('puppet', None))],
> +            mycloud.distro.install_packages.call_args_list)
> +
> +    @mock.patch('cloudinit.config.cc_puppet.util.subp')
> +    def test_handler_puppet_config_installs_puppet_on_true(self, m_subp, _):
> +        """Cloud-config with 'puppet' key installs when 'install' is True."""
> +        mycloud = self._get_cloud('ubuntu')
> +        mycloud.distro = mock.MagicMock()
> +        cfg = {'puppet': {'install': True}}
> +        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(
> +            [mock.call(('puppet', None))],
> +            mycloud.distro.install_packages.call_args_list)
> +
> +    @mock.patch('cloudinit.config.cc_puppet.util.subp')
> +    def test_handler_puppet_config_installs_puppet_version(self, m_subp, _):
> +        """Cloud-config 'puppet' configuration can specify a version."""
> +        mycloud = self._get_cloud('ubuntu')
> +        mycloud.distro = mock.MagicMock()
> +        cfg = {'puppet': {'version': '3.8'}}
> +        cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        self.assertEqual(
> +            [mock.call(('puppet', '3.8'))],
> +            mycloud.distro.install_packages.call_args_list)
> +
> +    @mock.patch('cloudinit.config.cc_puppet.util.subp')
> +    def test_handler_puppet_config_updates_puppet_conf(self, m_subp, m_auto):
> +        """When 'conf' is provided update values in PUPPET_CONF_PATH."""
> +        mycloud = self._get_cloud('ubuntu')
> +        cfg = {
> +            'puppet': {
> +                'conf': {'agent': {'server': 'puppetmaster.example.org'}}}}
> +        util.write_file(self.conf, '[agent]\nserver = origpuppet\nother = 3')
> +        puppet_conf_path = 'cloudinit.config.cc_puppet.PUPPET_CONF_PATH'
> +        mycloud.distro = mock.MagicMock()
> +        with mock.patch(puppet_conf_path, self.conf):
> +            cc_puppet.handle('notimportant', cfg, mycloud, LOG, None)
> +        content = util.load_file(self.conf)
> +        expected = '[agent]\nserver = puppetmaster.example.org\nother = 3\n\n'
> +        self.assertEqual(expected, content)


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/329087
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:cc-landscape-py3-config-fix into cloud-init:master.