Re: [Pki-devel] [PATCH] 26 CLI - Cert Request Review and Cert Request Approve Implementation

On 7/24/2012 4:05 PM, Abhishek Koneru wrote: I have not been able to run it successfully since I'm still working on auth, but I have some comments below. I'll test this once I have a working environment. 1. The CertRequestApproveCLI.printHelp() generates a message like this: usage: cert -U <uri> -d <Certificate database directory> -w <password> -n <cert> request-approve <request id> The command name is split into "cert ... request-approve". It's supposed to show "cert-request-approve". Also it's not necessary to show the -U, -d, -w, -n options because they are part of the MainCLI's options. You can view those options by executing "pki" without any parameters. 2. Same as #1 for CertRequestReviewCLI.printHelp(). 3. In CAEnrollProfile.java there are some "Check Point <n>" debug messages. I'd say we should either replace them with messages that explain what the code is doing or just remove them. 4. In CertRequestResource.java the MediaType.TEXT_XML is actually not needed. We should use MediaType.APPLICATION_XML for XML content type. 5. The CertRequestResourceService.assignRequest() returns an empty CertRequestInfos. It doesn't seem to be necessary, but I haven't tested it. 6. In CertRequestResourceService.java:163 the original exception should be attached to the new exception as follows: throw new CMSException("Problem approving request in CertRequestRes urce.assignRequest!", e); 7. In CertRequestResourceService.java:165 the debug message "RequestNotFound" below is not necessary because the toString() will include the exception name in the output. It should be just: CMS.debug(e); -- Endi S. Dewata