cloud-init-dev team mailing list archive

[Shraddha Pandhe <shraddha.pandhe@xxxxxxxxx>]

Review: Needs Information



Diff comments:

> === modified file 'cloudinit/config/cc_debug.py'
> --- cloudinit/config/cc_debug.py	2014-01-23 19:28:59 +0000
> +++ cloudinit/config/cc_debug.py	2014-10-18 16:32:48 +0000
> @@ -14,10 +14,13 @@
>  #    You should have received a copy of the GNU General Public License
>  #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> +import copy
> +from StringIO import StringIO
> +
>  from cloudinit import type_utils
>  from cloudinit import util
> -import copy
> -from StringIO import StringIO
> +
> +SKIP_KEYS = frozenset(['log_cfgs'])
>  
>  
>  def _make_header(text):
> @@ -31,6 +34,11 @@
>      return header.getvalue()
>  
>  
> +def _dumps(obj):
> +    text = util.yaml_dumps(obj, explicit_start=False, explicit_end=False)
> +    return text.rstrip()
> +
> +
>  def handle(name, cfg, cloud, log, args):
>      verbose = util.get_cfg_by_path(cfg, ('debug', 'verbose'), default=True)
>      if args:
> @@ -46,7 +54,7 @@
>          return
>      # Clean out some keys that we just don't care about showing...
>      dump_cfg = copy.deepcopy(cfg)
> -    for k in ['log_cfgs']:
> +    for k in SKIP_KEYS:
>          dump_cfg.pop(k, None)
>      all_keys = list(dump_cfg.keys())
>      for k in all_keys:
> @@ -55,10 +63,10 @@
>      # Now dump it...
>      to_print = StringIO()
>      to_print.write(_make_header("Config"))
> -    to_print.write(util.yaml_dumps(dump_cfg))
> +    to_print.write(_dumps(dump_cfg))
>      to_print.write("\n")
>      to_print.write(_make_header("MetaData"))
> -    to_print.write(util.yaml_dumps(cloud.datasource.metadata))
> +    to_print.write(_dumps(cloud.datasource.metadata))
>      to_print.write("\n")
>      to_print.write(_make_header("Misc"))
>      to_print.write("Datasource: %s\n" %
> 
> === modified file 'cloudinit/util.py'
> --- cloudinit/util.py	2014-09-30 20:24:54 +0000
> +++ cloudinit/util.py	2014-10-18 16:32:48 +0000
> @@ -1270,14 +1270,14 @@
>              logexc(LOG, "Failed writing url content to %s", target_fn)
>  
>  
> -def yaml_dumps(obj):
> -    formatted = yaml.dump(obj,
> -                    line_break="\n",
> -                    indent=4,
> -                    explicit_start=True,
> -                    explicit_end=True,
> -                    default_flow_style=False)
> -    return formatted
> +def yaml_dumps(obj, explicit_start=True, explicit_end=True):
> +    return yaml.safe_dump(obj,
> +                          line_break="\n",
> +                          indent=4,
> +                          explicit_start=explicit_start,
> +                          explicit_end=explicit_end,
> +                          default_flow_style=False,
> +                          allow_unicode=True)

qq. The last two arguments: default_flow_style and allow_unicode are not being sent as arguments to yaml_dumps. Should it be sending those (with appropriate defaults)?

I see that you fixed the other two args - explicit_start and explicit_end. You added those as arguments to yaml_dumps. That seems like the right thing to do. Should we do that for other arguments also?

>  
>  
>  def ensure_dir(path, mode=None):
> 
> === added file 'tests/unittests/test_handler/test_handler_debug.py'
> --- tests/unittests/test_handler/test_handler_debug.py	1970-01-01 00:00:00 +0000
> +++ tests/unittests/test_handler/test_handler_debug.py	2014-10-18 16:32:48 +0000
> @@ -0,0 +1,78 @@
> +# vi: ts=4 expandtab
> +#
> +#    Copyright (C) 2014 Yahoo! Inc.
> +#
> +#    This program is free software: you can redistribute it and/or modify
> +#    it under the terms of the GNU General Public License version 3, as
> +#    published by the Free Software Foundation.
> +#
> +#    This program is distributed in the hope that it will be useful,
> +#    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#    GNU General Public License for more details.
> +#
> +#    You should have received a copy of the GNU General Public License
> +#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +from cloudinit.config import cc_debug
> +
> +from cloudinit import cloud
> +from cloudinit import distros
> +from cloudinit import helpers
> +from cloudinit import util
> +
> +from cloudinit.sources import DataSourceNone
> +
> +from .. import helpers as t_help
> +
> +import logging
> +
> +LOG = logging.getLogger(__name__)
> +
> +
> +class TestDebug(t_help.FilesystemMockingTestCase):
> +    def setUp(self):
> +        super(TestDebug, self).setUp()
> +        self.new_root = self.makeDir(prefix="unittest_")
> +
> +    def _get_cloud(self, distro, metadata=None):
> +        self.patchUtils(self.new_root)
> +        paths = helpers.Paths({})
> +        cls = distros.fetch(distro)
> +        d = cls(distro, {}, paths)
> +        ds = DataSourceNone.DataSourceNone({}, d, paths)
> +        if metadata:
> +            ds.metadata.update(metadata)
> +        return cloud.Cloud(ds, paths, {}, d, None)
> +
> +    def test_debug_write(self):
> +        cfg = {
> +            'abc': '123',
> +            'c': u'\u20a0',
> +            'debug': {
> +                'verbose': True,
> +                # Does not actually write here due to mocking...
> +                'output': '/var/log/cloud-init-debug.log',
> +            },
> +        }
> +        cc = self._get_cloud('ubuntu')
> +        cc_debug.handle('cc_debug', cfg, cc, LOG, [])
> +        contents = util.load_file('/var/log/cloud-init-debug.log')
> +        # Some basic sanity tests...
> +        self.assertGreater(len(contents), 0)
> +        for k in cfg.keys():
> +            self.assertIn(k, contents)
> +
> +    def test_debug_no_write(self):
> +        cfg = {
> +            'abc': '123',
> +            'debug': {
> +                'verbose': False,
> +                # Does not actually write here due to mocking...
> +                'output': '/var/log/cloud-init-debug.log',
> +            },
> +        }
> +        cc = self._get_cloud('ubuntu')
> +        cc_debug.handle('cc_debug', cfg, cc, LOG, [])
> +        self.assertRaises(IOError,
> +                          util.load_file, '/var/log/cloud-init-debug.log')
> 


-- 
https://code.launchpad.net/~harlowja/cloud-init/debug-pretty/+merge/238799
Your team cloud init development team is requested to review the proposed merge of lp:~harlowja/cloud-init/debug-pretty into lp:cloud-init.