Sergey Gotliv has posted comments on this change.
Change subject: core,webadmin: Plug disk to VM when adding a new disk
......................................................................
Patch Set 2:
(7 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 97:
!isDiskPassPciAndIdeLimit(getParameters().getDiskInfo())) {
Line 98: return false;
Line 99: }
Line 100: }
Line 101: else {
} else ....
Line 102: if (getParameters().getPlugDiskToVm()) {
Line 103: return
failCanDoAction(VdcBllMessages.CANNOT_ADD_FLOATING_DISK_WITH_PLUG_VM_SET);
Line 104: }
Line 105: }
Line 98: return false;
Line 99: }
Line 100: }
Line 101: else {
Line 102: if (getParameters().getPlugDiskToVm()) {
Why this parameter is Boolean and not boolean? Null its like true in our case,
so let's initialize it with true always.
Line 103: return
failCanDoAction(VdcBllMessages.CANNOT_ADD_FLOATING_DISK_WITH_PLUG_VM_SET);
Line 104: }
Line 105: }
Line 106:
Line 367: StorageDomainCommandBase.proceedLUNInDb(lun,
lun.getLunType());
Line 368: getBaseDiskDao().save(getParameters().getDiskInfo());
Line 369: getDiskLunMapDao().save(new
DiskLunMap(getParameters().getDiskInfo().getId(), lun.getLUN_id()));
Line 370: if (getVm() != null) {
Line 371: boolean shouldPlugDisk = getVm().getStatus() ==
VMStatus.Down && (getParameters().getPlugDiskToVm() == null ||
getParameters().getPlugDiskToVm());
Its confusing me either, please put the comment in the code. Otherwise someone
will remove it one day.
Line 372: VmDeviceUtils.addManagedDevice(new
VmDeviceId(getParameters().getDiskInfo().getId(), getVmId()),
Line 373: VmDeviceGeneralType.DISK,
Line 374: VmDeviceType.DISK,
Line 375: null,
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java
Line 733: CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command);
Line 734: }
Line 735:
Line 736: @Test
Line 737: public void testCanDoFailOnAddDiskFloatingDiskWithPlugSet() {
The test name contains "Disk" twice :-). Can you remove the first appearance,
please.
Line 738: DiskImage disk = createDiskImage(1);
Line 739:
Line 740: AddDiskParameters parameters = createParameters();
Line 741: parameters.setDiskInfo(disk);
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.java
Line 315: isBootableEditor = new EntityModelCheckBoxEditor(Align.RIGHT);
Line 316: isShareableEditor = new
EntityModelCheckBoxEditor(Align.RIGHT);
Line 317: isReadOnlyEditor = new EntityModelCheckBoxEditor(Align.RIGHT);
Line 318: isSgIoUnfilteredEditor = new
EntityModelCheckBoxEditor(Align.RIGHT);
Line 319: isPluggedEditor = new EntityModelCheckBoxEditor(Align.RIGHT);
Agree with Liron.
In case you have to use it, this popup is used by edit either, right?
if so, please, verify that this new editor is hidden there.
Line 320: attachEditor = new EntityModelCheckBoxEditor(Align.RIGHT);
Line 321:
Line 322: internalDiskTable = new EntityModelCellTable<ListModel>(true);
Line 323: externalDiskTable = new EntityModelCellTable<ListModel>(true);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AbstractDiskModel.java
Line 66: private EntityModel isDirectLunDiskAvaialable;
Line 67: private EntityModel isSgIoUnfiltered;
Line 68: private EntityModel sizeExtend;
Line 69: private EntityModel plugDiskToVm;
Line 70:
Please, move it to NewDiskModel, otherwise EditDiskModel also inherits it.
Line 71: private ListModel storageType;
Line 72: private ListModel host;
Line 73: private ListModel dataCenter;
Line 74: private ListModel internalAttachableDisks;
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewDiskModel.java
Line 46: if (getVm() != null) {
Line 47: updateSuggestedDiskAlias();
Line 48: getPlugDiskToVm().setEntity(true);
Line 49: getPlugDiskToVm().setIsAvailable(true);
Line 50: } else {
I think that 'true' is the default value for availability.
Just verify that you don't display the checkbox for floating disks.
getPlugDiskToVm().setIsAvailable(false);
Line 51: // Read only disk can be created only in the scope of VM.
Line 52: getIsReadOnly().setIsAvailable(false);
Line 53: }
Line 54:
--
To view, visit http://gerrit.ovirt.org/21851
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9778bcaf21b346a55992590159cabd8d78f0c66
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Cheryn Tan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
