Yair Zaslavsky has posted comments on this change. Change subject: aaa: Changing ExternalId to String ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/IdType.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/IdType.java: Line 1: package org.ovirt.engine.extensions.aaa.builtin.kerberosldap; Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; > are you sure you need this function? why not perform conversion on demand? Yes. The conversion is type based, and not "parameter/parameter name" based. I have to distinguish between strings like user name and a string of user id. Line 4: Line 5: /** Line 6: * This is used as a wrapper to external ids, to distinguish them from other strings when passed as parameters to ldap Line 7: * calls (external ids need to go encoding) http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbGroupDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbGroupDAODbFacadeImpl.java: Line 53: Line 54: // In older versions of the engine the internal and external identifiers were the same, so we also need to check Line 55: // if the internal id is really an external id: Line 56: if (domain != null && id != null) { Line 57: externalId = guidToExternalId(id); > this should not be after conversion. right, i'll fix that. Line 58: DbGroup existing = getByExternalId(domain, externalId); Line 59: if (existing != null) { Line 60: return existing; Line 61: } http://gerrit.ovirt.org/#/c/26134/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DbUserDAODbFacadeImpl.java: Line 112: Line 113: // In older versions of the engine the internal and external identifiers were the same, so we also need to check Line 114: // if the internal id is really an external id: Line 115: if (domain != null && id != null) { Line 116: externalId = guidToExternalId(id); > this should not be after conversion. right. i'll fix that. Line 117: DbUser existing = getByExternalId(domain, externalId); Line 118: if (existing != null) { Line 119: return existing; Line 120: } http://gerrit.ovirt.org/#/c/26134/1/packaging/dbscripts/upgrade/03_05_0180_change_external_id_to_string.sql File packaging/dbscripts/upgrade/03_05_0180_change_external_id_to_string.sql: Line 13: UPDATE users SET Line 14: external_id = substring(external_id from 3 for 8) || '-' || Line 15: substring(external_id from 11 for 4) || '-' || Line 16: substring(external_id from 15 for 4) || '-' || Line 17: substring(external_id from 19 for 4) || '-' || > trailing spaces? i'll fix trailing whitespaces, not sure i understood the 2nd comment. re-entrancy is not required here. at this point in time of the development you have only legacy providers. in the future, when user installs or upgrades ovirt, it will go over this script, and convert external_id to string. the script should be run once, and actually, re-entrancy of upgrade scripts is not mandatory for quite some time now. Line 18: substring(external_id from 23); -- To view, visit http://gerrit.ovirt.org/26134 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I086e6be2e47518238f2011702ee9ef17071fc2b6 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
