Juan Hernandez has posted comments on this change. Change subject: restapi: Show full Gluster storage domain details ......................................................................
Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/39810/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainsResource.java: Line 357: case FCP: Line 358: mapVolumeGroupFcp(model, entity); Line 359: break; Line 360: case NFS: Line 361: mapNFSProperties(model, entity); It is very confusing to have a "case" without a "break". I can imagine myself or someone else scratching his head and wondering what is the reason for this, and maybe just adding the apparently missing "break". So I'd suggest to repeat the call to "mapFileDomain" and add the "break". Line 362: case LOCALFS: Line 363: case POSIXFS: Line 364: case GLUSTERFS: Line 365: mapFileDomain(model, entity); Line 380: } Line 381: if (cnx.getNfsVersion() != null) { Line 382: model.getStorage().setNfsVersion(StorageDomainMapper.map(cnx.getNfsVersion(), null)); Line 383: } Line 384: } Do we really need this new method? Can't we have just one as before? Note that this also means that we are calling "getStorageServerConnection" twice. Line 385: Line 386: protected void mapFileDomain(StorageDomain model, Line 387: org.ovirt.engine.core.common.businessentities.StorageDomain entity) { Line 388: final Storage storage = model.getStorage(); https://gerrit.ovirt.org/#/c/39810/2/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 290: model.setMountOptions(entity.getMountOptions()); Line 291: } Line 292: } Line 293: else if (entity.getstorage_type().equals(org.ovirt.engine.core.common.businessentities.storage.StorageType.POSIXFS) Line 294: || entity.getstorage_type().equals(StorageType.GLUSTERFS)) { This is comparing for equality values of two different enum types, which should never be equal. Did you intend to use org.ovirt.engine.core.common.businessentities.storage.StorageType.GLUSTERFS? Line 295: model.setMountOptions(entity.getMountOptions()); Line 296: model.setVfsType(entity.getVfsType()); Line 297: } Line 298: return model; -- To view, visit https://gerrit.ovirt.org/39810 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2294ab6a0256f5c8bc5f48e04dada506500f79d5 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@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
