Looks good to me. There's a trailing whitespace in the patch. Please fix before push. -- Endi S. Dewata ----- Original Message ----- > Please do a quick review of the attached patch with fixes made as per > our discussion on IRC. > Though Ade has ack'ed, would like to have the patch go through a > final > review as you have pointed out a mistake in the file path. > > The comments were: > 1. The file path specified where the user configuration is stored is > wrong. * It is corrected from /var/lib/pki/<instance > name>/<subsystem>/deployment.cfg to > /etc/sysconfig/pki/tomcat/<instance > name>/<subsystem>/deployment.cfg. > > 2. Removed the line : "-- When the -f <configuration filename> is not > specified:". > > 3. Corected the extraspaces in the line HTTP port under Tomcat > instance > section > > 4. Capitalize the HTTP, AJP, HTTPS in the description of those > fields. > > 5. Proper formatting of commit message - Done. > > --Abhishek > > On Wed, 2013-03-20 at 09:07 -0400, Abhishek Koneru wrote: > > Please find the fixes for review comments given for patch 37 > > attached > > for review. > > > > --Abhishek > > > > On Tue, 2013-03-19 at 10:15 -0400, Ade Lee wrote: > > > Comments: > > > > > > 1. The commit message looks like it goes on continuously on a > > > single > > > line. This makes it difficult to see when doing a "git log". > > > > > > 2. The patch ends up adding several trailing whitespace errors. > > > Fix > > > these before checking in. > > > > > > 3. In pkidestroy man page, add the note about prompting for the > > > parameter under each parameter, rather than as a separate note. > > > > > > For example, > > > -s <subsystem> > > > Specifies the subsystem to be removed, where > > > <subsystem> is CA, KRA, > > > OCSP, or TKS. If this option is not specified, > > > pkidestroy will prompt > > > for its value. > > > > > > 4. Include documentation of the new password/ user options for > > > pkidestroy. You can do this in the other ticket you are working > > > on. > > > > > > 5. pkispawn: > > > > > > a) Move the section on interactive mode to before examples. It > > > should > > > be a heading at the same level as OPTIONS and EXAMPLES > > > b) Replace Dogtag with Certificate Server (all instances) > > > c) The subheadings do not look right. Change as follows. Note > > > that I > > > have removed the enclosing brackets. These headings should be in > > > bold. > > > > > > [Choosing a subsystem type to install] --> Subsystem Type > > > [Providing instance specific parameters] --> Instance Specific > > > Parameters > > > [Setting up the Administrator user] --> Administrative User > > > Parameters > > > [Specify the 389 Directory Server to be used for the internal > > > database]-> Directory Server Parameters > > > [Setting up the Security Domain properties]-> Security Domain > > > Parameters > > > > > > d) The security domain part should look like this: > > > > > > Security Domain Parameters > > > > > > Name: > > > the name of the security domain. Required only if installing > > > a root CA. > > > Default value: <DNS domain name> Security Domain. > > > > > > Hostname: > > > the hostname for the security domain CA. Required for all > > > non-CA subsystems. > > > The default value is the hostname of this system. > > > Secure HTTP port: > > > the https port for the security domain. Required for all > > > non-CA subsystems. > > > The default value is 8443. > > > Username: > > > the username of the security domain administrator. Required > > > for all non-CA subsystems. > > > The default value is caadmin. > > > Password: > > > password for the security domain administrator. Required > > > for all non-CA subsystems. > > > > > > e) Do not bold or italicize any default values. [8080] -> 8080 > > > > > > > > > On Fri, 2013-03-15 at 11:00 -0400, Abhishek Koneru wrote: > > > > Please review the attached patch w.r.t the trac ticket 471 to > > > > add > > > > information regarding interactive mode installation to > > > > pkispawn/pkidestroy man pages. > > > > > > > > --Abhishek > > > > _______________________________________________ > > > > Pki-devel mailing list > > > > Pki-devel(a)redhat.com > > > > https://www.redhat.com/mailman/listinfo/pki-devel > > > > > > > > > > _______________________________________________ > > Pki-devel mailing list > > Pki-devel(a)redhat.com > > https://www.redhat.com/mailman/listinfo/pki-devel > > >