Alissa Bonas has posted comments on this change. Change subject: core: add check for iqn in addStorageServerConn ......................................................................
Patch Set 5: Liron - design wise I'm not in favor of helper classes, especially for validation such as this. Usually extracting code to helper classes implicates that some things were not solved properly in the original object that uses the helper. (object oriented wise) Per the the validation issue - the problem exists not just in iscsi, but in other storage types as well. StorageServerConnections object is currently a big container having all fields of all storage types together. I think there are other better ways to model those connections per storage type in a more clear way. The correct solution IMO is to have validation of required fields on the connection object (or in validator) based on each storage type (for example iqn is mandatory for iscsi but irrelevant for NFS, vfs_type is mandatory for Posix but irrelevant for other types, etc.) Making a helper for iscsi solves a very regional small problem and I prefer seeing this issue solved right for all storage types with a bigger refactoring, because such validation is good to have for all storage types. However - definitely not in the scope of this patch. -- To view, visit http://gerrit.ovirt.org/15516 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide54ab2ec4a864dadd01cbe746df6592e32eab2f Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alissa Bonas <[email protected]> Gerrit-Reviewer: Alissa Bonas <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
