cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Think I'm done with this branch. Thanks Josh for shepherding this all in.
This is good step in the right direction, and since it's in our test code instead of core, I shouldn't try to be a stickler on some of these concerns as we can iterate on this without direct impact to core code.

Diff comments:

> diff --git a/tests/cloud_tests/setup_image.py b/tests/cloud_tests/setup_image.py
> index 5d6c638..1b74ceb 100644
> --- a/tests/cloud_tests/setup_image.py
> +++ b/tests/cloud_tests/setup_image.py
> @@ -7,6 +7,30 @@ from functools import partial
>  import os
>  
>  
> +def installed_version(image, package, ensure_installed=True):

rename request to give more context s/installed_version/installed_package_version/. The benefit of a more verbose function naming is that it potentially prevents someone from having to come back from callsites to the original function defintion to find out what the intent is.

> +    """
> +    get installed version of package
> +    image: cloud_tests.images instance to operate on
> +    package: name of package
> +    ensure_installed: raise error if not installed
> +    return_value: cloud-init version string
> +    """
> +    # get right cmd for os family
> +    os_family = util.get_os_family(image.properties['os'])
> +    if os_family == 'debian':
> +        cmd = ['dpkg-query', '-W', "--showformat='${Version}'", package]
> +    elif os_family == 'redhat':
> +        cmd = ['rpm', '-q', '--queryformat', "'%{VERSION}'", package]
> +    else:
> +        raise NotImplementedError
> +
> +    # query version
> +    msg = 'query version for package: {}'.format(package)
> +    (out, err, exit) = image.execute(
> +        cmd, description=msg, rcs=(0,) if ensure_installed else range(0, 256))
> +    return out.strip()
> +
> +
>  def install_deb(args, image):
>      """
>      install deb into image
> diff --git a/tests/cloud_tests/util.py b/tests/cloud_tests/util.py
> index 64a8667..79ed883 100644
> --- a/tests/cloud_tests/util.py
> +++ b/tests/cloud_tests/util.py
> @@ -158,6 +174,140 @@ def write_file(*args, **kwargs):
>      """
>      write a file using cloudinit.util.write_file
>      """
> -    c_util.write_file(*args, **kwargs)
> +    return c_util.write_file(*args, **kwargs)
> +
> +
> +def read_conf(*args, **kwargs):
> +    """
> +    read configuration using cloudinit.util.read_conf
> +    """
> +    return c_util.read_conf(*args, **kwargs)
> +
> +
> +def subp(*args, **kwargs):
> +    """
> +    execute a command on the system shell using cloudinit.util.subp
> +    """
> +    return c_util.subp(*args, **kwargs)
> +
> +
> +def tmpdir(prefix='cloud_test_util_'):

Questionable utility function. We probbably should just call this tempfile.mkdtemp directly. It's just adding bloat in my mind.

> +    return tempfile.mkdtemp(prefix=prefix)
> +
> +
> +def rel_files(basedir):
> +    """
> +    list of files under directory by relative path, not including directories
> +    return_value: list or relative paths
> +    """
> +    basedir = os.path.normpath(basedir)
> +    return [path[len(basedir) + 1:] for path in
> +            glob.glob(os.path.join(basedir, '**'), recursive=True)
> +            if not os.path.isdir(path)]
> +
> +
> +def flat_tar(output, basedir, owner='root', group='root'):
> +    """
> +    create a flat tar archive (no leading ./) from basedir
> +    output: output tar file to write
> +    basedir: base directory for archive
> +    owner: owner of archive files
> +    group: group archive files belong to
> +    return_value: none
> +    """
> +    c_util.subp(['tar', 'cf', output, '--owner', owner, '--group', group,
> +                 '-C', basedir] + rel_files(basedir), capture=True)
> +
> +
> +def parse_conf_list(entries, valid=None, boolean=False):

We can drop boolean if we make this util function a bit smarter. It should probably also return an empty dict upon invalid keys as it shouldn't change the type of it's return value. Also it should raise a ValueError if it doesn't see an = sign so we get a better traceback if called improperly (like --feature-override asdf:true)

It's almost there:

def parse_conf_list(entries, valid=None):
   """Parse config list of key=value.

   entries: List of key=value or key:value configuration strings.
   valid: List of valid keys in result.
   return_value: Dict of configuration values or empty dict if invalid keys
   """
   res = {}
   for entry in entries:
       if "=" not in entry:
           raise ValueError("Invalid key provided. Expected key=value, found '{}'".format(entry))
       key, value = entry.split("=")
       if value.lower() in ("true", "false"):
           value = value.lower() == 'true'
       if valid and key in valid:
           res[key] = value
   return res

> +    """
> +    parse config in a list of strings in key=value format
> +    entries: list of key=value strings
> +    valid: list of valid keys in result, return None if invalid input
> +    boolean: if true, then interpret all values as booleans where 'true' = True
> +    return_value: dict of configuration or None if invalid
> +    """
> +    res = {key: value.lower() == 'true' if boolean else value
> +           for key, value in (i.split('=') for i in entries)}
> +    return res if not valid or all(k in valid for k in res.keys()) else None
> +
> +
> +def update_args(args, updates, preserve_old=True):
> +    """
> +    update cmdline arguments from a dictionary
> +    args: cmdline arguments
> +    updates: dictionary of {arg_name: new_value} mappings
> +    preserve_old: if true, create a deep copy of args before updating
> +    return_value: updated cmdline arguments, as new object if preserve_old=True
> +    """
> +    args = copy.deepcopy(args) if preserve_old else args
> +    if updates:
> +        vars(args).update(updates)
> +    return args
> +
> +
> +def update_user_data(user_data, updates, dump_to_yaml=True):
> +    """
> +    user_data: user data as yaml string or dict
> +    updates: dictionary to merge with user data
> +    dump_to_yaml: return as yaml dumped string if true
> +    return_value: updated user data, as yaml string if dump_to_yaml is true
> +    """
> +    user_data = (c_util.load_yaml(user_data)
> +                 if isinstance(user_data, str) else copy.deepcopy(user_data))
> +    user_data.update(updates)
> +    return (yaml_format(user_data, content_type='cloud-config')
> +            if dump_to_yaml else user_data)
> +
> +
> +class InTargetExecuteError(c_util.ProcessExecutionError):
> +    """
> +    Error type for in target commands that fail
> +    """
> +    default_desc = 'Unexpected error while running command in target instance'
> +
> +    def __init__(self, stdout, stderr, exit_code, cmd, instance,
> +                 description=None):
> +        """
> +        init error and parent error class
> +        """
> +        if isinstance(cmd, (tuple, list)):
> +            cmd = ' '.join(cmd)
> +        super(InTargetExecuteError, self).__init__(
> +            stdout=stdout, stderr=stderr, exit_code=exit_code, cmd=cmd,
> +            reason="Instance: {}".format(instance),
> +            description=description if description else self.default_desc)
> +
> +
> +class TempDir(object):
> +    """
> +    temporary directory like tempfile.TemporaryDirectory, but configurable
> +    """
> +
> +    def __init__(self, args):
> +        """
> +        setup and store args
> +        args: cmdline arguments
> +        """
> +        self.args = args
> +        self.tmpdir = None
> +
> +    def __enter__(self):
> +        """
> +        create tempdir
> +        return_value: tempdir path
> +        """
> +        self.tmpdir = tempfile.mkdtemp(prefix='cloud_test_')
> +        LOG.debug('using tmpdir: %s', self.tmpdir)
> +        return self.tmpdir
> +
> +    def __exit__(self, etype, value, trace):
> +        """
> +        destroy tempdir if no errors occurred
> +        """
> +        if etype:
> +            LOG.warn('erros occurred, leaving data in %s', self.tmpdir)

s/erros/errors/

> +        else:
> +            shutil.rmtree(self.tmpdir)
>  
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/323588
Your team cloud-init commiters is requested to review the proposed merge of ~powersj/cloud-init:integration-test-revamp into cloud-init:master.