> On Feb. 27, 2014, 12:10 p.m., Laszlo Hornyak wrote: > > In general I believe it makes sense to return false in this case, since it > > is clear that the host was not fenced. > > > > At the same time the reviewers dislike havig formating (finals) in the > > patches, it might be easier to push it through the process if you could > > separate the cleanup from the fix. Also, is there a bug url that should be > > included?
Hi Laszlo, Concerning the "finals" it was due check-style combined with save actions in my Eclipse, just to make sure the code is according to the CS templates. I understand it kind of make a bit difficult to read the patch, but redoing it would require more effort than pushing it through. We can agree that for the next I will do the formatting first, commit and send the patch. Afterwards, I do the fix, squash the commit and update the patch. Is it a better idea? Concerning the bug URL, there is none. I just picked up a package and ran find Bugs on it. - Wilder ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18568/#review35618 ----------------------------------------------------------- On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18568/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2014, 8:03 a.m.) > > > Review request for cloudstack, daan Hoogland and Hugo Trippaers. > > > Repository: cloudstack-git > > > Description > ------- > > Fix for Find Bugs findings on troubling issues: returning null when expected > is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects > with == instead of equals() > > > Diffs > ----- > > plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > 48ae3ea > plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8 > server/src/com/cloud/ha/KVMFencer.java 516a579 > server/test/com/cloud/ha/KVMFencerTest.java cdd13b6 > > Diff: https://reviews.apache.org/r/18568/diff/ > > > Testing > ------- > > Added 6 unit tests for XenServerFencer (removed an useless one which was > testing get/set methods) > Build completed successfully > > Tested also on DevCloud + XenServer the following: > > Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and > System VMS > Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip > Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance > SSH into the instance > > > Thanks, > > Wilder Rodrigues > >
