Alona Kaplan has posted comments on this change. Change subject: engine: update UpdateVdsGroupCommand ......................................................................
Patch Set 14: (8 comments) http://gerrit.ovirt.org/#/c/33813/14//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-10-23 16:04:12 +0300 Line 6: Line 7: engine: update UpdateVdsGroupCommand Line 8: Line 9: Update logic of attaching a cluster to a DC. Please add more informative comment. Line 10: Line 11: Change-Id: Ica33589687a6190c1fa595beda461470afe9a6db http://gerrit.ovirt.org/#/c/33813/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java: Line 112: } Line 113: Line 114: getVdsGroupDAO().update(getParameters().getVdsGroup()); Line 115: Line 116: if (oldGroup.getStoragePoolId() == null && getVdsGroup().getStoragePoolId() != null) { You can use- isAddedToStoragePool Line 117: for (VDS vds : allForVdsGroup) { Line 118: VdsActionParameters parameters = new VdsActionParameters(); Line 119: parameters.setVdsId(vds.getId()); Line 120: VdcReturnValueBase addVdsSpmIdReturn = runInternalAction(VdcActionType.AddVdsSpmId, parameters, cloneContextAndDetachFromParent()); Line 124: return; Line 125: } Line 126: } Line 127: Line 128: if (isAddedToStoragePool) { redundant- you are inside an if that has already checked it Line 129: getNetworkClusterDAO().save(managementNetworkCluster); Line 130: } Line 131: } Line 132: Line 410: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DEFAULT_MANAGEMENT_NETWORK_NOT_FOUND); Line 411: return false; Line 412: } Line 413: Line 414: managementNetworkCluster = createManagementNetworkCluster(managementNetwork); I think it is better to store the newly created networkCluster in managementNetworkCluster as part of createManagementNetworkCluster (you can rename it to getManagementNetworkCluster). Line 415: final UpdateClusterNetworkClusterValidator networkClusterValidator = createManagementNetworkClusterValidator(); Line 416: return validate(networkClusterValidator.managementNetworkChange()); Line 417: } Line 418: http://gerrit.ovirt.org/#/c/33813/14/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommandTest.java: Line 90: Line 91: @Mock Line 92: private Network mockManagementNetwork = createManagementNetwork(); Line 93: Line 94: private Network createManagementNetwork() { Please add tests for- 1. There is no default management network. 2. The is management network but the cluster is not empty. Line 95: final Network network = new Network(); Line 96: network.setId(TEST_MANAGEMENT_NETWORK_ID); Line 97: return network; Line 98: } Line 487: } Line 488: Line 489: private static VDSGroup createVdsGroupWithDifferentPool() { Line 490: VDSGroup group = createNewVdsGroup(); Line 491: group.setStoragePoolId(TEST_MANAGEMENT_NETWORK_ID); why do you use the TEST_MANAGEMENT_NETWORK_ID? Line 492: return group; Line 493: } Line 494: Line 495: private static VDSGroup createVdsGroupWith(boolean virtService, boolean glusterService) { http://gerrit.ovirt.org/#/c/33813/14/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateClusterNetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/UpdateClusterNetworkClusterValidatorTest.java: Line 16: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 17: Line 18: @RunWith(MockitoJUnitRunner.class) Line 19: public class UpdateClusterNetworkClusterValidatorTest Line 20: extends Please format with the standard formatter. Line 21: NetworkClusterValidatorTestBase<UpdateClusterNetworkClusterValidator> { Line 22: Line 23: @Override Line 24: protected UpdateClusterNetworkClusterValidator createValidator() { Line 38: Line 39: private void testUpdateManagementNetworkChange(boolean emptyCluster, Line 40: Matcher<ValidationResult> expectedResult) { Line 41: when(networkCluster.getClusterId()).thenReturn(TEST_CLUSTER_ID); Line 42: when(vdsDao.getAllForVdsGroup(TEST_CLUSTER_ID)).thenReturn(emptyCluster ? Please format. Line 43: Collections.<VDS> emptyList() : Line 44: Collections.<VDS> singletonList(null)); Line 45: assertThat(validator.managementNetworkChange(), expectedResult); Line 46: } -- To view, visit http://gerrit.ovirt.org/33813 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica33589687a6190c1fa595beda461470afe9a6db Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
