Lior Vernia has posted comments on this change.
Change subject: webadmin: Vnic hotplug validation - Patch 2 of 2
......................................................................
Patch Set 3:
(11 comments)
Sorry man, originally misunderstood the commit message. Please see comments
throughout the patch.
....................................................
Commit Message
Line 6:
Line 7: webadmin: Vnic hotplug validation - Patch 2 of 2
Line 8:
Line 9: * This change adds an validation supports hotplug nic in the frontend.
Line 10: * Was added a validation to prevent an user to do an nic hotplug in an
This is actually not correct and what confused me. A user may still hot-unplug
an interface with a VM status different than down, as long as hotplug is
supported. Please correct the message.
Line 11: VM with status different of down.
Line 12:
Line 13: Change-Id: I2434386f98c31a8d0fe44011ecadecb732ded19a
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/BaseEditVmInterfaceModel.java
Line 21:
Line 22: public abstract class BaseEditVmInterfaceModel extends
VmInterfaceModel {
Line 23:
Line 24: VmNetworkInterface nic;
Line 25: VMStatus vmStatus;
You've added this both in BaseEditVmInterfaceModel and in NewVmInterfaceModel.
So why not add it only once in their shared ancestor, VmInterfaceModel?
Line 26:
Line 27: protected BaseEditVmInterfaceModel(VmBase vm,
Line 28: VMStatus vmStatus, Guid dcId,
Line 29: Version clusterCompatibilityVersion,
Line 24: VmNetworkInterface nic;
Line 25: VMStatus vmStatus;
Line 26:
Line 27: protected BaseEditVmInterfaceModel(VmBase vm,
Line 28: VMStatus vmStatus, Guid dcId,
Please format for uniformity, i.e. one argument per line. There's a formatter
that's part of the git repository, you can just plug it into an IDE and let it
do the job for you.
Line 29: Version clusterCompatibilityVersion,
Line 30: ArrayList<VmNetworkInterface> vmNicList,
Line 31: VmNetworkInterface nic,
Line 32: EntityModel sourceModel) {
Line 71:
getPlugged().setChangeProhibitionReason(ConstantsManager.getInstance()
Line 72: .getMessages()
Line 73:
.hotPlugNotSupported(getClusterCompatibilityVersion().toString()));
Line 74: }
Line 75: getPlugged().setIsChangable(hotPlugSupported ||
vmStatus.equals(VMStatus.Down));
You've added this exact same line in NewVmInterfaceModel. Duplication is error
prone when changes are made in the future, so why not change the name of the
variable hotPlugSupported to allowHotPlug and set it in VmInterfaceModel to be
the logical OR of the cluster compatibility version and the VM status?
Line 76:
Line 77: initCommands();
Line 78: }
Line 79:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmInterfaceModel.java
Line 18: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 19:
Line 20: public class NewVmInterfaceModel extends VmInterfaceModel {
Line 21:
Line 22: private final VMStatus vmStatus;
Same as BaseEditVmInterfaceModel.
Line 23:
Line 24: public static NewVmInterfaceModel createInstance(VmBase vm,
Line 25: VMStatus vmStatus, Guid dcId,
Line 26: Version clusterCompatibilityVersion,
Line 21:
Line 22: private final VMStatus vmStatus;
Line 23:
Line 24: public static NewVmInterfaceModel createInstance(VmBase vm,
Line 25: VMStatus vmStatus, Guid dcId,
Same formatting comment as in BaseEditVmInterfaceModel.
Line 26: Version clusterCompatibilityVersion,
Line 27: ArrayList<VmNetworkInterface> vmNicList,
Line 28: EntityModel sourceModel) {
Line 29: NewVmInterfaceModel instance = new NewVmInterfaceModel(vm,
vmStatus, dcId, clusterCompatibilityVersion, vmNicList, sourceModel);
Line 31: return instance;
Line 32: }
Line 33:
Line 34: protected NewVmInterfaceModel(VmBase vm,
Line 35: VMStatus vmStatus, Guid dcId,
Same formatting comment...
Line 36: Version clusterCompatibilityVersion,
Line 37: ArrayList<VmNetworkInterface> vmNicList,
Line 38: EntityModel sourceModel) {
Line 39: super(vm, dcId, clusterCompatibilityVersion, vmNicList,
sourceModel, new NewProfileBehavior());
Line 64:
getPlugged().setChangeProhibitionReason(ConstantsManager.getInstance()
Line 65: .getMessages()
Line 66:
.hotPlugNotSupported(getClusterCompatibilityVersion().toString()));
Line 67: }
Line 68: getPlugged().setIsChangable(hotPlugSupported ||
vmStatus.equals(VMStatus.Down));
Same as BaseEditVmInterfaceModel, should probably happen in VmInterfaceModel.
Line 69: getPlugged().setEntity(hotPlugSupported ||
vmStatus.equals(VMStatus.Down));
Line 70:
Line 71: initLinked();
Line 72:
Line 65: .getMessages()
Line 66:
.hotPlugNotSupported(getClusterCompatibilityVersion().toString()));
Line 67: }
Line 68: getPlugged().setIsChangable(hotPlugSupported ||
vmStatus.equals(VMStatus.Down));
Line 69: getPlugged().setEntity(hotPlugSupported ||
vmStatus.equals(VMStatus.Down));
Why has this changed? Shouldn't the interface be plugged by default no matter
what?
Line 70:
Line 71: initLinked();
Line 72:
Line 73: initProfiles();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmGuideModel.java
Line 146: return;
Line 147: }
Line 148:
Line 149: VmInterfaceModel model =
Line 150:
NewVmInterfaceModel.createInstance(getEntity().getStaticData(),
getEntity().getStatus(), getEntity().getStoragePoolId(),
I think this should be formatted as well.
Line 151: getEntity().getVdsGroupCompatibilityVersion(),
Line 152: nics,
Line 153: this);
Line 154: setWindow(model);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmInterfaceListModel.java
Line 138: return;
Line 139: }
Line 140:
Line 141: VmInterfaceModel model =
Line 142:
NewVmInterfaceModel.createInstance(getEntity().getStaticData(),
getEntity().getStatus(), getEntity().getStoragePoolId(),
Should probably be formatted as well.
Line 143:
getEntity().getVdsGroupCompatibilityVersion(), (ArrayList<VmNetworkInterface>)
getItems(), this);
Line 144: setWindow(model);
Line 145: }
Line 146:
--
To view, visit http://gerrit.ovirt.org/19189
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434386f98c31a8d0fe44011ecadecb732ded19a
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gustavo Frederico Temple Pedrosa <gustavo.pedr...@eldorado.org.br>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
<gustavo.pedr...@eldorado.org.br>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Vitor de Lima <vitor.l...@eldorado.org.br>
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
