Martin Mucha has posted comments on this change. Change subject: engine: Introduce HostSetupNetworks command ......................................................................
Patch Set 47: (6 comments) https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/AddNetworkAttachmentCommand.java: Line 34: } Line 35: Line 36: @Override Line 37: protected boolean canDoAction() { Line 38: // VDS host = getVds(); > Please remove the commented code and consequently, remove this class from t Done Line 39: // Line 40: // FailingValidationResults<String> validationResults = new HostValidator(host, isInternalExecution()).validate(); Line 41: // if (!validationResults.isValid()) { Line 42: // getReturnValue().getCanDoActionMessages().addAll(validationResults.translateToViolationMessages()); https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostSetupNetworksCommand.java: Line 95: protected boolean canDoAction() { Line 96: qosDao = getDbFacade().getHostNetworkQosDao(); Line 97: Line 98: VDS host = getVds(); Line 99: > You should execute HostValidator.hostExists(..) here (before the completer) If it's previous patch, lest discuss it there, also if you mentioned it in previous patch already, it's nothing but duplicate here. Done. Line 100: NetworkAttachmentCompleter completer = new NetworkAttachmentCompleter(getExistingNics()); Line 101: completer.completeNicNames(getParameters().getNetworkAttachments()); Line 102: completer.completeBondNames(getParameters().getBonds()); Line 103: https://gerrit.ovirt.org/#/c/37154/47/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 99: && validRemovedBonds() Line 100: && validateNotRemovingUsedNetworkByVms()) { Line 101: Line 102: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 103: boolean valid = networksUniquelyConfiguredOnHost(attachmentsToConfigure) > The valid parameter is never used. how do you expect me to fix this unused variable? All other options seems worse. Hint: is "methodA() && methodB();" valid java statement? Line 104: && !violations.addViolations(new HostValidator(host, internalExecution).validate()) Line 105: && violations.addViolation(validateNetworkExclusiveOnNics(attachmentsToConfigure)) Line 106: && validateMtu(attachmentsToConfigure) Line 107: && validateCustomProperties(); Line 101: Line 102: Collection<NetworkAttachment> attachmentsToConfigure = getAttachmentsToConfigure(); Line 103: boolean valid = networksUniquelyConfiguredOnHost(attachmentsToConfigure) Line 104: && !violations.addViolations(new HostValidator(host, internalExecution).validate()) Line 105: && violations.addViolation(validateNetworkExclusiveOnNics(attachmentsToConfigure)) > Shoudn't you add '!' (not) here. I will remove addValidation methods and replace them with validate. If possible after we decide how error handling will be done. Done. Line 106: && validateMtu(attachmentsToConfigure) Line 107: && validateCustomProperties(); Line 108: Line 109: // TODO: Cover qos change not supported and network sync. see SetupNetworkHelper.validateNetworkQos() Line 341: continue; Line 342: } Line 343: Line 344: String networkId = attachment.getNetworkId() == null ? "" : attachment.getNetworkId().toString(); Line 345: if (!(violations.addViolation(validator.networkExists(), networkId) > should be- will fix it once we know how should error handling look like. (multiple errors/ multiple violations/ sole error with sole violation). This decision will probably make this comment obsolete.Done. Line 346: && violations.addViolation(referencedNetworkAttachmentActuallyExists(attachment.getId()), networkId) Line 347: && violations.addViolation(validator.notExternalNetwork(), networkId) Line 348: && violations.addViolation(validator.networkAttachedToCluster(), networkId) Line 349: && violations.addViolation(validator.ipConfiguredForStaticBootProtocol(), networkId) Line 388: boolean passed = true; Line 389: for (NetworkAttachment attachment : this.removedNetworkAttachments) { Line 390: NetworkAttachment attachmentToValidate = attachmentsById.get(attachment.getId()); Line 391: NetworkAttachmentValidator validator = new NetworkAttachmentValidator(attachmentToValidate, host, managementNetworkUtil); Line 392: if (!(violations.addViolation(validator.networkAttachmentIsSet(), attachment.getId().toString()) > same here. Done Line 393: && violations.addViolation(validator.notExternalNetwork()) Line 394: && violations.addViolation(validator.notManagementNetwork()) Line 395: && notRemovingLabeledNetworks(attachment, getExistingIfaces()))) { Line 396: passed = false; -- To view, visit https://gerrit.ovirt.org/37154 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If92e3476ad490d5bd5957358ad69be6ace4fd4fa Gerrit-PatchSet: 47 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
