Yevgeny Zaspitsky has posted comments on this change. Change subject: engine: update UpdateVdsGroupCommand ......................................................................
Patch Set 14: (7 comments) 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 Done 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 Done 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 managemen 1. I'd prefer the assignment be explicit here in canDoAction method. 2. createManagementNetworkCluster method actually creates it rather than retrieves the member, what a typical getter does 3. having a parameter isn't a getter semantic. 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- Done 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? Done 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. Done 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. Done 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 <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
