Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 26: (7 comments) https://gerrit.ovirt.org/#/c/33333/26/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksValidator.java: Line 86: && validModifiedNetworkAttachments(attachmentsById) Line 87: && validRemovedNetworkAttachments(attachmentsById) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { > I think it is more readable and clear. And anyway, you're not testing valid I don't understand meaning in second sentence. Ok, I'll do it. Done. Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 88: && validModifiedBonds() Line 89: && validRemovedBonds() Line 90: && validateNotRemovingUsedNetworkByVms()) { Line 91: Line 92: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); > Same answer as the previous comment. You're not testing validModifiedNetwor a) no, it shouldn't be part of it. First batch just verifies input. This verifies if they can be applied. b) seriously, can you imagine how hard it is to do this change in whole this branch? Ok, I going to change my tone now. Please, mercy. I'm begging you. This code is shit and will be shit regardless from wherever we call certain method. Please, pretty please. I don't want to spend two days on this. What's the point anyways? So the method call will move, it will not make any difference, all will remain in exact horrible state. Please once again, forget about these changes. Line 93: if (networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 94: && validate(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 95: && validateMtu(attachmentsToConfigure) Line 96: && validateCustomProperties()) { Line 209: Line 210: private Collection<NetworkAttachment> getAttachmentsToConfigure() { Line 211: Map<Guid, NetworkAttachment> attachmentsToConfigure = new HashMap<>(Entities.businessEntitiesById(existingAttachments)); Line 212: // ignore removed attachments Line 213: for (NetworkAttachment removedAttachment : this.removedNetworkAttachments) { > I meant- s/this.removedNetworkAttachments/removedNetworkAttachments Done Line 214: attachmentsToConfigure.remove(removedAttachment.getId()); Line 215: } Line 216: Line 217: List<NetworkAttachment> newAttachments = new ArrayList<>(); Line 312: Line 313: return false; Line 314: } Line 315: Line 316: private boolean validModifiedNetworkAttachments(Map<Guid, NetworkAttachment> attachmentsById) { > I discussed it with Moti, and he and I think that removing the attachment a ok. why do we have to do it here? PLEASE do it as a separate patch after we just peek into ui code. The benefit would be, that if we figure out, that required UI code to support this will be nightmare, we can abandon this idea without touching this horrible patch. Jesus, there's more comments than code! Line 317: boolean passed = true; Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 318: for (NetworkAttachment attachment : params.getNetworkAttachments()) { Line 319: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachment, host, managementNetworkUtil); Line 320: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 321: if (!(validate(validator.networkAttachmentIsSet()) Line 322: && validate(validator.networkExists(), networkId) > Regarding 3- I don't understand given sentences. I've added todo, not to forget to add validation that networkId is set once problems with passing errors to UI is figured out.Done. Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 323: && validate(validator.notExternalNetwork(), networkId) Line 324: && validate(validator.networkAttachedToCluster(), networkId) Line 325: && validate(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 326: && validate(validator.bootProtocolSetForDisplayNetwork(), networkId) Line 327: && validate(validator.nicExists(), networkId) > Please refer to my comment on patchset-35. Maybe I miss something, but addi I totally not understand this sentence(s). I surely will look on comment on patchset 35 when I got to it. I'm skipping this comment altogether Line 328: && validate(validator.networkIpAddressWasSameAsHostnameAndChanged(getExistingIfaces())) Line 329: && validate(validator.networkNotChanged(attachmentsById.get(attachment.getId()))) Line 330: && validate(validator.validateGateway()))) { Line 331: passed = false; Line 339: boolean passed = true; Line 340: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 341: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 342: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 343: if (!(validate(validator.networkAttachmentIsSet()) > I agree, the validation should be removed. Done Line 344: && validate(validator.notExternalNetwork()) Line 345: && validate(validator.notManagementNetwork()) Line 346: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 347: passed = false; -- To view, visit https://gerrit.ovirt.org/33333 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I60077019f308f371f21fb7b5545ba48adb38bd06 Gerrit-PatchSet: 26 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: 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
