Alona Kaplan has posted comments on this change. Change subject: engine: Add ModifiedNetworkAttachmentValidator ......................................................................
Patch Set 15: (2 comments) https://gerrit.ovirt.org/#/c/34967/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/ModifiedNetworkAttachmentValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/ModifiedNetworkAttachmentValidator.java: Line 36: getNetworkNameReplacement()) Line 37: .when(managementNetworkUtil.isManagementNetwork(getOldNetwork().getId())); Line 38: } Line 39: Line 40: public ValidationResult networkNotUsedByVms(String networkName) { > Done You marked it as done, but it is still public (patchset 24). Line 41: List<String> vmNames = Line 42: new VmInterfaceManager().findActiveVmsUsingNetworks(host.getId(), Line 43: Collections.singleton(networkName)); Line 44: Line 67: Line 68: return oldAttachment; Line 69: } Line 70: Line 71: private Network getOldNetwork() { > I don't understand what's 'internalized attachment'... internalized = initialized (oops...) And generally, when writing validations. it is ok to assume that more basic validations had run previously. What I tried to say in the previous comment, is that it is weird to have a method called getOldNetwork in this class, since there is no option for different old and new network entities, one of the most basic validation should be the network is not changed. Why aren't you using the regular getNetwork() method? Is it because it uses networkAttachment.netowrkId and there is a chance the networkAttachment passed to the ctor is not fully initialized and doesn't contain the networkId? If it is the case (and since the other validations assume the networkAttachment is fully initialized), I suggested to move the notRemovingManagementNetwork to RemoveNetworkAttachmentValidator, to avoid confusion. But now after rethinking it, the core problem is that you can pass a networkAttachment that is not fully initialized. It is because the parameter of RemoveNetworkAttachmentCommand is to wide. It should contain just hostId and attachmentId (and not the whole networkAttachment). Line 72: if (oldNetwork == null) { Line 73: oldNetwork = getDbFacade().getNetworkDao().get(getOldNetworkAttachment().getNetworkId()); Line 74: } Line 75: -- To view, visit https://gerrit.ovirt.org/34967 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie64dff04dbf43b8507b0559d82dc56a24e170e7d Gerrit-PatchSet: 15 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: 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
