Juan Hernandez has posted comments on this change.
Change subject: restapi: Allow enabling/disabling SSO
......................................................................
Patch Set 2:
(11 comments)
I think that the .xsd needs to be changed to avoid potential breakages in the
Python and Java SDKs. The rest are minor comments.
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 2301:
Line 2302: <!-- Single Sign-On Section -->
Line 2303: <xs:complexType name="Sso">
Line 2304: <xs:sequence>
Line 2305: <xs:element name="methods" type="Methods" minOccurs="1"
maxOccurs="1" />
The "methods" and "method" element names are ok, but I think that the name of
the types should be "SsoMethods" and "SsoMethod", to clearly distinguish them
from other potential meanings of "methods".
Line 2306: </xs:sequence>
Line 2307: </xs:complexType>
Line 2308:
Line 2309: <xs:complexType name="Methods">
Line 2311: <xs:extension base="BaseResources">
Line 2312: <xs:sequence>
Line 2313: <!-- In future we may allow more method elements,
Line 2314: but now only 1 occurence of this element is
allowed. -->
Line 2315: <xs:element name="method" type="Method"
minOccurs="0" maxOccurs="1" />
Your comment makes sense, but setting the maxOccurs to 1 affects not only the
model entities used internally by the API, but also the entities generated for
the Java SDK. If we do this then the generated method will return String now
and will change to a list of strings in the future. That is in incompatible
change. Better set it unbounded.
Also I think that this shouldn't extends BaseResources, as it isn't actually a
collection of resources.
Line 2316: </xs:sequence>
Line 2317: </xs:extension>
Line 2318: </xs:complexContent>
Line 2319: </xs:complexType>
Line 2319: </xs:complexType>
Line 2320:
Line 2321: <xs:simpleType name="Method">
Line 2322: <xs:restriction base="xs:string">
Line 2323: <xs:enumeration value="NONE"/>
I think that NONE should not exist, at that is already represented by an empty
list of methods.
Line 2324: <xs:enumeration value="GUEST_AGENT"/>
Line 2325: </xs:restriction>
Line 2326: </xs:simpleType>
Line 2327:
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/TemplateMapper.java
Line 63: if (model.isSetDeleteProtected()) {
Line 64: entity.setDeleteProtected(model.isDeleteProtected());
Line 65: }
Line 66: if (model.isSetSso()) {
Line 67:
entity.setSsoMethod(SsoUtils.fromRestToBackend(model.getSso()));
If you use the mapper framework you can do here:
entity.setSsoMethod(SsoMapper.map(model.getSso(), null));
Line 68: }
Line 69: if (model.isSetType()) {
Line 70: VmType vmType = VmType.fromValue(model.getType());
Line 71: if (vmType != null) {
Line 188: if (model.isSetDeleteProtected()) {
Line 189: staticVm.setDeleteProtected(model.isDeleteProtected());
Line 190: }
Line 191: if (model.isSetSso()) {
Line 192:
staticVm.setSsoMethod(SsoUtils.fromRestToBackend(model.getSso()));
Same here, use the mapper framework if possible.
Line 193: }
Line 194: if (model.isSetType()) {
Line 195: VmType vmType = VmType.fromValue(model.getType());
Line 196: if (vmType != null) {
Line 282:
model.getHighAvailability().setEnabled(entity.isAutoStartup());
Line 283: model.getHighAvailability().setPriority(entity.getPriority());
Line 284: model.setStateless(entity.isStateless());
Line 285: model.setDeleteProtected(entity.isDeleteProtected());
Line 286:
model.setSso(SsoUtils.fromBackendToRest(entity.getSsoMethod()));
And same here.
Line 287: if (entity.getVmType() != null) {
Line 288: model.setType(VmMapper.map(entity.getVmType(), null));
Line 289: }
Line 290: if (entity.getOrigin() != null) {
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java
Line 196: if (vm.isSetDeleteProtected()) {
Line 197: staticVm.setDeleteProtected(vm.isDeleteProtected());
Line 198: }
Line 199: if (vm.isSetSso()) {
Line 200:
staticVm.setSsoMethod(SsoUtils.fromRestToBackend(vm.getSso()));
Try to use the mapper framework.
Line 201: }
Line 202: if (vm.isSetHighAvailability()) {
Line 203: HighAvailability ha = vm.getHighAvailability();
Line 204: if (ha.isSetEnabled()) {
Line 437: model.setType(map(entity.getVmType(), null));
Line 438: model.setStateless(entity.isStateless());
Line 439: model.setDeleteProtected(entity.isDeleteProtected());
Line 440: if (entity.getSsoMethod() != null) {
Line 441:
model.setSso(SsoUtils.fromBackendToRest(entity.getSsoMethod()));
Same.
Line 442: }
Line 443: model.setHighAvailability(new HighAvailability());
Line 444:
model.getHighAvailability().setEnabled(entity.isAutoStartup());
Line 445: model.getHighAvailability().setPriority(entity.getPriority());
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/SsoUtils.java
Line 3: import org.ovirt.engine.api.model.Method;
Line 4: import org.ovirt.engine.api.model.Methods;
Line 5: import org.ovirt.engine.api.model.Sso;
Line 6:
Line 7: public class SsoUtils {
Would you mind checking if you can use the "mappers" framework for this?
Something like:
public class SsoMapper {
@Mapping(from = SsoMethods.class, to = o.o.e.c.b.SsoMethod backendValue)
public static Sso map(o.o.e.c.b.SsoMethod backendValue entity, Sso
template) {
...
}
}
Same for the reverse conversion.
Line 8:
Line 9: public static org.ovirt.engine.api.model.Sso
fromBackendToRest(org.ovirt.engine.core.common.businessentities.SsoMethod
backendValue) {
Line 10: if (backendValue == null) {
Line 11: return null;
Line 19: case GUEST_AGENT:
Line 20: methods.setMethod(Method.GUEST_AGENT);
Line 21: break;
Line 22: case NONE:
Line 23: methods.setMethod(Method.NONE);
NONE should be the result when the input list of methods is empty.
Line 24: break;
Line 25: }
Line 26:
Line 27: return sso;
Line 32: switch (restValue.getMethods().getMethod()) {
Line 33: case GUEST_AGENT:
Line 34: return
org.ovirt.engine.core.common.businessentities.SsoMethod.GUEST_AGENT;
Line 35: case NONE:
Line 36: return
org.ovirt.engine.core.common.businessentities.SsoMethod.NONE;
NONE should be translated to an empty list of methods.
Line 37: }
Line 38: }
Line 39: return null;
Line 40: }
--
To view, visit http://gerrit.ovirt.org/21911
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I25bf94646ea89684152e48f25ad604db6e59e86c
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Ori Liel <ol...@redhat.com>
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
