You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Santhosh Edukulla <sa...@citrix.com> on 2013/10/11 14:50:09 UTC
Review Request 14595: Adding https support to marvin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/
-----------------------------------------------------------
Review request for cloudstack.
Bugs: 4832
Repository: cloudstack-git
Description
-------
adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
useHttps, certCAPath, certPath - the params are self explanatory
Diffs
-----
setup/dev/advanced.cfg 4a48399
tools/marvin/marvin/cloudstackConnection.py 686c533
tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
tools/marvin/marvin/configGenerator.py a966ae0
tools/marvin/marvin/deployDataCenter.py beed8c8
Diff: https://reviews.apache.org/r/14595/diff/
Testing
-------
Thanks,
Santhosh Edukulla
Re: Review Request 14595: Adding https support to marvin
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/#review27278
-----------------------------------------------------------
Ship it!
Thanks for the patch!
dfa0678 on master. Please ensure your editor uses only spaces and no tabs. There were several syntax and indentation errors in the patch probably as a result of that. Also please ensure not to break pep8 in marvin's core.
- Prasanna Santhanam
On Oct. 11, 2013, 12:51 p.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14595/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2013, 12:51 p.m.)
>
>
> Review request for cloudstack, daan Hoogland and Prasanna Santhanam.
>
>
> Bugs: 4832
> https://issues.apache.org/jira/browse/4832
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
>
>
> useHttps, certCAPath, certPath - the params are self explanatory
>
>
> Diffs
> -----
>
> setup/dev/advanced.cfg 4a48399
> tools/marvin/marvin/cloudstackConnection.py 686c533
> tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
> tools/marvin/marvin/configGenerator.py a966ae0
> tools/marvin/marvin/deployDataCenter.py beed8c8
>
> Diff: https://reviews.apache.org/r/14595/diff/
>
>
> Testing
> -------
>
> Tested against http. Setting up https environment for testing. Will appreciate a test if anyone has https enabled on their setup
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 14595: Adding https support to marvin
Posted by Girish Shilamkar <gi...@clogeny.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/#review28521
-----------------------------------------------------------
4.2: 6e1821f585df5c643873ab0ce1e7d91d1d94f00d
- Girish Shilamkar
On Oct. 11, 2013, 12:51 p.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14595/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2013, 12:51 p.m.)
>
>
> Review request for cloudstack, daan Hoogland and Prasanna Santhanam.
>
>
> Bugs: 4832
> https://issues.apache.org/jira/browse/4832
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
>
>
> useHttps, certCAPath, certPath - the params are self explanatory
>
>
> Diffs
> -----
>
> setup/dev/advanced.cfg 4a48399
> tools/marvin/marvin/cloudstackConnection.py 686c533
> tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
> tools/marvin/marvin/configGenerator.py a966ae0
> tools/marvin/marvin/deployDataCenter.py beed8c8
>
> Diff: https://reviews.apache.org/r/14595/diff/
>
>
> Testing
> -------
>
> Tested against http. Setting up https environment for testing. Will appreciate a test if anyone has https enabled on their setup
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 14595: Adding https support to marvin
Posted by Santhosh Edukulla <sa...@citrix.com>.
> On Oct. 18, 2013, 12:36 p.m., daan Hoogland wrote:
> > The params are self explanatory indeed, i like them. The concern I have with them is regarding backward compatibility of the interface. I don't (yet) use the BVT suite due to lack of knowledge on my side. I use the connection object and api calls directly and fear the my present scripts will break with this code. This would warant a major version upgrade for marvin.
1. I tested the changes using simulator and it worked fine, i.e., it created relevant entities in CS post the run, so i assume that the changes are fine. Anyways, we are planning to test the changes with real cs https setup. If we can have one setup provided, that should be good. Apart from this, if there are any test cases either written by you or otherwise which are breaking, i can take a look.
2. The changes to interface are few, i agree that the changes are done to the interface, but i felt that user need not know minute details of cs connection, i can alter it to keep the changes as it is, but the regular regression and bvt will not be anyways effected, as user does not know about them unless they are explicitly using test client.
3. The current testclient is little confusing, i mean we need to abstract the user from all the details of test client creation. Internally, we would have read the configuration from file if provided, create both api client and db connection, currently we create api client with user passed mgmt details, and then again call dbconfigure for db connection. The testclient internally should take care to read the configuration and create both api and db connection necessary. I thought of changing this as well and atleast pass both db and mgmt details in one single call to testclient and it will provide apiclient and dbclient.
4. Also, testcase client would have inherited from dbconnection and exposed operations again rather than rewriting some methods again through test client.
5. There is one more issue with loadCfg of deployData..py where it is creating testclients unnecessarily based upon apiKey value, which i thought will change as another fix.
- Santhosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/#review27185
-----------------------------------------------------------
On Oct. 11, 2013, 12:51 p.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14595/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2013, 12:51 p.m.)
>
>
> Review request for cloudstack, daan Hoogland and Prasanna Santhanam.
>
>
> Bugs: 4832
> https://issues.apache.org/jira/browse/4832
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
>
>
> useHttps, certCAPath, certPath - the params are self explanatory
>
>
> Diffs
> -----
>
> setup/dev/advanced.cfg 4a48399
> tools/marvin/marvin/cloudstackConnection.py 686c533
> tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
> tools/marvin/marvin/configGenerator.py a966ae0
> tools/marvin/marvin/deployDataCenter.py beed8c8
>
> Diff: https://reviews.apache.org/r/14595/diff/
>
>
> Testing
> -------
>
> Tested against http. Setting up https environment for testing. Will appreciate a test if anyone has https enabled on their setup
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 14595: Adding https support to marvin
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/#review27185
-----------------------------------------------------------
The params are self explanatory indeed, i like them. The concern I have with them is regarding backward compatibility of the interface. I don't (yet) use the BVT suite due to lack of knowledge on my side. I use the connection object and api calls directly and fear the my present scripts will break with this code. This would warant a major version upgrade for marvin.
- daan Hoogland
On Oct. 11, 2013, 12:51 p.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14595/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2013, 12:51 p.m.)
>
>
> Review request for cloudstack, daan Hoogland and Prasanna Santhanam.
>
>
> Bugs: 4832
> https://issues.apache.org/jira/browse/4832
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
>
>
> useHttps, certCAPath, certPath - the params are self explanatory
>
>
> Diffs
> -----
>
> setup/dev/advanced.cfg 4a48399
> tools/marvin/marvin/cloudstackConnection.py 686c533
> tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
> tools/marvin/marvin/configGenerator.py a966ae0
> tools/marvin/marvin/deployDataCenter.py beed8c8
>
> Diff: https://reviews.apache.org/r/14595/diff/
>
>
> Testing
> -------
>
> Tested against http. Setting up https environment for testing. Will appreciate a test if anyone has https enabled on their setup
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 14595: Adding https support to marvin
Posted by Santhosh Edukulla <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14595/
-----------------------------------------------------------
(Updated Oct. 11, 2013, 12:51 p.m.)
Review request for cloudstack, daan Hoogland and Prasanna Santhanam.
Changes
-------
adding Dahn and Prasanna
Bugs: 4832
Repository: cloudstack-git
Description
-------
adding support to marvin for https. We will allow user to specify whether to usehttps or not using three params
useHttps, certCAPath, certPath - the params are self explanatory
Diffs
-----
setup/dev/advanced.cfg 4a48399
tools/marvin/marvin/cloudstackConnection.py 686c533
tools/marvin/marvin/cloudstackTestClient.py 36f7f8d
tools/marvin/marvin/configGenerator.py a966ae0
tools/marvin/marvin/deployDataCenter.py beed8c8
Diff: https://reviews.apache.org/r/14595/diff/
Testing (updated)
-------
Tested against http. Setting up https environment for testing. Will appreciate a test if anyone has https enabled on their setup
Thanks,
Santhosh Edukulla