Re: [PATCH][KVM-AUTOTEST] Add custom install option for kvm_install

[Michael Goldish <mgoldish@redhat.com>]

----- "Mike Burns" <mburns@redhat.com> wrote:

> Eduardo Habkost wrote:
> > Hi,
> >
> > Excerpts from Michael Burns's message of Fri May 08 15:55:30 -0300
> 2009:
> >   
> >> --- a/client/tests/kvm_runtest_2/kvm_install.py
> >> +++ b/client/tests/kvm_runtest_2/kvm_install.py
> >> @@ -77,6 +77,17 @@ def run_kvm_install(test, params, env):
> >>      elif install_mode == "localsrc":
> >>          __install_kvm(test, srcdir)
> >>  
> >> +    # install from custom script
> >> +    elif install_mode == "custom":
> >> +        install_script = params.get("install_script")
> >> +        script =
> os.path.join(os.environ['AUTODIR'],install_script)
> >> +        if not install_script:
> >> +            message = "Custom script filename not specified"
> >> +            kvm_log.error(message)
> >> +            raise error.TestError, message
> >> +    kvm_log.info("Running " + script + " to install kvm")
> >> +        os.system(script)
> >>     
> >
> > What if we had some way to pass the other parameters from 'params'
> to
> > the custom script?
> >
> > Maybe something like (untested):
> >
> >   for k in params.keys():
> >       os.putenv("KVM_INSTALL_%s" % (k), params[k])
> >
> > Are all values on 'params' guaranteed to be strings, or they can be
> set
> > to any python value? In the latter case, we could use
> str(params[k]), or
> > export only the string parameters.
> >   
> That's a good idea.  I'm not sure about whether the params are all 
> strings.  I'll try it out and respin the patch after. 

1. Not all params are strings -- 'depend' is a list of strings, so str(params[k]) is a good idea.

2. Why not just pass the parameters via the command line, e.g.
install_command = my_script.sh param1 param2

3. Why do you use os.environ['AUTODIR'] instead of test.bindir? As far as I know, $AUTODIR is the 'client' dir, while test.bindir takes you directly to kvm_runtest_2.

4. It may be useful to run the script in the kvm_runtest_2 dir, in case the script wants to create symlinks or anything like that. So the last line could be something like
os.system("cd %s; %s" % (test.bindir, install_script))