Alona Kaplan has posted comments on this change. Change subject: core: refactor&test for HostNetworkAttachmentsPersister ......................................................................
Patch Set 37: (3 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), > ok. This is highly out of date. The error should be removed from this patch, since it is not related to it. But please add this validation (in a separate patch) to the NetworkAttachmentsValidator. You have to make sure the user didn't pass the same network on more than one attachment. Added the relevant comment to- https://gerrit.ovirt.org/#/c/34968/23/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentsValidator.java 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/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 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(); > I'm not sure if we understand each other. What I tried to say is, that if u I think you can simply override it (with no TODO:)). There is no need for cando (I don't see this kind of cando on other commands). But of-course, if you decide to add cando, it is also ok. Just choose one of the approaches and don't live the code with TODOs:) 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? > see above. How is the above related. Here the issue is the nic_id not the attachment id. The nic_id supplied by the user (in this stage the attachment is after the completion and NicIdNicNameCompleter) should be used. I case the nic_id supplied by the user doesn't match the reported nic_id, the attachment shouldn't be persisted. Please add test for this use case. 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(); -- 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
