Re: [Pki-devel] CLI for editing profiles

On Thu, Oct 09, 2014 at 09:47:24AM -0500, Endi Sukma Dewata wrote: > On 9/19/2014 2:46 AM, Fraser Tweedale wrote: > >Third cut of the LDAP profiles feature. All comments from previous > >reviews have been addressed in this patchset. > > > >The switch from byte[] to Properties in the ProfileClient API (item > >#13 from Endi's first review) is currently in a separte patch > >(#0014) to make it clear what had to happen for this change, and to > >make it easy to change or remove if there are problems. If it's > >preferred to squash it when acked, let me know (or it can stay > >separate - I don't mind either way). > > > >Cheers, > > > >Fraser > > > Sorry for the delay. I have some more comments below. > > > Patch #4-3: > > 1. What do you think about renaming classId into certProfileClassID? Is > there a plan to use the classId for something other than profile? > The classId represents a Java class. It is not yet used elsewhere, but if we move to finer-grained LDAP profile config schema it will be used for the inputs/outputs/defaults also, therefore I will leave it as-is. > 2. Let's use real descriptions instead of generic 'CMS defined > attribute/class', for example: > * classId: Certificate profile class ID > * certProfileConfig: Certificate profile configuration > * certProfile: Certificate profile > Will be adjusted in next patch revision. > No major issue, but see comments about database upgrade below. ACK. > > > Patch #5-3: > > 1. The commit() parameter "createBackup" doesn't match the @param backup. > Fixed the javadoc. > Nothing major. ACK. > > > Patch #6-3: > > 1. The LDAP-based profile subsystem is fine, but it might not run on > existing installation that still uses file-based profile. Are we going to > (a) support both types and provide a migration tool that can be run at a > later time, or (b) require the people to migrate to LDAP before they can use > the new code? > > Option (a) is more flexible, but we would need to keep both the old > ProfileSubsystem and the new LDAPProfileSubsystem, and then switch the class > when we migrate to LDAP. With (b) once the RPM is upgraded, the database has > to be upgraded and the profiles have to be migrated before we can restart > the server, otherwise the profile subsystem will not work. See also comments > about profile migration tool below. > Need to have a deeper discussion of the implications. Will create a new thread for this topic, or maybe we can discuss in next ipa-cs team meeting. > 2. If the default profile subsystem is changed to LDAP, we shouldn't need to > copy the profile files anymore to the instance. Look for > "pki_subsystem_profiles_path" in subsystem_layout.py. > > 3. Are the profile.* properties in CS.cfg still used by any code (other than > importProfiles()) if we use LDAP-based profile? If not, we probably should > remove them from CS.cfg and move them into the corresponding profile files > (just like the raw format), and use file list instead of profile.list during > import. > If we go with option b) above, this will clean things up a bit. If we go with option a) we either keep it or make more changes to move the classid into the file-based representation. > I'd suggest we use option (a), change the default to LDAP, and remove > profile.* properties after we have the database upgrade and profile > migration tool ready. > > > Patch #7-3: > > 1. By default the subsystems are enabled, so in setSubsystemEnabled() if > enabled=true we can actually remove the PROP_ENABLED property to keep it > consistent with other subsystems that are enabled by default. > > Nothing major. ACK. > > > Patch #8-3: > 1. There's an unused variable: > > String className = info.getClassName(); > Removed it. > Nothing major. ACK. > > > Patch #9-3: > > 1. The ca-profile-add --raw works, but the regular ca-profile-add throws an > error. If this is an existing problem please open a ticket. > This is a regression. Will have it fixed in next patch revision. > 2. The ca-profile-show --raw prepends a message to the raw output, for > example: > > --------------------- > Profile "caAdminCert" > --------------------- > ... > > I think the message should not be included in the raw output so it can be > redirected into a file, for example: > > pki ca-profile-show caAdminCert --raw > caAdminCert.cfg > Was included it for consistency with non-raw output. Happy to see it go. > 3. On the server side we should use CMS.debug(e) to log exceptions instead > of e.printStackTrace() so that the amount of logging can be controlled. > OK > 4. On the CLI side it would be better to throw an exception on execution > error (not option error), or just don't catch the original exception, rather > than printing an error message. The exception will be caught and displayed > properly by the MainCLI, and we can see the full stack trace in verbose > mode. > > 5. The code that reads the raw profile data in ProfileAddCLI and > ProfileModifyCLI probably can be refactored into > ProfileData.loadRawProfile(). The ProfileCLI.readProfileFromFile() probably > can also be moved into ProfileData.loadXMLProfile() for consistency. > OK > 6. The ca-profile-edit can also support both formats: XML and raw. > > I think #1 and #2 should be fixed. The others can be addressed later. > > > Patch #14-3: > > 1. The ProfileResource interface still uses byte[]. I originally thought it > will use Properties as well. Is that possible? > > 2. Related to #1, the response content-type is application/xml, but we > should be returning text/plain or application/octet-stream. I need to take a > look at the REST framework to see how we can support it. > > These issues require further investigation. The code itself is fine. ACK. > I have found RESTeasy to be anything but. Thanks for ACK; happy to revisit later with someone who groks RESTeasy more than I. > > Other comments: > > 1. For database upgrade, we need to add the profile schema and the profile > base entry into the database. We don't have a database upgrade framework > yet, but I suppose we can prepare a script that will connect to the database > and make the modifications. Later we can convert that script into an proper > database upgrade scriptlet. > > 2. For profile migration, we need to read profile files in the existing > instance, add it to LDAP, and remove the profile files and the profile > properties in CS.cfg. If it's a one-way migration the migration tool can be > implemented as database upgrade scriptlet too. If it's two-way, it would > have to be implemented as a stand-alone tool. > > 3. Since these patches are targeted for 10.3, check with Matt/Ade to make > sure the repo is branched properly before you push. > > 4. The patches can be pushed separately or squashed, it doesn't really > matter. Just make sure that each patch pushed contains a complete set of > changes so the build doesn't break. This would be important if we need to > cherry-pick or revert some patches later. > > -- > Endi S. Dewata _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel