Moti Asayag has posted comments on this change. Change subject: engine: Consider custom properties in Setup Networks ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/26646/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java: Line 61: Line 62: public SetupNetworksHelper(SetupNetworksParameters parameters, VDS vds) { Line 63: params = parameters; Line 64: this.vds = vds; Line 65: the two lines could be extracted into a method of their own, i.e. setSupportedFeatures(...) Line 66: hostNetworkQosSupported = FeatureSupported.hostNetworkQos(vds.getVdsGroupCompatibilityVersion()); Line 67: networkCustomPropertiesSupported = Line 68: FeatureSupported.networkCustomProperties(vds.getVdsGroupCompatibilityVersion()); Line 69: } Line 239: getCustomProperties is it allowed/possible to have custom properties on a nic which has no network configured on? Line 239: if (iface.getCustomProperties() != null) { Line 240: if (!networkCustomPropertiesSupported) { Line 241: addViolation(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_CUSTOM_PROPERTIES_NOT_SUPPORTED, Line 242: iface.getNetworkName()); Line 243: } else if (!util.validateProperties(validProperties, iface.getCustomProperties()).isEmpty()) { if the returned failed validation list contains a human readable information, this is a good place to print the wrong parameters to the log and direct the user for further information about the misconfiguration from the can-do-action message. Line 244: addViolation(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_CUSTOM_PROPERTIES_BAD_INPUT, Line 245: iface.getNetworkName()); Line 246: } Line 247: } Line 534: Line 535: return !ObjectUtils.equals(iface.getNetworkName(), existingIface.getNetworkName()) Line 536: || iface.getBootProtocol() != existingIface.getBootProtocol() Line 537: || staticBootProtoPropertiesChanged(iface, existingIface) Line 538: || !ObjectUtils.equals(iface.getQos(), existingIface.getQos()) this 2 lines are also repeated in extractModifiedInterfaces() (lines 149-150). You can extract them into a method and reuse it. Line 539: || !ObjectUtils.equals(iface.getCustomProperties(), existingIface.getCustomProperties()); Line 540: } Line 541: Line 542: /** http://gerrit.ovirt.org/#/c/26646/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelperTest.java: Line 401: SetupNetworksHelper helper = createHelper(createParametersForNics(iface), Version.v3_4); Line 402: Line 403: validateAndAssertNetworkModified(helper, network); Line 404: } Line 405: please another test to cover the case of bad parameters: VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_CUSTOM_PROPERTIES_BAD_INPUT Line 406: @Test Line 407: public void customPropertiesNotSupported() { Line 408: Network network = createNetwork(MANAGEMENT_NETWORK_NAME); Line 409: mockExistingNetworks(network); -- To view, visit http://gerrit.ovirt.org/26646 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic607fb6073f2c4c0a4790763d9a09dc7879f11ca Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
