> On Dec. 11, 2013, 8:57 a.m., Sebastien Goasguen wrote: > > tools/marvin/marvin/marvinInit.py, line 32 > > <https://reviews.apache.org/r/15922/diff/1/?file=392621#file392621line32> > > > > Pretty sure this is not PEP8 compliant
I just ran the pep8 against this module and it worked to show no errors. > On Dec. 11, 2013, 8:57 a.m., Sebastien Goasguen wrote: > > tools/marvin/marvin/deployDataCenter.py, line 610 > > <https://reviews.apache.org/r/15922/diff/1/?file=392620#file392620line610> > > > > Why importing at this time ? deployDataCenter as a module should do deploy related functionalities. Currently\Earlier, it does parsing config,creating test client etc and some other non related functionalities. As part of this change and later, we did clean up and will do few more. Individual components as per responsibility principle should follow their responsibilities only. So, moved parsing config away from this module, we will move creating test client as well from this module and so streamline other changes as well. Now, we have seen many people use deployDatacenter as a standalone module to deployDC and use it as a script, One of the requirement for deployDC is parsed config and logger. So getSetupConfig is ideally not required at the module load time but only when it is run as a script? So, import at this stage to keep backward compatability for people using deployDC to run deployement. > On Dec. 11, 2013, 8:57 a.m., Sebastien Goasguen wrote: > > tools/marvin/marvin/deployDataCenter.py, line 34 > > <https://reviews.apache.org/r/15922/diff/1/?file=392620#file392620line34> > > > > Why removing the check for cfgFile ? what happens if it's not there ? Iam not able to see this statement in latest code, where is this raise exception message coming from? - Santhosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15922/#review30187 ----------------------------------------------------------- On Nov. 30, 2013, 6:27 p.m., Santhosh Edukulla wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15922/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2013, 6:27 p.m.) > > > Review request for cloudstack and Girish Shilamkar. > > > Repository: cloudstack-git > > > Description > ------- > > Added few enhancements to marvin. > Added new module for Logging Facility to marvin. > Added new Init facility to marvin. > Currently, there are multiple ways we are doing logging > Removed few unwanted logging cases. > Removed few command line switch options for logging. > The new way of logging now provides consolidated logging > under one single folder timestamped under the configured > folder path. > Removed parsing configuration from deploydata center > Added parsing,start logging and deploy as part of init > Added new error handling facility to catch unknown exception from > test cases. Currently, lot of scripts are throwing unknown > exceptions, add a handler to plugin to dump them to a file > > ToDO: > Will do clean up in phase2 for this patch. > Separate deployDatacenter from creating test client. > Clean up configGenerator > > > Diffs > ----- > > setup/dev/advanced.cfg 216314f > tools/marvin/marvin/cloudstackConnection.py 23f81fb > tools/marvin/marvin/codes.py f409c7c > tools/marvin/marvin/configGenerator.py 631e40f > tools/marvin/marvin/deployDataCenter.py 3f7eebb > tools/marvin/marvin/marvinInit.py PRE-CREATION > tools/marvin/marvin/marvinLog.py PRE-CREATION > tools/marvin/marvin/marvinPlugin.py 0e52bab > > Diff: https://reviews.apache.org/r/15922/diff/ > > > Testing > ------- > > Tested locally. Ran smoke tests. As well, run deploydatacenter separately to > check deployement is working fine. > > > Thanks, > > Santhosh Edukulla > >
