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.
--
Endi S. Dewata
_______________________________________________
Pki-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/pki-devel
