You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by sebgoa <ru...@gmail.com> on 2013/12/11 10:12:26 UTC
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
Re: Marvin refactoring
Posted by Prasanna Santhanam <ts...@apache.org>.
On Wed, Dec 11, 2013 at 10:12:26AM +0100, sebgoa wrote:
>
> 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 ?
It's not for Citrix internal actually and I'm not aware what is used
internally within Citrix right now. Shouldn't be widely different.
This code drives tests run via jenkins.bacd.org only. I think it's a
mistake to bring in infra-code into product-code because of a marvin
dependency. Instead marvin should be separated and pushed to pypi on
successful builds regularly and the infra code and choose/not to pick
that change. Sorry, I didn't notice the discussion for this but it's
going to be problematic having it within marvin code. Just like we
have unrelated test code lying about :)
TestInfraCode makes breaking and radical changes which is why I kept
it separate. A lot like openstack-infra in fact. Curious as to why it
was thought to be a good idea to bring this into cloudstack? May be a
cloudstack-infra repo would be more appropriate? *
* Although, I prefer github and its PRs for this.
>
> Let me know what you think,
>
> thanks,
>
> -Sebastien
--
Prasanna.,
------------------------
Powered by BigRock.com
RE: Marvin refactoring
Posted by Rayees Namathponnan <ra...@citrix.com>.
+1 for any Marvin refactor should be in separate branch, then run and BVT and regression before merging to master and current release branch.
Regards,
Rayees
-----Original Message-----
From: Sudha Ponnaganti [mailto:sudha.ponnaganti@citrix.com]
Sent: Wednesday, December 11, 2013 4:49 AM
To: dev@cloudstack.apache.org; Santhosh Edukulla; girish@clogeny.com
Cc: Prasanna Santhanam
Subject: RE: Marvin refactoring
+1 for running on separate branch before check in
-----Original Message-----
From: sebgoa [mailto:runseb@gmail.com]
Sent: Wednesday, December 11, 2013 1:12 AM
To: Santhosh Edukulla; girish@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
RE: Marvin refactoring
Posted by Sudha Ponnaganti <su...@citrix.com>.
+1 for running on separate branch before check in
-----Original Message-----
From: sebgoa [mailto:runseb@gmail.com]
Sent: Wednesday, December 11, 2013 1:12 AM
To: Santhosh Edukulla; girish@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
RE: Marvin refactoring
Posted by Animesh Chaturvedi <an...@citrix.com>.
+1
-----Original Message-----
From: sebgoa [mailto:runseb@gmail.com]
Sent: Wednesday, December 11, 2013 1:12 AM
To: Santhosh Edukulla; girish@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
Re: Marvin refactoring
Posted by sebgoa <ru...@gmail.com>.
On Dec 12, 2013, at 8:36 PM, Santhosh Edukulla <sa...@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 [runseb@gmail.com]
> Sent: Wednesday, December 11, 2013 4:12 AM
> To: Santhosh Edukulla; girish@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
RE: Marvin refactoring
Posted by Santhosh Edukulla <sa...@citrix.com>.
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 [runseb@gmail.com]
Sent: Wednesday, December 11, 2013 4:12 AM
To: Santhosh Edukulla; girish@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