cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Prasanna Santhanam" <...@apache.org>
Subject Re: Review Request 15021: Fixed Bug: 4899 : Added Configuration Support to Marvin.
Date Thu, 31 Oct 2013 04:51:40 GMT


> On Oct. 30, 2013, 10:22 a.m., Santhosh Edukulla wrote:
> > tools/marvin/marvin/configGenerator.py, line 303
> > <https://reviews.apache.org/r/15021/diff/1/?file=373114#file373114line303>
> >
> >     The reason is simple. We already have a module configGenerator and is used as
configuration utility ( reading configuraiton, object mapping and various few things ). I
dont want to create multiple modules with config** name, it adds some confusion to users to
understand and unnecesarily module creation. Ideally it can be separate, but the task is achieved
through a simple class inside of a configGenerator

since this module is providing facilities to tests it would've been nice to move it out. but
i'm not strongly against your way either.


> On Oct. 30, 2013, 10:22 a.m., Santhosh Edukulla wrote:
> > tools/marvin/marvin/configGenerator.py, line 337
> > <https://reviews.apache.org/r/15021/diff/1/?file=373114#file373114line337>
> >
> >     Nopes, its right, user by default no need to mention any path while creating
the object instead provides it under getConfig. User by default dont need to worry about the
configuration provided by default, we are providing him the parsed dictionary by default by
placing the configuration at a default location. Only if he wanted to change this configuration
to a separate one he can still change by using getConfig method.

so self.__init__(self, filepath='defaultpath'): should work. The reason for adding the filepath
argument is simply to aid call to the command line switch from nose. By losing the command
line switch from nose to specify the marvin-config path, do you not feel we are restricting?

also, i guess i'm having a hard time seeing how the said user will use this getConfig utility.
can you perhaps re-use the ldap test to show how the hardcoded external resource is simplified
using ConfigManager? that would make it easier to understand and this patch can be absorbed
with that test.


> On Oct. 30, 2013, 10:22 a.m., Santhosh Edukulla wrote:
> > tools/marvin/marvin/cloudstackTestClient.py, line 22
> > <https://reviews.apache.org/r/15021/diff/1/?file=373112#file373112line22>
> >
> >     It came from merge, i didnt used them in modules required.
> 
> Santhosh Edukulla wrote:
>     This i will change to remove unused imports in a different patch.

can you readjust this patch and rebase atop master?


- Prasanna


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15021/#review27765
-----------------------------------------------------------


On Oct. 30, 2013, 10:12 a.m., Santhosh Edukulla wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15021/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2013, 10:12 a.m.)
> 
> 
> Review request for cloudstack, Girish Shilamkar and Prasanna Santhanam.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
>  Added Configuration Support to Marvin.
>     
>     1. It provides the basic configuration facilities to marvin.
>     
>     2. User can just add configuration files for his tests, deployment
>                   etc, under one config folder before running their tests.
>                   cs/tools/marvin/marvin/config.
>                   They can remove all hard coded values from code and separate
>                   it out as config at this location.
>                   Either add this to the existing setup.cfg as separate section
>                   or add new configuration.
>     3. This will thus removes hard coded tests and separate
>                   data from tests.
>     
>     4. This API is provided as an additional facility under
>                   cloudstackTestClient and users can get the
>                   configuration object as similar to apiclient,dbconnection
>                   etc to drive their test.
>     
>     5. They just add their configuration for a test,
>                   setup etc,at one single place under configuration dir
>                   and use "getConfigParser" API of cloudstackTestClient
>                   It will give them "configObj".They can either pass their own
>                   config file for parsing to "getConfig" or it will use
>                   default config file @ config/setup.cfg.
>     6. They will then get the dictionary of parsed
>                   configuration and can use it further to drive their tests or
>                   config drive
>     7. Test features, can  drive their setups thus removing hard coded
>               values. Configuration default file will be under config and as
>                   setup.cfg.
>     8. Users can use their own configuration file passed to
>                   "getConfig" API,once configObj is returned.
>     
> Another such case where we are using sed or bash script is  in between a build run for
replacing hard coded ldap ip for region\setup specific. We can now change all parameters before
run under configuration, the test features will use configuration object and thus values,
rather hard coded strings which avoids replacement through shell script.
> 
>  Also, did few naming convention changes. Its better to follow uniform naming conventions.
Currently, wherever iam seeing a specific notation not followed. We are incorporating those
changes.
> 
> ToDo:
> clean up of current config at places, making configuration required for marvin\tests
unified and available at one place and clean up of files\code related to it. 
> 
> 
> Diffs
> -----
> 
>   tools/marvin/marvin/cloudstackTestClient.py be93f35 
>   tools/marvin/marvin/config/setup.cfg PRE-CREATION 
>   tools/marvin/marvin/configGenerator.py 0cfad30 
>   tools/marvin/marvin/integration/lib/utils.py 7d662af 
> 
> Diff: https://reviews.apache.org/r/15021/diff/
> 
> 
> Testing
> -------
> 
> Basic check to retrieve configuration values is done.
> 
> 
> Thanks,
> 
> Santhosh Edukulla
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message