You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alex Brett <al...@citrix.com> on 2014/07/01 16:34:46 UTC
Review Request 23197: CLOUDSTACK-7031 Improve error handling in
deployDataCenter.py
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23197/
-----------------------------------------------------------
Review request for cloudstack and SrikanteswaraRao Talluri.
Bugs: CLOUDSTACK-7031
https://issues.apache.org/jira/browse/CLOUDSTACK-7031
Repository: cloudstack-git
Description
-------
The DeployDataCenters class should not call sys.exit directly, instead we now raise an exception, which can be handled as required.
In addition this patch tidies up logging by removing calls to print from inside the class, and ensures after a successful deploy / remove, we return an exit code of 0, rather than the previous behaviour that always returned an exit code of 1.
Diffs
-----
tools/marvin/marvin/deployDataCenter.py c097238
Diff: https://reviews.apache.org/r/23197/diff/
Testing
-------
Run through pylint, manual testing now in progress to verify functionality...
Thanks,
Alex Brett
Re: Review Request 23197: CLOUDSTACK-7031 Improve error handling in
deployDataCenter.py
Posted by Santhosh Edukulla <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23197/#review47064
-----------------------------------------------------------
tools/marvin/marvin/deployDataCenter.py
<https://reviews.apache.org/r/23197/#comment82667>
Move this to inside cloudstackException module.
tools/marvin/marvin/deployDataCenter.py
<https://reviews.apache.org/r/23197/#comment82664>
print statements provide useful info to the people on the console, when they run it from shell and people use it as standalone script to deploy on various occasions. We can do one thing, use a local log function, pass the msg to it, there dump the message passed to both streams i.e., stdout and file and use that log function. Or enhance current logging module to provide facilitation where it can take an argument to dump to both stdout and file stream, default set to false.
tools/marvin/marvin/deployDataCenter.py
<https://reviews.apache.org/r/23197/#comment82668>
As well , please add me as reviewer
tools/marvin/marvin/deployDataCenter.py
<https://reviews.apache.org/r/23197/#comment82665>
Why we need both exception and debug message, is it not confusing both to read here as well in logs?
tools/marvin/marvin/deployDataCenter.py
<https://reviews.apache.org/r/23197/#comment82666>
If we raise exception here, will cleanup be called if some thing fails in between?
- Santhosh Edukulla
On July 1, 2014, 2:34 p.m., Alex Brett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23197/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 2:34 p.m.)
>
>
> Review request for cloudstack and SrikanteswaraRao Talluri.
>
>
> Bugs: CLOUDSTACK-7031
> https://issues.apache.org/jira/browse/CLOUDSTACK-7031
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> The DeployDataCenters class should not call sys.exit directly, instead we now raise an exception, which can be handled as required.
>
> In addition this patch tidies up logging by removing calls to print from inside the class, and ensures after a successful deploy / remove, we return an exit code of 0, rather than the previous behaviour that always returned an exit code of 1.
>
>
> Diffs
> -----
>
> tools/marvin/marvin/deployDataCenter.py c097238
>
> Diff: https://reviews.apache.org/r/23197/diff/
>
>
> Testing
> -------
>
> Run through pylint, manual testing now in progress to verify functionality...
>
>
> Thanks,
>
> Alex Brett
>
>