Juan Hernandez has posted comments on this change. Change subject: restapi: Allow enabling/disabling SSO ......................................................................
Patch Set 7: (3 comments) http://gerrit.ovirt.org/#/c/21911/7/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SsoMethod.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/model/SsoMethod.java: Line 2: Line 3: public enum SsoMethod { Line 4: Line 5: GUEST_AGENT, Line 6: NONE; I think that NONE shouldn't be part of this enum, because that is already represented by the empty list of methods. I mean, if the API receives something like this when creating or updating a VM: <sso> <methods/> </sso> The backend should then assume that all the SSO methods should be disabled. On the other hand, If it receives an empty "sso" element, or no "sso" element, or an "sso" element without a nested "methods" element, then it should use the default (when creating) or don't change anything (when updating). For example, this: <sso> <some_other_sso_properties> <!-- But no "methods" element given. --> </sso> Should result in using the backend using the default SSO method when creating a new VM, and in no changes to the SSO method when updating a VM. Line 7: Line 8: public String value() { Line 9: return name().toLowerCase(); Line 10: } http://gerrit.ovirt.org/#/c/21911/7/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/SsoMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/SsoMapper.java: Line 16: Line 17: if (entity == SsoMethod.GUEST_AGENT) { Line 18: model.getMethods().getMethods().add(org.ovirt.engine.api.model.SsoMethod.GUEST_AGENT.toString()); Line 19: } else if (entity == SsoMethod.NONE) { Line 20: model.getMethods().getMethods().add(org.ovirt.engine.api.model.SsoMethod.NONE.toString()); I think that the mapping of the backend NONE should result in an empty list of methods, something like this: <sso> <methods/> </sso> That explicitly indicates that no SSO method is enabled. Line 21: } Line 22: Line 23: return model; Line 24: } http://gerrit.ovirt.org/#/c/21911/7/backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/VmMapperTest.java File backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/VmMapperTest.java: Line 133: assertEquals(model.getTimezone(), transform.getTimezone()); Line 134: assertEquals(model.getDisplay().isSmartcardEnabled(), transform.getDisplay().isSmartcardEnabled()); Line 135: assertEquals(model.getDisplay().getKeyboardLayout(), transform.getDisplay().getKeyboardLayout()); Line 136: assertEquals(model.isDeleteProtected(), transform.isDeleteProtected()); Line 137: if (!model.getSso().getMethods().getMethods().isEmpty()) { // empty <methods> should result in GUEST_AGENT The mapper, and thus the test, should know anything about GUEST_AGENT, that is backend logic. Ideally here we should check that the transform is identical to the original. For example, if the original had a "sso" element the transform should also have it, and with the same content. And the other way around: if the original doesn't have a "sso" element then the transform shouldn't have it either. Doing all that here would be cumbersome, so it is probably better to create a new SsoMapper test class, and put the tests there, then you can do here simpler assertions: assertEquals(mode.isSetSso(), transform.isSetSso()); The bulk of the testing will be in the SsoMapperTest class. Line 138: assertEquals(model.getSso().getMethods().getMethods(), transform.getSso().getMethods().getMethods()); Line 139: } Line 140: assertEquals(model.isTunnelMigration(), transform.isTunnelMigration()); Line 141: } -- 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: 7 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 <mishka8...@yahoo.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
