cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

unittest?  comment in line.

Diff comments:

> diff --git a/cloudinit/util.py b/cloudinit/util.py
> index e42498d..779d2b2 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -1829,58 +1835,60 @@ def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
>          env = env.copy()
>          env.update(update_env)
>  
> -    try:
> -        if target_path(target) != "/":
> -            args = ['chroot', target] + list(args)
> +    if target_path(target) != "/":
> +        args = ['chroot', target] + list(args)
>  
> -        if not logstring:
> -            LOG.debug(("Running command %s with allowed return codes %s"
> -                       " (shell=%s, capture=%s)"), args, rcs, shell, capture)
> -        else:
> -            LOG.debug(("Running hidden command to protect sensitive "
> -                       "input/output logstring: %s"), logstring)
> -
> -        stdin = None
> -        stdout = None
> -        stderr = None
> -        if capture:
> -            stdout = subprocess.PIPE
> -            stderr = subprocess.PIPE
> -        if data is None:
> -            # using devnull assures any reads get null, rather
> -            # than possibly waiting on input.
> -            devnull_fp = open(os.devnull)
> -            stdin = devnull_fp
> -        else:
> -            stdin = subprocess.PIPE
> -            if not isinstance(data, bytes):
> -                data = data.encode()
> +    if not logstring:
> +        LOG.debug(("Running command %s with allowed return codes %s"
> +                   " (shell=%s, capture=%s)"), args, rcs, shell, capture)
> +    else:
> +        LOG.debug(("Running hidden command to protect sensitive "
> +                   "input/output logstring: %s"), logstring)
> +
> +    stdin = None
> +    stdout = None
> +    stderr = None
> +    if capture:
> +        stdout = subprocess.PIPE
> +        stderr = subprocess.PIPE
> +    if data is None:
> +        # using devnull assures any reads get null, rather
> +        # than possibly waiting on input.
> +        devnull_fp = open(os.devnull)
> +        stdin = devnull_fp
> +    else:
> +        stdin = subprocess.PIPE
> +        if not isinstance(data, bytes):
> +            data = data.encode()
>  
> +    try:

Can you add a comment about relocating setup logic out of the try block ?  It does seem unrelated to fixing the contents of the output.

>          sp = subprocess.Popen(args, stdout=stdout,
>                                stderr=stderr, stdin=stdin,
>                                env=env, shell=shell)
>          (out, err) = sp.communicate(data)
> -
> -        # Just ensure blank instead of none.
> -        if not out and capture:
> -            out = b''
> -        if not err and capture:
> -            err = b''
> -        if decode:
> -            def ldecode(data, m='utf-8'):
> -                if not isinstance(data, bytes):
> -                    return data
> -                return data.decode(m, decode)
> -
> -            out = ldecode(out)
> -            err = ldecode(err)
>      except OSError as e:
> -        raise ProcessExecutionError(cmd=args, reason=e,
> -                                    errno=e.errno)
> +        raise ProcessExecutionError(
> +            cmd=args, reason=e, errno=e.errno,
> +            stdout="-" if decode else b"-",
> +            sterr="-" if decode else b"-")
>      finally:
>          if devnull_fp:
>              devnull_fp.close()
>  
> +    # Just ensure blank instead of none.
> +    if not out and capture:
> +        out = b''
> +    if not err and capture:
> +        err = b''
> +    if decode:
> +        def ldecode(data, m='utf-8'):
> +            if not isinstance(data, bytes):
> +                return data
> +            return data.decode(m, decode)
> +
> +        out = ldecode(out)
> +        err = ldecode(err)
> +
>      rc = sp.returncode
>      if rc not in rcs:
>          raise ProcessExecutionError(stdout=out, stderr=err,


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/336497
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/subp-exception-with-bytes into cloud-init:master.