cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Santhosh Edukulla <>
Subject RE: Marvin refactoring
Date Thu, 12 Dec 2013 19:36:55 GMT

Few notes.

1. The whole exercise is to bring quality and make Marvin "little" cleaner where ever possible.
If we see earlier\currently, at some places we followed some convention, say to use proper
casing for class names and modules and at some places we break from this, some times the directory
structure convention is properly not followed, few dead code, hard coded strings, invalid
returns or no check of returns some times for function calls, hard design implementations
to work only with first zone,domain etc, repeatable service class in each test module, no
proper exception handling, not cleaning resources at some places when exceptions are thrown
etc few issues we have seen currently. So, we tried to fix them them wherever we can, to get
to a cleaner and better state.   

2. We did few changes as you mentioned to bring out EX: catching test exceptions by plugin
itself etc to catch test script failures for entire run at a single place to differentiate
them from product failures and uncover as many script errors as possible, streamlining logging
etc, fixing few ssh bugs, config folder to config parser  etc.  Even automation code is also
code and to make integration tests good, we did these changes. I believe if all agree we will
make few more as well to fix few issues mentioned in note 1 above. 

3. Following a proper convention i believe is good ( provided agreeable to all ). We make
sure that main marvin code we check in  atleast  pep8 compliant and all follows proper convention
to make it maintainable. Separate few responsibilities for individual modules, make it work
with few design issues removed  etc.  We can have multiple entities and framework should cope
up to ensure that it works with minimal configuration and scalable for cloud testing. 

"Pass","pass","PASS" etc are few notions where different conventions were getting followed,
so added codes( I agree name of this module can be still better ) to make sure that all follow
same convention, least possible if they are related to increase readability and one point
of change rather than changing at multiple places. We make sure that temporary changes are
zero or not at all. Some changes are done to keep current tests going with out which they
failed. EX: We started VM but didnt verified the operation was successful and many like these
at some places, this was existing currently at some\many places.   As such we cant clean all
things as a whole, whenever we encounter few issues, with out breaking the current tests,
we are fixing these conventions though few are simple along with other issues.  

There were few build scripts maintained separately from this repo and are using relevant lib
calls from marvin, Any change to marvin here, we have to see these builds scripts dependencies
in other repo before checking them in are not broken. Felt that maintaining a separate repo
for build scripts dependent upon marvin framework\libs(  not all ) is little away and moved
them here under misc\build. This place holder can be used again for ACS related build scripts
as well if there are any dependencies used. Its just a misc\build folder. 

Currently, any change requires push to 4.2,master,4.3, a user install again  and we some how
made to fit marvin in mvn life cycle. I believe we can make this segregation of CS and marvin
and carve out a new repo to ease fixing any issues here like we did for documentation ( only
if agreed by all ). keeping CS dependencies minimal is also one goal to change, i mean in
terms of branching and maintenance. We can separate marvin and with init point for marvin
can take api.xml and start generating relevant structures possible decouple it totally from
mvn as an example, rather tightly integrating currently with CS build.

Regarding maintaining a separate branch for refactoring, i believe Its  a good point and i
second that the changes need to be done separately and be merged once tested and agreed post
verification, and not break the current Automation Runs. We are making sure that every effort
is made not to break the runs as much as possible. We will make sure that the changes will
not impact the current runs. We as well agreed to make the changes separately in a different
branch and merge them here going ahead. 

Please feel free to add your comments to the reviews, wherever you feel it can be fixed better\improved
and as well log bugs under Automation\Marvin and assign them to me. 

From: sebgoa []
Sent: Wednesday, December 11, 2013 4:12 AM
To: Santhosh Edukulla;;
Cc: Prasanna Santhanam
Subject: Marvin refactoring

Hi devs and Santosh and Girish,

I am looking at Marvin lately and I am seeing lots of changes happening, especially:

That's great to see effort for integration testing. However I am concerned that these changes
(sometimes temporary fixes) are going straight in 4.2, 4.3 and master.

How about creating a marvin refactoring branch and working there ?

I believe that what is being done to Marvin is significant enough (renaming of files, change
of logger structures, change of function names, addition of constants etc..) that it should
be developed in its own feature branch and a merge should be called once the refactoring is

Personally, I don't agree with some of the cosmetic changes: like move to camel case for function
name (not pythonic even though pep8 compliant) or the addition of (different name
maybe ?).  Sometimes the api gets broken as well, like in cloudstackConnection.

We should also have an open discussion about the import of
in marvin/marvin/misc/build. That repo from prasanna while part of the CI infra is really
custom config for Citrix internal infra (right ?). I don't think this has a place inside the
Marvin code. When we build Marvin for instance, what happens to that code ? Does it get uploaded
to pypi if we push Marvin to pypi ?

Let me know what you think,



View raw message