Sebastian\Team, 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. Regards, Santhosh ________________________________________ From: sebgoa [run...@gmail.com] Sent: Wednesday, December 11, 2013 4:12 AM To: Santhosh Edukulla; gir...@clogeny.com; dev@cloudstack.apache.org 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: https://reviews.apache.org/users/santhoshe/ 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 done. 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 codes.py (different name maybe ?). Sometimes the api gets broken as well, like in cloudstackConnection. We should also have an open discussion about the import of https://github.com/vogxn/cloud-autodeploy 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, thanks, -Sebastien
