Alona Kaplan has posted comments on this change. Change subject: engine: replace NetworkUtils.getDefaultManagementNetwork calls ......................................................................
Patch Set 15: Code-Review-1 (6 comments) http://gerrit.ovirt.org/#/c/34243/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsDeploy.java: Line 416: final Guid clusterId = _vds.getVdsGroupId(); Line 417: final Network managementNetwork = managmentNetworkUtil.getManagementNetwork(clusterId); Line 418: _parser.cliEnvironmentSet( Line 419: VdsmEnv.MANAGEMENT_BRIDGE_NAME, Line 420: managementNetwork.getName()); Please format Line 421: } Line 422: else { Line 423: _parser.cliNoop(); Line 424: } http://gerrit.ovirt.org/#/c/34243/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java: Line 301: return ValidationResult.VALID; Line 302: } Line 303: Line 304: public ValidationResult notRenamingManagementNetwork() { Line 305: return managementNetworkUtil.isManagementNetwork(network.getId()) You will throw a validation error in any case management network is updated. I don't think it is the purpose of the notRenamingManagementNetwork() validation. It should just check if the mgmt network was renamed. Although I"m not sure we need this validation any more since we don't identify the mgmt network by its name. Line 306: ? new ValidationResult(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK) Line 307: : ValidationResult.VALID; Line 308: } Line 309: http://gerrit.ovirt.org/#/c/34243/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkToVdsInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/UpdateNetworkToVdsInterfaceCommand.java: Line 227: getReturnValue().getCanDoActionMessages() Line 228: .add(VdcBllMessages.NETWORK_DEFAULT_UPDATE_NAME_INVALID.toString()); Line 229: final Network managementNetwork = managementNetworkUtil.getManagementNetwork(clusterId); Line 230: getReturnValue().getCanDoActionMessages().add(String.format("$NetworkName %1$s", Line 231: managementNetwork.getName())); You can use- getParameters().getOldNetworkName(), no need to query the db. Line 232: return false; Line 233: } Line 234: Line 235: if (StringUtils.isNotEmpty(getParameters().getGateway())) { http://gerrit.ovirt.org/#/c/34243/15/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkValidatorTest.java: Line 131: } Line 132: Line 133: @Test Line 134: public void managementNetworkRenamed() { Line 135: when(managementNetworkUtil.isManagementNetwork(NETWORK_ID)).thenReturn(true); Notice there is a comment about the renaming in the commands- please rewrite/remove the test according to the changes in the command. Line 136: Line 137: assertThat(validator.notRenamingManagementNetwork(), Line 138: failsWith(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK)); Line 139: } Line 138: failsWith(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK)); Line 139: } Line 140: Line 141: @Test Line 142: public void managementNetworkNotRenamed() { same. Line 143: when(managementNetworkUtil.isManagementNetwork(NETWORK_ID)).thenReturn(false); Line 144: Line 145: assertThat(validator.notRenamingManagementNetwork(), isValid()); Line 146: } http://gerrit.ovirt.org/#/c/34243/15/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java: Line 27: * majority of the cases. Line 28: * Line 29: * @return the default management network name. Line 30: */ Line 31: public static String getDefaultManagementNetworkName() { I would move this method to ManagementNetowrkUtil Line 32: return Config.<String> getValue(ConfigValues.DefaultManagementNetwork); Line 33: } Line 34: Line 35: public static Integer getDefaultMtu() { -- To view, visit http://gerrit.ovirt.org/34243 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0bd50e16b577b3f31765011d9b16501149c2479 Gerrit-PatchSet: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@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
