On Wed, Apr 17, 2013 at 11:00 AM, Venkata Siva Vijayendra Bhamidipati < vijayendra.bhamidip...@citrix.com> wrote:
> > > > On April 16, 2013, 4:31 a.m., Prasanna Santhanam wrote: > > > tools/marvin/marvin/cloudstackConnection.py, line 61 > > > < > https://reviews.apache.org/r/10294/diff/4/?file=281521#file281521line61> > > > > > > How and when does marvin decide to use POST? Which commands will > call the POST httpmethod? May be you can provide a more complete example > when your test is ready. > > I saw that make_request() was calling into make_request_with_auth(), so I > put in the default value for httpmethod as GET, but missed the fact that > make_request_with_auth() is calling itself when retrying - thanks for > catching that - I've put in a default value of GET for this parameter. When > a unittest calls this function, it would be expected to pass in all > arguments in case it wants to use POST, but existing tests that simply use > GET don't need to change.. does that look ok? Or is there a better way for > me to implement this? > > > > On April 16, 2013, 4:31 a.m., Prasanna Santhanam wrote: > > > tools/marvin/marvin/cloudstackConnection.py, line 77 > > > < > https://reviews.apache.org/r/10294/diff/4/?file=281521#file281521line77> > > > > > > Are you sure you don't want to urlencode the POST data? > > > > > Yes that is definitely better than doing requestUrl = > "&".join(["=".join([r[0], urllib.quote_plus(str(r[1]))]) for r in request]) > , which won't take care of spaces either.. have made the change here and > even in make_request_without_auth().. > > > > On April 16, 2013, 4:31 a.m., Prasanna Santhanam wrote: > > > tools/marvin/marvin/cloudstackConnection.py, line 79 > > > < > https://reviews.apache.org/r/10294/diff/4/?file=281521#file281521line79> > > > > > > Minor nitpick: urllib2.Request() can be used for both GET and POST > and you can set the data argument only when you intend to use POST. That > will reduce some code duplication here. > > I changed both cases to use urllib2.Request(), but still it's still the > same number of lines :) Is there a way I can merge those two separate calls > to urllib2.Request(url) and urllib2.Request(url, requestUrl) into a single > call? > If it does not cost much, let's introduce a new dependency for requesting stuff (get or post etc.) using "requests" [1] for both marvin and cloudmonkey. Vijay, you can write your own requester, as it's not much effort required to implement your own requester once you how signature is calculated. IMO, cloudstackConnection could be rewritten using latest ways and better libraries as it's not much code and is totally do-able with much less effort. A lot of flows for auth or non-auth are inter-dependent, the json->obj and obj->json parts and requesting logic is not straight forward (each method does *only* one thing). We need to make it generic by providing both raw and processed outputs, for example the result expects to return an obj and not json (maybe I need json and not an obj), one reason why I chose to write my own for cloudmonkey after I tried to reuse it. Lastly, host marvin on pypi so it's easier for any developer to get it off the shelf and start hacking some cool clients. Cheers. [1] http://docs.python-requests.org/en/latest/ > > > - Venkata Siva Vijayendra > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10294/#review19245 > ----------------------------------------------------------- > > > On April 16, 2013, 2:53 a.m., Venkata Siva Vijayendra Bhamidipati wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/10294/ > > ----------------------------------------------------------- > > > > (Updated April 16, 2013, 2:53 a.m.) > > > > > > Review request for cloudstack, Chip Childers, Hugo Trippaers, Kelven > Yang, and Min Chen. > > > > > > Description > > ------- > > > > Please refer to > https://cwiki.apache.org/confluence/display/CLOUDSTACK/DeployVirtualMachine+userdata+enhancementsfor > a background on the requirements driving this patch. > > > > This patch hasn't been extensively tested yet, and I will update this > request with more info. I am uploading a first diff for initial > review/comments. > > > > > > This addresses bug CLOUDSTACK-1086. > > > > > > Diffs > > ----- > > > > api/src/com/cloud/vm/UserVmService.java d963b74 > > api/src/org/apache/cloudstack/api/BaseCmd.java 42c0680 > > api/src/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java > 77ba9fe > > core/src/com/cloud/vm/UserVmVO.java a16eaf9 > > server/src/com/cloud/api/ApiDispatcher.java 925d90a > > server/src/com/cloud/api/ApiServer.java d842819 > > server/src/com/cloud/api/ApiServerService.java 12d8b52 > > server/src/com/cloud/api/ApiServlet.java 03bfb5f > > server/src/com/cloud/vm/UserVmManagerImpl.java d281e5b > > server/test/com/cloud/vm/MockUserVmManagerImpl.java fd826d9 > > server/test/com/cloud/vm/dao/UserVmDaoImplTest.java 0936180 > > server/test/com/cloud/vm/dao/UserVmDaoTestConfiguration.java > PRE-CREATION > > server/test/resources/UserVMDaoTestContext.xml PRE-CREATION > > setup/db/db/schema-410to420.sql fb760bf > > tools/marvin/marvin/cloudstackConnection.py 1caeef3 > > > > Diff: https://reviews.apache.org/r/10294/diff/ > > > > > > Testing > > ------- > > > > > > Thanks, > > > > Venkata Siva Vijayendra Bhamidipati > > > > > >
