Ori Liel has posted comments on this change.
Change subject: restapi: Mappers for Gluster entities
......................................................................
Patch Set 8: (8 inline comments)
....................................................
File
backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/StringHelper.java
Line 247: }
I'm totally with you about putting a utility method where everyone can use it
:)
However, the project 'compat' is a legacy project, it's deprecated and should
be deleted one of these days.
Can you please move the method to
ovirt-engine/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/StringUtils.java?
It's a nice place, I talked to the other methods and they will treat it nicely
....................................................
File
backend/manager/modules/compat/src/test/java/org/ovirt/engine/core/compat/StringHelperTest.java
Line 35: }
See comment in StringHelper, move test accordingly
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterVolumeMapper.java
Line 51:
If we stick to the decision that an Option is a business entity, then this
should be removed.
This mapper is only responsible for mapping the Volume 'level'. When GETting a
volume using REST (.../volumes/{volume:id}) the user won't see the options,
only a link to the options of this volume. If you'll look at our other mappers,
each mapper is always responsible only for it's 'level'.
Line 58: volume.addTransportType(TransportType.TCP);
I believe that default values should be handled in the Backend, not at the API
layer.
If it's legal for the user not to choose a transport type (and in that case the
transport type should be TCP) then enable REST-API not pass any value, and make
this decision in the Backend. The reasoning behind this is that if one day the
default value changes, we don't want to change it in all clients, only in the
Backend.
Also, in terms of consistency, I don't remember any of our mappers setting
default values.
Line 68:
Same comment - I would have these default values in the Backend.
Line 79:
Same comment as before: since Bricks are a collection of business entities
within the Volume, the VolumeMapper shouldn't map them.
Line 135:
same comment, no mapping of bricks in this context
Line 146: }
same comment, no mapping of options in this context
--
To view, visit http://gerrit.ovirt.org/3385
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I364d853106eda1a956241eb7210ce85781ee88b9
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Eoghan Glynn <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
