Added Virtualmachine count and ID's to listSecurityGroups response#679
Added Virtualmachine count and ID's to listSecurityGroups response#679borisroman wants to merge 2 commits into
Conversation
|
cloudstack-pull-rats #260 SUCCESS |
|
cloudstack-pull-requests #957 SUCCESS |
|
cloudstack-pull-analysis #193 SUCCESS |
There was a problem hiding this comment.
Add a check that findById is not returning null. There is a possibility that VM may get deleted after the call to listBySecurityGroup() but before that map entry is processed in the for-loop.
|
Please add tests. |
|
cloudstack-pull-rats #266 SUCCESS |
|
cloudstack-pull-requests #963 SUCCESS |
|
cloudstack-pull-analysis #199 SUCCESS |
|
@koushik-das Added tests. Please have a look! |
|
cloudstack-pull-rats #274 FAILURE |
f3deb5d to
8e35315
Compare
|
cloudstack-pull-rats #275 FAILURE |
|
cloudstack-pull-requests #971 SUCCESS |
|
cloudstack-pull-requests #972 SUCCESS |
|
cloudstack-pull-analysis #207 SUCCESS |
|
cloudstack-pull-analysis #208 SUCCESS |
There was a problem hiding this comment.
Could you enclose this if by brackets? { }
|
LGTM if you fix the brackets in the if-statement |
|
cloudstack-pull-rats #283 SUCCESS |
|
cloudstack-pull-requests #980 ABORTED |
|
cloudstack-pull-analysis #216 ABORTED |
|
@borisroman Could you please squash the commits into 1 (or maybe 2 if you want the tests separate). |
There was a problem hiding this comment.
@borisroman Please make findById() call only once. Something like
UserVMVO vm = _userVmDao.findById(securityGroupVMMapVO.getInstanceId());
if (vm != null) { sgResponse.addVirtualMachineId(vm.getUuid()); }
|
@borisroman I have provided a code comment. Once you fix that and squash the commits as suggested by @remibergsma then its a LGTM from me. Thanks for the unit tests. |
…oups response. See issue CLOUDSTACK-8133 for more information. Added null check by comment of Koushik Das. Added brackets by comment of Wido den Hollander. Removed a call to findById() by comment of Koushik Das.
Tests will confirm the behaviour of the newly added response fields of listSecurityGroups.
d853db8 to
6977473
Compare
|
@wido @remibergsma @koushik-das Folowed up on lastest comments by Remi and Koushik. |
|
cloudstack-pull-rats #285 SUCCESS |
|
cloudstack-pull-requests #982 SUCCESS |
See issue CLOUDSTACK-8133 for more information.