Alona Kaplan has posted comments on this change. Change subject: restapi: Add support for Network Attachements. ......................................................................
Patch Set 38: (8 comments) https://gerrit.ovirt.org/#/c/32775/38//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-09-10 10:23:33 +0300 Line 4: Commit: Martin Mucha <mmu...@redhat.com> Line 5: CommitDate: 2015-06-03 10:55:09 +0200 Line 6: Line 7: restapi: Add support for Network Attachements. This patch contains also non restapi code. Please create a new patch with all the non restapi related classes. Line 8: Line 9: A network attachment represents the network as it Line 10: designed to be configured on the host and allow the Line 11: user to use it for managing network on hosts. https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java: Line 64: IpConfiguration ipConfiguration = attachment.getIpConfiguration(); Line 65: return ValidationResult.failWith(VdcBllMessages.NETWORK_ADDR_MANDATORY_IN_STATIC_IP) Line 66: .unless(ipConfiguration == null Line 67: || ipConfiguration.getBootProtocol() != NetworkBootProtocol.STATIC_IP Line 68: || ipConfiguration.hasPrimaryAddressSet() && ipConfiguration.getPrimaryAddress() != null && ipConfiguration.getPrimaryAddress().getNetmask() != null); 'ipConfiguration.getPrimaryAddress() != null' redundant, Sounds like ipConfiguration.hasPrimaryAddressSet() should check it. Line 69: } Line 70: Line 71: public ValidationResult bootProtocolSetForDisplayNetwork() { Line 72: IpConfiguration ipConfiguration = attachment.getIpConfiguration(); https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IPv4Address.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IPv4Address.java: Line 24: @Pattern(regexp = ValidationUtils.IP_PATTERN, message = "NETWORK_ADDR_IN_GATEWAY_BAD_FORMAT") Line 25: @Size(max = BusinessEntitiesDefinitions.GENERAL_GATEWAY_SIZE) Line 26: private String gateway; Line 27: Line 28: public static long getSerialVersionUID() { Why do you need this getter? Line 29: return serialVersionUID; Line 30: } Line 31: Line 32: public String getAddress() { https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IpConfiguration.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/IpConfiguration.java: Line 42: } Line 43: Line 44: public IPv4Address getPrimaryAddress() { Line 45: if (!hasPrimaryAddressSet()) { Line 46: throw new IllegalStateException("IpConfiguration does not have set IPv4 address set."); 1. s/does not have set IPv4 address set/does not have IPv4 address set 2. Why not returning null in case the primaryAddress is not set. It will save you the need to call hasPrimaryAddressSet before each call the getPrimaryAddress. It is more intuitive to check if getPrimaryAddress() != null before using it. Line 47: } Line 48: return getIPv4Addresses().get(0); Line 49: } Line 50: Line 48: return getIPv4Addresses().get(0); Line 49: } Line 50: Line 51: public boolean hasPrimaryAddressSet() { Line 52: return IPv4Addresses != null && !IPv4Addresses.isEmpty(); Please also check the the primaryAddress is not null (a null value can be added to ArrayList). Line 53: } Line 54: Line 55: public void setIPv4Addresses(List<IPv4Address> IPv4Addresses) { Line 56: this.IPv4Addresses = IPv4Addresses; https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java: Line 57: } Line 58: Line 59: private void mapIpConfiguration(MapSqlParameterSource mapper, IpConfiguration ipConfiguration) { Line 60: boolean hasPrimaryAddressSet = ipConfiguration.hasPrimaryAddressSet(); Line 61: IPv4Address primaryAddress = hasPrimaryAddressSet ? ipConfiguration.getPrimaryAddress() : null; If ipConfiguration.getPrimaryAddress() will return null if it is not set, you won't need this trinary if. Line 62: Line 63: mapper.addValue("boot_protocol", EnumUtils.nameOrNull(ipConfiguration.getBootProtocol())) Line 64: .addValue("address", hasPrimaryAddressSet ? primaryAddress.getAddress() : null) Line 65: .addValue("netmask", hasPrimaryAddressSet ? primaryAddress.getNetmask() : null) Line 60: boolean hasPrimaryAddressSet = ipConfiguration.hasPrimaryAddressSet(); Line 61: IPv4Address primaryAddress = hasPrimaryAddressSet ? ipConfiguration.getPrimaryAddress() : null; Line 62: Line 63: mapper.addValue("boot_protocol", EnumUtils.nameOrNull(ipConfiguration.getBootProtocol())) Line 64: .addValue("address", hasPrimaryAddressSet ? primaryAddress.getAddress() : null) primaryAddress != null instead of hasPrimaryAddressSet is more intuitive. Line 65: .addValue("netmask", hasPrimaryAddressSet ? primaryAddress.getNetmask() : null) Line 66: .addValue("gateway", hasPrimaryAddressSet ? primaryAddress.getGateway() : null); Line 67: } Line 68: https://gerrit.ovirt.org/#/c/32775/38/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java: Line 82: Line 83: return expected; Line 84: } Line 85: Line 86: public IPv4Address createPrimaryAddress() { The default values of IPv4Address are null, this method is not needed. Line 87: IPv4Address primaryAddress = new IPv4Address(); Line 88: primaryAddress.setAddress(null); Line 89: primaryAddress.setNetmask(null); Line 90: primaryAddress.setGateway(null); -- To view, visit https://gerrit.ovirt.org/32775 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6ac91eea1000f7fdf6105777343a1ac1c77368d Gerrit-PatchSet: 38 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
