Lior Vernia has posted comments on this change. Change subject: engine: Consider custom properties in Setup Networks ......................................................................
Patch Set 6: (6 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. setSuppor Done Line 66: hostNetworkQosSupported = FeatureSupported.hostNetworkQos(vds.getVdsGroupCompatibilityVersion()); Line 67: networkCustomPropertiesSupported = Line 68: FeatureSupported.networkCustomProperties(vds.getVdsGroupCompatibilityVersion()); Line 69: } Line 235: util.convertProperties(Config.<String> getValue(ConfigValues.PreDefinedNetworkCustomProperties, version)); Line 236: validProperties.putAll(util.convertProperties(Config.<String> getValue(ConfigValues.UserDefinedNetworkCustomProperties, Line 237: version))); Line 238: for (VdsNetworkInterface iface : params.getInterfaces()) { Line 239: if (iface.getCustomProperties() != null) { > is it allowed/possible to have custom properties on a nic which has no netw Done 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()) { 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 informatio Done 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-15 Not exactly the same two lines, but improved 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.ACT Done Line 406: @Test Line 407: public void customPropertiesNotSupported() { Line 408: Network network = createNetwork(MANAGEMENT_NETWORK_NAME); Line 409: mockExistingNetworks(network); http://gerrit.ovirt.org/#/c/26646/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommand.java: Line 79: opts.put(DEFAULT_ROUTE, Boolean.TRUE); Line 80: } Line 81: Line 82: if (iface.getCustomProperties() != null) { Line 83: opts.put(VdsProperties.NETWORK_CUSTOM_PROPERTIES, iface.getCustomProperties()); > is there a point in sending an empty map ? Done Line 84: } Line 85: Line 86: networks.put(network.getName(), opts); Line 87: } -- 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 <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
