cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

I'd like 2 things changed
a.) do not use /bin/bash or require it by (Instance/Image).execute.
   That is not python's default shell, unless /bin/sh == /bin/bash on the system.
b.) lets only fix strings where we were already using sh (via sh -c).

throwing stuff to sh just allows for errors and unexpected behavior.

I realize this is a contrived example, but its the kind of  unexpected results you can get when you hand things off to shell interpretation and are not careful about quoting

## pretend tmp=$(mktemp -d) happens to return the literal name '/tmp/foo?'
## simulate this behavior by the 'tmp=; mkdir -p'
$ touch /tmp/foo1 /tmp/foo2
$ tmp=/tmp/foo?; mkdir -p "/tmp/foo?"
# ls /tmp/
foo1  foo2  foo?
# rm -Rvf $tmp
removed `/tmp/foo1'
removed `/tmp/foo2'
removed directory: `/tmp/foo?'

## YIKES!


I'll give you a diff or a MP with the changes i'd like.


Diff comments:

> diff --git a/tests/cloud_tests/instances/base.py b/tests/cloud_tests/instances/base.py
> index 959e9cc..af0d235 100644
> --- a/tests/cloud_tests/instances/base.py
> +++ b/tests/cloud_tests/instances/base.py
> @@ -31,6 +31,8 @@ class Instance(object):
>          target filesystem being available at /.
>  
>          @param command: the command to execute as root inside the image
> +            if command is a string, then it will be executed as:
> +            ['bash', '-c', command]

It doesn't seem right to me to build in an assumption on bash.
I think much better to make the integration tests work with posix sh.
then we will not depend on bash specifically, which might not
be available in an OS (ubuntu-core or freebsd).

>          @param stdout, stderr: file handles to write output and error to
>          @param env: environment variables
>          @param rcs: allowed return codes from command
> @@ -137,9 +139,8 @@ class Instance(object):
>              tests.append(self.config['cloud_init_ready_script'])
>  
>          formatted_tests = ' && '.join(clean_test(t) for t in tests)
> -        test_cmd = ('for ((i=0;i<{time};i++)); do {test} && exit 0; sleep 1; '
> -                    'done; exit 1;').format(time=time, test=formatted_tests)
> -        cmd = ['/bin/bash', '-c', test_cmd]
> +        cmd = ('for ((i=0;i<{time};i++)); do {test} && exit 0; sleep 1; '
> +               'done; exit 1;').format(time=time, test=formatted_tests)

lets limit the dependency on bash by leaving this as it was.
so only this function itself depends on bash (which is easily fixable)

>  
>          if self.execute(cmd, rcs=(0, 1))[-1] != 0:
>              raise OSError('timeout: after {}s system not started'.format(time))


-- 
https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/330535
Your team cloud-init commiters is requested to review the proposed merge of ~powersj/cloud-init:cii-strings into cloud-init:master.