cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

reply in line.


Diff comments:

> diff --git a/cloudinit/util.py b/cloudinit/util.py
> index c0473b8..6eff880 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -1843,9 +1843,59 @@ def subp_blob_in_tempfile(blob, *args, **kwargs):
>          return subp(*args, **kwargs)
>  
>  
> -def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
> +def subp(args, data=None, rcs=None, env=None, capture=True,
> +         combine_capture=False, shell=False,

I would put more weight on making this similar to curtin than to sorting, and in general I dont think sorting arguments in a function signature makes much sense.

2 things motivated this:
a.) the order of the kwargs in curtin
b.) I suspect that 'combine_capture' in curtin was added where it was to be "close" to 'capture', which is obviously related.

Generally speaking as you evolve a function signature you have to just append variables for backwards compat.  So this clearly does break backwards compatibility if anything was using 'shell' as the 6th argv.  I'm not worried about that as generally speaking we have not done that.

>           logstring=False, decode="replace", target=None, update_env=None,
>           status_cb=None):
> +    """Run a subprocess.
> +
> +    :param args: command to run in a list. [cmd, arg1, arg2...]
> +    :param data: input to the command, made available on its stdin.
> +    :param rcs:
> +        a list of allowed return codes.  If subprocess exits with a value not
> +        in this list, a ProcessExecutionError will be raised.  By default,
> +        data is returned as a string.  See 'decode' parameter.
> +    :param env: a dictionary for the command's environment.
> +    :param capture:
> +        boolean indicating if output should be captured.  If True, then stderr
> +        and stdout will be returned.  If False, they will not be redirected.
> +    :param combine_capture:
> +        boolean indicating if stderr should be redirected to stdout. When True,
> +        interleaved stderr and stdout will be returned as the first element of
> +        a tuple, the second will be empty string or bytes (per decode).
> +        if combine_capture is True, then output is captured independent of
> +        the value of capture.
> +    :param log_captured:

fixed.

> +        boolean indicating if output should be logged on capture.  If
> +        True, then stderr and stdout will be logged at DEBUG level.  If
> +        False, they will not be logged.
> +    :param shell: boolean indicating if this should be run with a shell.
> +    :param logstring:
> +        the command will be logged to DEBUG.  If it contains info that should
> +        not be logged, then logstring will be logged instead.
> +    :param decode:
> +        if False, no decoding will be done and returned stdout and stderr will
> +        be bytes.  Other allowed values are 'strict', 'ignore', and 'replace'.
> +        These values are passed through to bytes().decode() as the 'errors'
> +        parameter.  There is no support for decoding to other than utf-8.
> +    :param target:
> +        not supported, kwarg present only to make function signature similar
> +        to curtin's subp.
> +    :param update_env:
> +        update the enviornment for this command with this dictionary.
> +        this will not affect the current processes os.environ.
> +    :param status_cb:
> +        call this fuction with a single string argument before starting
> +        and after finishing.
> +
> +    :return
> +        if not capturing, return is (None, None)
> +        if capturing, stdout and stderr are returned.
> +            if decode:
> +                entries in tuple will be python2 unicode or python3 string
> +            if not decode:
> +                entries in tuple will be python2 string or python3 bytes
> +    """
>  
>      # not supported in cloud-init (yet), for now kept in the call signature
>      # to ease maintaining code shared between cloud-init and curtin


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/347066
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/subp-combine-output into cloud-init:master.