On Dec 12, 2013, at 8:36 PM, Santhosh Edukulla <santhosh.eduku...@citrix.com> wrote:
> 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. These are all good and needed to improve Marvin. However they are not just bug fixes, they fit into a 'refactoring' endeavor and as such should be made in a separate branch. I feel that these changes should not be committed to master and 4.3 and certainly not into 4.2 which is now in a bug fix only release cycle. There is already a branch marvin_refactoring (which I believe was merged some time back), I have not checked what's in there, but I suggest that you commit all your changes to such branch until the code stabilizes. then we can call for a merge to the master branch. > > 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. > I need to understand this better, but it seems very wrong. As noted by Prasanna, this is a bad fit to bring it within the marvin code. I would be in favor to revert that commit. > 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. Current runs should use current Marvin. Your refactoring should go in a separate branch. Looks like we agree. Let's see if we need to revive this thread http://markmail.org/thread/a3rha4zwetfngqxb > > 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
