Alissa Bonas has posted comments on this change. Change subject: engine: Add custom mount options to NFS SD ......................................................................
Patch Set 1: (2 comments) I'd add some mount options in the REST tests. (BackendStorageConnectionsResourceTest for example) http://gerrit.ovirt.org/#/c/27694/1/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StorageDomainMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StorageDomainMapper.java: Line 84: if (nfsVersion != null) { Line 85: entity.setNfsVersion(map(nfsVersion, null)); Line 86: } Line 87: } Line 88: if (storage.isSetMountOptions()) { What is the behavior if user puts value in let's say timeo field, and then again puts timeo as part of the mount options which in my understanding is a freetext field with no validation? Taking it further - what will happen if in both places the value of timeo will be different? Which will be applied? Will the mount fail? will the user be notified with a clear error? I suggest verifying that the 3 separate options we expose do not appear in the mount options field again, or at least if they appear in both places - that their values are the same (and then there's no need to append them again) Line 89: entity.setMountOptions(storage.getMountOptions()); Line 90: } Line 91: break; Line 92: case LOCALFS: http://gerrit.ovirt.org/#/c/27694/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/NfsStorageView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/NfsStorageView.ui.xml: Line 105: </td> Line 106: </tr> Line 107: <tr> Line 108: <td nowrap="nowrap"> Line 109: <g:Label ui:field="mountOptionsLabel" addStyleNames="{style.label}"/> IMHO it's misleading to the user to see in UI (and REST) in NFS the 3 separate mount options as we display today (retrans, timeo, etc.), and then next to them them put a field named "mount options" - the other 3 are mount options as well. In POSIX the problem doesn't exist as there's only one field there named "mount options". I would at least name it "additional mount options", or "extra mount options". Line 110: </td> Line 111: <td> Line 112: <ge:StringEntityModelTextBoxOnlyEditor ui:field="mountOptionsEditor" addStyleNames="table_contentWidget_pfly_fix"/> Line 113: </td> -- To view, visit http://gerrit.ovirt.org/27694 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c7d51f5bf1ffb3491788b9fcda770a55b94cf50 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <[email protected]> Gerrit-Reviewer: Alissa Bonas <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Xavi Francisco <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
