Tal Nisan has posted comments on this change. Change subject: core: Block import of storage domain if the storage contains hosted engine ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/36549/4//COMMIT_MSG Commit Message: Line 6: Line 7: core: Block import of storage domain if the storage contains hosted engine Line 8: Line 9: Block an import of a storage domain that contains the hosted engine by the Line 10: it's storage name defined in the configuration value named > I didn't understood the relation "by the it's ": Done Line 11: HOSTED_ENGINE_STORAGE_DOMAIN_NAME Line 12: Line 13: Change-Id: I36db8c4a7a1d13be8874407c56596ecd9ab74333 Line 14: Bug-Url: https://bugzilla.redhat.com/1177247 http://gerrit.ovirt.org/#/c/36549/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingBlockStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingBlockStorageDomainCommand.java: Line 67: Line 68: StorageDomainValidator validator = new StorageDomainValidator(getStorageDomain()); Line 69: if (!validate(validator.isHostedEngineStorage())) { Line 70: return false; Line 71: } > I would move this before you call getLUNsFromVgInfo (to reduce the vds oper Allon thought otherwise, we should check existence first :) Line 72: Line 73: return true; Line 74: } Line 75: http://gerrit.ovirt.org/#/c/36549/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingFileStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddExistingFileStorageDomainCommand.java: Line 68: storageDomainFromIrs.setStorageStaticData(domainFromIrs.getFirst()); Line 69: StorageDomainValidator validator = new StorageDomainValidator(storageDomainFromIrs); Line 70: if (!validate(validator.isHostedEngineStorage())) { Line 71: return false; Line 72: } > on second thought, why not using only getStorageDomain() for the validator, Yes, but when and if we go through REST the storage name can be defined in the request, safest way is to compare it to the storage name that exists on the storage itself Line 73: Line 74: return concreteCheckExistingStorageDomain(domainFromIrs); Line 75: } Line 76: http://gerrit.ovirt.org/#/c/36549/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddExistingBlockStorageDomainCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/AddExistingBlockStorageDomainCommandTest.java: Line 86: doReturn(new ArrayList<LUNs>()).when(command).getAllLuns(); Line 87: Line 88: parameters.getStorageDomain().setStorageName(StorageConstants.HOSTED_ENGINE_STORAGE_DOMAIN_NAME); Line 89: assertFalse(command.canAddDomain()); Line 90: CanDoActionTestUtils.assertCanDoActionMessages("", command, VdcBllMessages.ACTION_TYPE_FAILED_HOSTED_ENGINE_STORAGE); > Please add a message instead of empty string Done Line 91: } Line 92: Line 93: private static StorageDomainStatic getStorageDomain() { Line 94: StorageDomainStatic storageDomain = new StorageDomainStatic(); -- To view, visit http://gerrit.ovirt.org/36549 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I36db8c4a7a1d13be8874407c56596ecd9ab74333 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
