cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Under
 https://code.launchpad.net/~mb-methot/cloud-init/+git/cloud-init/+merge/326017
I pointed to http://paste.ubuntu.com/25069502/
which showed why the fix there was insufficient.
if you've *not* covered what those unit tests cover, please add them.

over all, looks good, some comments inline.

Diff comments:

> diff --git a/cloudinit/config/cc_puppet.py b/cloudinit/config/cc_puppet.py
> index dc11561..28b1d56 100644
> --- a/cloudinit/config/cc_puppet.py
> +++ b/cloudinit/config/cc_puppet.py
> @@ -43,12 +45,13 @@ yaml notation).
>          install: <true/false>
>          version: <version>
>          conf:
> -            server: "puppetmaster.example.org"
> -            certname: "%i.%f"
> -            ca_cert: |
> -                -------BEGIN CERTIFICATE-------
> -                <cert data>
> -                -------END CERTIFICATE-------
> +            agent:
> +                server: "puppetmaster.example.org"
> +                certname: "%i.%f"
> +                ca_cert: |
> +                    -------BEGIN CERTIFICATE-------
> +                    <cert data>
> +                    -------END CERTIFICATE-------

so this was simply wrong/broken previously, right?

>  """
>  
>  from six import StringIO
> diff --git a/cloudinit/helpers.py b/cloudinit/helpers.py
> index f01021a..bda40bb 100644
> --- a/cloudinit/helpers.py
> +++ b/cloudinit/helpers.py
> @@ -441,12 +441,12 @@ class DefaultingConfigParser(RawConfigParser):
>  
>      def stringify(self, header=None):
>          contents = ''
> -        with six.StringIO() as outputstream:
> -            self.write(outputstream)
> -            outputstream.flush()
> -            contents = outputstream.getvalue()
> -            if header:
> -                contents = "\n".join([header, contents])
> +        outputstream = StringIO()
> +        self.write(outputstream)
> +        outputstream.flush()
> +        contents = outputstream.getvalue()
> +        if header:
> +            contents = "\n".join([header, contents])

do we need a trailing '\n' there?
  contents = "\n".join([header, contents, ""])

>          return contents
>  
>  # vi: ts=4 expandtab
> 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')

os.path.join(self.new_root, 'default/landscape') == self.tmp_path('default/landscape', self.new_root)

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

i dont like excessive indentation.
thats why wrap_and_call exists, do do things very similar to this.

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


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