Re: [Pki-devel] [PATCH] 315-319 KRA realm related patches

On 6/2/2016 8:51 AM, Ade Lee wrote: > And now with the patches .. > > On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote: > > Patch descriptions (in reverse order). > > > > The final patch will need some discussion. Please review, > > > > Ade Some comments: 1. In SrchKey and SrchKeyForRecovery the check probably should have been like this (no closing paren): if (filter.contains("(realm=")) { 2. If a realm is specified, I suppose it won't match any of the VLV indexes. Do the queries still work correctly without a matching VLV? 3. Could you document how to run the upgrade command in this page? http://pki.fedoraproject.org/wiki/Database_Upgrade_for_PKI_10.3.x Note that there are still some manual upgrade procedures that also need to be executed as documented in that page. 4. In PKISubsystem.open_database() can we use bind_dn and bind_password parameter names instead of user and password (it's a DN not a user ID)? Also in DBUpgrade should we use -D, -w, and -Z for consistency with DS tools? 5. The gnu_getopt() is called with "server_id=". I think it should be "server-id=". But since it actually contains the DS instance name maybe we should use "ds-instance=" instead? 6. In the DBUpgrade.modify_kra_vlv() the os.unlink(ldif_file) probably should be moved into the finally clause above it to ensure the temporary file is deleted in all cases. Alternatively the code could be written like this: with NamedTemporaryFile() as ldif_file: # do everything with the ldif_file here 7. In DBUpgrade.modify_schema() why not use subsystem.open_database() instead of ldapmodify? That will ensure it uses the same mechanism to connect to the LDAP server. 8. Also the DBUpgrade.modify_schema() is ignoring all schema update failures. I think we can only ignore the case where the new schema is already added. In other cases the upgrade should fail since the subsequent upgrades may depend on it. 9. I think DBUpgrade.create_realm_index() should only execute if there's a KRA subsystem in the instance. The code right now creates the index in the first subsystem's database which may not be a KRA. subsystem = instance.subsystems[0] 10. The DBUpgrade uses db2index.pl to rebuild the indexes which means the LDAP server must run locally. To support remote LDAP server I think we need to use reindex task. We have indextasks.ldif and vlvtasks.ldif that can be used for this. See also: http://pki.fedoraproject.org/wiki/DS_Database_Indexes 11. The DBUpgrade should not ignore reindex failures since subsequent upgrades may depend on it. 12. There's a typo in the last patch's commit description.