Allon Mureinik has posted comments on this change. Change subject: engine: Add custom mount options to NFS SD ......................................................................
Patch Set 1: (5 comments) Xavi: 1. Generally looks good to me, but see comments inline. 2. Just to make sure - preexisting NFS connections will have a NULL value in the database. Did you verify that nothing will break with an NPE? Juan: Can you please take a look at the REST parts (and anything else you're interested in, of course)? Daniel: Can you please take a look at the frontend parts (and anything else you're interested in, of course)? 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 86: } Line 87: } Line 88: if (storage.isSetMountOptions()) { Line 89: entity.setMountOptions(storage.getMountOptions()); Line 90: } I wonder if it's worth working the code around so this block: if (storage.isSetMountOptions()) { entity.setMountOptions(storage.getMountOptions()); } which relates to NFS, POSIX and Gluster appears only once. Line 91: break; Line 92: case LOCALFS: Line 93: if (storage.isSetPath()) { Line 94: entity.setconnection(storage.getPath()); Line 228: } Line 229: } Line 230: if (model.isSetMountOptions()) { Line 231: entity.setMountOptions(model.getMountOptions()); Line 232: } I wonder if it's worth working the code around so this block: if (storage.isSetMountOptions()) { entity.setMountOptions(storage.getMountOptions()); } which relates to NFS, POSIX and Gluster appears only once. Line 233: break; Line 234: case LOCALFS: Line 235: if (model.isSetPath()) { Line 236: entity.setconnection(model.getPath()); Line 283: model.setNfsTimeo(entity.getNfsTimeo().intValue()); Line 284: } Line 285: if (entity.getMountOptions() != null) { Line 286: model.setMountOptions(entity.getMountOptions()); Line 287: } I wonder if it's worth working the code around so this block: if (storage.isSetMountOptions()) { entity.setMountOptions(storage.getMountOptions()); } which relates to NFS, POSIX and Gluster appears only once. Line 288: } Line 289: else if (entity.getstorage_type().equals(org.ovirt.engine.core.common.businessentities.StorageType.POSIXFS)) { Line 290: model.setMountOptions(entity.getMountOptions()); Line 291: model.setVfsType(entity.getVfsType()); http://gerrit.ovirt.org/#/c/27694/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java: Line 1530: private void updatePosixProperties(IStorageModel storageModel) { Line 1531: PosixStorageModel posixModel = (PosixStorageModel) storageModel; Line 1532: connection.setVfsType(posixModel.getVfsType().getEntity().toString()); Line 1533: if (posixModel.getMountOptions().getEntity() != null) { Line 1534: connection.setMountOptions(posixModel.getMountOptions().getEntity()); This change, while it may be true, is unrealted to NFS Mount Options. Please publish it in a separate fix. Line 1535: } Line 1536: Line 1537: } Line 1538: 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.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/NfsStorageView.java: Line 200: Line 201: // When all advanced fields are unavailable - hide the expander. Line 202: boolean anyField = object.getVersion().getIsAvailable() Line 203: || object.getRetransmissions().getIsAvailable() Line 204: || object.getTimeout().getIsAvailable() || object.getMountOptions().getIsAvailable(); Please move this new condition down a line, to keep the code style for this block. Line 205: Line 206: expander.getElement().getStyle().setVisibility(anyField ? Style.Visibility.VISIBLE : Style.Visibility.HIDDEN); Line 207: } Line 208: -- 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: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
