Martin Mucha has posted comments on this change. Change subject: core: allowed to add network label even if Vm using this network is running. ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/32688/1//COMMIT_MSG Commit Message: Line 13: was not changed, or if former label was not specified (null) and new Line 14: one is specified (!=null). Line 15: Line 16: Change-Id: I57a658a4e2d4648917a265493cde4749ad15e3ad Line 17: Bug-Url: https://bugzilla.redhat.com/?????? > Please fix this to refer to 1134009. Done http://gerrit.ovirt.org/#/c/32688/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java: Line 114: } Line 115: Line 116: @Override Line 117: protected boolean canDoAction() { Line 118: if (onlyPermittedFieldsChanged() && allowedNetworkLabelManipulation()) { > I'm not sure if it is currently blocked somewhere (maybe only on UI side), I did some verification whether there should be more entry checks, and yes, from gui neither label cannot be added, not datacenter can be changed. But from REST I was able to do both. I can add both checks, and I have strong feeling that I should, but I'll do it in separate path. Ok? Or should I not do that for any reason? Line 119: return true; Line 120: } Line 121: Line 122: NetworkValidator validatorNew = new NetworkValidator(getNetwork()); Line 142: Line 143: private boolean allowedNetworkLabelManipulation() { Line 144: Network oldNetwork = getOldNetwork(); Line 145: Network newNetwork = getNetwork(); Line 146: > SyncNetworkParametersBuilder contains those logic in labelAdded() & labelCh thanks for recommendation. I used them, but I keep those two variables for simpler return statement. I do not insist on this form, just find it better this way. If you want to inline them, just ask for it. Also I'll try to refactor SyncNetworkParametersBuilder, since referenced methods are badly placed ~ to reach them, I need to create new instance using commandContext parameter, which is completely unrelated to those methods. Since SyncNetworkParametersBuilder is not static, they can easily be moved to upper level, to enclosing class. Fixed in following patch, please do CR. Line 147: boolean labelNotChanged = Objects.equals(oldNetwork.getLabel(), newNetwork.getLabel()); Line 148: boolean newLabelAssigned = oldNetwork.getLabel() == null && newNetwork.getLabel() != null; Line 149: Line 150: return labelNotChanged || newLabelAssigned; Line 160: if (oldNetwork == null || newNetwork == null) { Line 161: return false; Line 162: } Line 163: Line 164: return Objects.equals(oldNetwork.getName(), newNetwork.getName()) && I'd like to comment on this: there are just few getters being considered -- nine, and that's sum with getName() being there twice. But there are 17 settters: setAddr setDescription setComment setId setName setSubnet setGateway setType setVlanId setStp setDataCenterId setCluster setProvidedBy setLabel setMtu setVmNetwork setQosId so there are ways to change other fields which are not checked by this method. Properties description, commend and label are safe. Can somebody confirm, that these property changes are irrelevant to this method? setAddr setSubnet setGateway setType setCluster setQosId Line 165: Objects.equals(oldNetwork.getDataCenterId(), newNetwork.getDataCenterId()) && Line 166: Objects.equals(oldNetwork.getId(), newNetwork.getId()) && Line 167: Objects.equals(oldNetwork.getMtu(), newNetwork.getMtu()) && Line 168: Objects.equals(oldNetwork.getName(), newNetwork.getName()) && -- To view, visit http://gerrit.ovirt.org/32688 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I57a658a4e2d4648917a265493cde4749ad15e3ad Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
