> On Aug. 13, 2014, 1:43 p.m., Santhosh Edukulla wrote: > > tools/marvin/marvin/cloudstackTestClient.py, line 348 > > <https://reviews.apache.org/r/24544/diff/1/?file=657577#file657577line348> > > > > Make listall as keyword argument, instead of always making it to set to > > true. > > Gaurav Aradhye wrote: > Are you suggesting to add it as a parameter to the function? If I add it > as keyword argument here, it won't make much sense because the main task of > function is creating the api client. listUsers is one of the operations we do > inside the function. > > Santhosh Edukulla wrote: > Hardcoding inside to listall=True is not the right approach, instead > createuserapiclient is called from getUserApiClient, so here we can make it > keyword argument with listall=True, and pass this arg to createuserapiclient. > That we we can control the behavior of what we wanted and default set to true > using get function. Down the lane if behavior changed, just like we added > here inside the code, then instead of that we will control from outside layer > with out touching its internal.
This was already committed, I will add a new patch for incorporating your comments. Closing this review request. - Gaurav ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24544/#review50430 ----------------------------------------------------------- On Aug. 11, 2014, 3:50 p.m., Gaurav Aradhye wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24544/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2014, 3:50 p.m.) > > > Review request for cloudstack, Doug Clark and Santhosh Edukulla. > > > Bugs: CLOUDSTACK-7294 > https://issues.apache.org/jira/browse/CLOUDSTACK-7294 > > > Repository: cloudstack-git > > > Description > ------- > > According to API changes > (https://cwiki.apache.org/confluence/display/CLOUDSTACK/List*+API+commands+rules), > listall parameter is necessary when admin wants to list the resources which > belong to some other account. > > Adding domainid and listall parameter while making listUsers api call through > admin. > > > Diffs > ----- > > tools/marvin/marvin/cloudstackTestClient.py 1ca79ee > > Diff: https://reviews.apache.org/r/24544/diff/ > > > Testing > ------- > > Yes. > > Tested two failing test cases with this change. > > Test Deploy VM with 4 core CPU & verify the usage ... === TestName: > test_01_multiple_core_vm_start_stop_instance | Status : SUCCESS === > ok > > ---------------------------------------------------------------------- > Ran 1 test in 627.195s > > OK > > > @summary: Test List IP Addresses pagination ... === TestName: > test_01_list_ipaddresses_pagination | Status : SUCCESS === > ok > > ---------------------------------------------------------------------- > Ran 1 test in 32.022s > > OK > > > Thanks, > > Gaurav Aradhye > >
