Sergey Gotliv has posted comments on this change.
Change subject: engine,webadmin: Limit usage of Local ISO SD to Local DC
......................................................................
Patch Set 2:
(4 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddLocalStorageDomainCommand.java
Line 47: }
Line 48:
Line 49: if (retVal &&
Line 50: (getStorageDomain().getStorageDomainType() ==
StorageDomainType.Data ||
Line 51: getStorageDomain().getStorageDomainType()
== StorageDomainType.ISO) &&
I agree with the comment and will change that condition.
But adding Local Export Domain limited to Local DC, will be definitely game
changer! :-)
Line 52: storagePool.getStorageType() !=
StorageType.LOCALFS) {
Line 53:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_IS_NOT_LOCAL);
Line 54: retVal = false;
Line 55: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/ImportStorageModelBehavior.java
Line 72: {
Line 73: Model model = (Model) item;
Line 74: StoragePool dataCenter = (StoragePool)
getModel().getDataCenter().getSelectedItem();
Line 75:
Line 76: if (!isStorageDomainSelectableForDataCenter(dataCenter, item))
{
see my answer there either.
Line 77: model.setIsSelectable(false);
Line 78: } else {
Line 79: // available type/function items are:
Line 80: // all in case of Unassigned DC.
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/NewEditStorageModelBehavior.java
Line 79: boolean isData = item.getRole() == StorageDomainType.Data;
Line 80: boolean isExportOrIso = item.getRole() ==
StorageDomainType.ImportExport || item.getRole() == StorageDomainType.ISO;
Line 81:
Line 82: boolean canAttachData = isData && item.getType() ==
dataCenter.getStorageType();
Line 83: boolean canAttachExportOrIso = isExportOrIso &&
isNoExportOrIsoStorageAttached &&
I read my code again, and I definitely have a bug here. It should be
!isStorageDomainSelectableForDataCenter(dataCenter, item), maybe NotSelectable
will be better name, but except of that it looks reasonable.
I meant to create 2 separate (if/else) logic areas:
1. The "if" area (negative/excluding logic) if this condition/s are satisfied
then this storage type is not selectable for this data center no matter what
2. The "else" area (positive/including logic) - if at least one of these
conditions/s are satisfied then this storage type is selectable for this DC.
Now let's examine your bullets:
1. I have no problem to create new flag "canAttachLocalSD", but what's next?
2. This statement too long(8 flags), too complicated and probably wrong or at
least its very hard to prove its correctness! How many different permutations
you have here?
We need to find good compromise...
Line 84: dataCenter.getstatus() !=
StoragePoolStatus.Uninitialized;
Line 85:
Line 86: model.setIsSelectable(isExistingStorage ||
(isNoneDataCenter && isData) ||
Line 87: (!isNoneDataCenter && (canAttachData ||
canAttachExportOrIso)));
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageModelBehavior.java
Line 107: getModel().frontend_QueryComplete();
Line 108: }
Line 109: }
Line 110:
Line 111: public boolean isStorageDomainSelectableForDataCenter(StoragePool
dataCenter, IStorageModel storageModel) {
I think the mane debate we have in NewEditStorageModelBehavior.java, see my
comment there, please.
Line 112: boolean isLocalIsoStorageDomain = (storageModel.getRole() ==
StorageDomainType.ISO &&
Line 113: storageModel.getType() == StorageType.LOCALFS);
Line 114: boolean isLocalDataCenter = (dataCenter != null &&
dataCenter.getStorageType() == StorageType.LOCALFS);
Line 115:
--
To view, visit http://gerrit.ovirt.org/18229
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cfc3bb68581664fa6729e5cb8a163324e7fbc29
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
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
