----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17789/#review34015 -----------------------------------------------------------
tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63941> The name is also little confusing. It is basically getting an ostype name for a featured and builtin template which is in ready state. Modify the name to suite its flow tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63945> use try\except block to wrap and return exception code in case of an exception. tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63940> Add doc strings for this method. tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63946> Also, i believe the need for usage of this function. Why cant it be handled inside of get_template only. If user provided ostype is not matching return the record which is builtin featured and ready. Why need a separate function. When people use this and when other? Any cases? tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63947> If it is tested, which cases have modified, please add them here and along with their logs? tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63942> Please dont add assert in library functions. Use return codes and assert for your test case. Assertion for library is not good. tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63943> why raise and return both? Dont raise exception, with this it will become enforcing on tests to either catch it or make it to work for tests according to their behavior tools/marvin/marvin/integration/lib/common.py <https://reviews.apache.org/r/17789/#comment63944> Again remove raise. Use return codes. - Santhosh Edukulla On Feb. 6, 2014, 2:06 p.m., suresh sadhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17789/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2014, 2:06 p.m.) > > > Review request for cloudstack, Santhosh Edukulla and SrikanteswaraRao Talluri. > > > Bugs: cloudstack-6042 > https://issues.apache.org/jira/browse/cloudstack-6042 > > > Repository: cloudstack-git > > > Description > ------- > > Right now default template for xen changed and test scripts are still point > to old template ,due to this deployvm and other scripts are failing and its > hard coded in each script. > > So I wrote a method to return built-in OS name dynamically. > > but here we need to add one line of code in each script > > > another solution is : > > we have get_template method and again this is depend on hardcoded parameter > type ostype.this is generic method . ,if modify exiting logic little bit(it > won't be generic and specific to built-in template)then no need to touch > other scripts. > > > Please review and share you input > > > Diffs > ----- > > tools/marvin/marvin/integration/lib/common.py 550de1a > > Diff: https://reviews.apache.org/r/17789/diff/ > > > Testing > ------- > > yes > > > Thanks, > > suresh sadhu > >
