Alona Kaplan has posted comments on this change. Change subject: core: refactor&test for HostNetworkAttachmentsPersister ......................................................................
Patch Set 37: (13 comments) https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 543: NETWORK_NOT_EXISTS(ErrorType.BAD_PARAMETERS), Line 544: ACTION_TYPE_FAILED_NETWORK_QOS_NOT_EXISTS(ErrorType.BAD_PARAMETERS), Line 545: NETWORK_NOT_EXISTS_IN_CLUSTER(ErrorType.BAD_PARAMETERS), Line 546: NETWORK_ATTACHMENT_NOT_EXISTS(ErrorType.BAD_PARAMETERS), Line 547: NETWORK_ATTACHMENT_CONTAINS_DUPLICATES(ErrorType.BAD_PARAMETERS), 1. The name here doesn't match the name in the AppErrors files- ACTION_TYPE_FAILED_NETWORK_ATTACHMENT_CONTAINS_DUPLICATES 2. Where do you use this error? Refer to my comment on HostNetworkAttachmentsPersister.verifyUserAttachmentsDoesNotReferenceSameNetworkDuplicately(..) Seems your intent was correct, you need to add this error to a new validation in NetworkAttachmentsValitors (instead of throwing exception in the persister). Line 548: NETWORK_ALREADY_ATTACHED_TO_HOST(ErrorType.CONFLICT), Line 549: NETWORK_OLD_NETWORK_NOT_SPECIFIED(ErrorType.BAD_PARAMETERS), Line 550: NETWORK_ALREADY_ATTACHED_TO_CLUSTER(ErrorType.CONFLICT), Line 551: NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK(ErrorType.CONSTRAINT_VIOLATION), https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 97: return containsNetwork(existingNetworkAttachments, network); Line 98: } Line 99: Line 100: private boolean containsNetwork(List<NetworkAttachment> networkAttachments, Network network) { Line 101: for (NetworkAttachment networkAttachment : networkAttachments) { You can use -Linq.firstOrNull to find out whether the list contains networkAttachment with the specified networkId. Line 102: if (network.getId().equals(networkAttachment.getNetworkId())) { Line 103: return true; Line 104: } Line 105: } Line 109: Line 110: /** Line 111: * Saves or updates network attachments which were provided by the user and were reported by the host Line 112: * @return newly create network attachments. Line 113: * @param validNetworkAttachments Shouldn't the order be opposite? First the param and then return value. Line 114: */ Line 115: private List<NetworkAttachment> saveUserNetworkAttachments(List<NetworkAttachment> validNetworkAttachments) { Line 116: List<NetworkAttachment> newlyCreatedNetworkAttachments = new ArrayList<>(); Line 117: Line 136: Line 137: return newlyCreatedNetworkAttachments; Line 138: } Line 139: Line 140: private void verifyUserAttachmentsDoesNotReferenceSameNetworkDuplicately(List<NetworkAttachment> userNetworkAttachments) { This should have been verified in the candoaction of the relevant commands. Why do you verify it here again? If it is not verified (and it seems it isn't) please add to the relevant validator (NetworkAttachmentsValidator I think). Line 141: Set<Guid> networkIds = new HashSet<>(userNetworkAttachments.size()); Line 142: for (NetworkAttachment networkAttachment : userNetworkAttachments) { Line 143: networkIds.add(networkAttachment.getNetworkId()); Line 144: } https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java: Line 149: verifyNoMoreInteractions(networkAttachmentDao); Line 150: } Line 151: Line 152: @Test Line 153: public void testPersistNetworkAttachmentsWhenCalledWithNewUserAttachments() throws Exception { The method looks very similar to the previous one. Please create a common method and reuse it. Line 154: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))).thenReturn(new ArrayList<NetworkAttachment>()); Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 150: } Line 151: Line 152: @Test Line 153: public void testPersistNetworkAttachmentsWhenCalledWithNewUserAttachments() throws Exception { Line 154: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))).thenReturn(new ArrayList<NetworkAttachment>()); Please statically import when Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 159: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); 1. The id passed by the user is not verified by the canDoActions, so it cannot be used here. Therefore it should ignored. Please remove the TODO from the comment. 2. s/useProvidedNetworkAttachmentId/userProvidedNetworkAttachmentId Line 160: Line 161: //user sends some possibly nonsensical value, but code will override it for him //TODO MM: is this really right? Line 162: userNetworkAttachment.setNicId(Guid.newGuid()); Line 163: Line 157: Line 158: //when persisting new record user provided Id should be ignored and replaced. //TODO MM: is this really right? How about rejecting user input? Line 159: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); Line 160: Line 161: //user sends some possibly nonsensical value, but code will override it for him //TODO MM: is this really right? As I wrote on the previous patch, I don't see any reason overriding it. Line 162: userNetworkAttachment.setNicId(Guid.newGuid()); Line 163: Line 164: //user attachments references network, which is not assigned to NIC. Line 165: createPersister(Arrays.asList(userNetworkAttachment)).persistNetworkAttachments(); Line 182: Line 183: @Test Line 184: public void testPersistNetworkAttachmentsWhenCalledWithAlreadyExistingAttachmentItWillUpdated() throws Exception { Line 185: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 186: Guid useProvidedNetworkAttachmentId = userNetworkAttachment.getId(); s/useProvidedNetworkAttachmentId/userProvidedNetworkAttachmentId Line 187: Line 188: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))) Line 189: .thenReturn(new ArrayList<>(Arrays.asList(userNetworkAttachment))); Line 190: Line 188: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))) Line 189: .thenReturn(new ArrayList<>(Arrays.asList(userNetworkAttachment))); Line 190: Line 191: Line 192: //user attachments references network, which is not assigned to NIC. Please add space after the // - relevant to all the comments in the class. Line 193: createPersister(Arrays.asList(userNetworkAttachment)).persistNetworkAttachments(); Line 194: Line 195: verify(networkAttachmentDao).getAllForHost(any(Guid.class)); Line 196: Line 201: assertThat(networkAttachmentCaptor.getValue().getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 202: //new id will be generated for persisted record Line 203: assertThat(networkAttachmentCaptor.getValue().getId(), equalTo(useProvidedNetworkAttachmentId)); Line 204: assertThat(networkAttachmentCaptor.getValue().getIpConfiguration(), is(userNetworkAttachment.getIpConfiguration())); Line 205: assertThat(networkAttachmentCaptor.getValue().getNetworkId(), is(userNetworkAttachment.getNetworkId())); What about properties? Line 206: Line 207: Line 208: //verify that nothing else happens, no removals, no creations. Line 209: verifyNoMoreInteractions(networkAttachmentDao); Line 261: Line 262: assertThat(networkAttachmentCaptor.getValue().getNetworkId(), is(clusterNetworkA.getId())); Line 263: assertThat(networkAttachmentCaptor.getValue().getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 264: assertThat(networkAttachmentCaptor.getValue().getId(), notNullValue()); Line 265: //TODO MM: verify also properties? do we want to be so specific? what about properties and ipConfiguration? Line 266: Line 267: //verify that nothing else happens, namely, interfaceWithoutAttachedNetwork will not trigger persisting any data. Line 268: verifyNoMoreInteractions(networkAttachmentDao); Line 269: } https://gerrit.ovirt.org/#/c/36067/37/frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: Line 487: NETWORK_INTERFACE_ALREADY_HAVE_NETWORK=The Network Interface is already attached to a Logical Network. Line 488: NETWORK_ALREAY_ATTACH_TO_INTERFACE=Logical Network is already attached to a Network Interface. Line 489: NETWORK_NOT_EXISTS=The specified Logical Network doesn't exist. Line 490: ACTION_TYPE_FAILED_NETWORK_QOS_NOT_EXISTS=The specified Network QoS doesn't exist. Line 491: ACTION_TYPE_FAILED_NETWORK_ATTACHMENT_CONTAINS_DUPLICATES=Provided NetworkAttachments contains duplicates. This error is not relevant for user-portal. You don't need to add it to this file. Line 492: ACTION_TYPE_FAILED_VNIC_PROFILE_NOT_EXISTS=Cannot ${action} ${type}. The specified VM network interface profile doesn't exist. Line 493: ACTION_TYPE_FAILED_VNIC_PROFILE_NAME_IN_USE=Cannot ${action} ${type}. The VM network interface profile's name is already used by an existing profile for the same network.\n-Please choose a different name. Line 494: ACTION_TYPE_FAILED_VNIC_PROFILE_IN_USE=Cannot ${action} ${type}. Several ${entities} (${ENTITIES_USING_VNIC_PROFILE_COUNTER}) are using this VM network interface profile:\n${ENTITIES_USING_VNIC_PROFILE}\n - Please remove it from all ${entities} that are using it and try again. Line 495: ACTION_TYPE_FAILED_CANNOT_CHANGE_VNIC_PROFILE_NETWORK=Cannot ${action} ${type}. VM network interface profile's network cannot be changed. -- To view, visit https://gerrit.ovirt.org/36067 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib39c3678a29a7bd8728d32b55490f8500a77d9d6 Gerrit-PatchSet: 37 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
