On 4/19/2013 1:42 PM, Abhishek Koneru wrote:
Please review the patch for trac ticket 217.
Added an additional check over the actual result of a revoke/unrevoke operation.
Some comments: 1. In CertRequestInfo the synchronized keywords don't seem to be necessary. CertRequestInfo objects are created to pass REST operation results to the client. They don't seem to be used by multiple threads. 2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values which are duplicated in several places. It might be better to use 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these values can be put in constant variables in CertRequestInfo: public static final String REQ_SUCCESS = "success"; public static final String REQ_ERROR = "error"; This way we can be sure the operationResult will be used consistently. 3. In CertRequestInfoFactory the code for operationResult assignment seems to be incorrect because it will assign 'fail' to completed requests without error. Try cert-request-find, it will show all requests for system certs as failed. 4. In CertCLI if the request status is COMPLETE but the operation result is 'fail' the status will appear as REJECTED. This could be confusing because the request is not actually rejected. I think we should display the operation result separately from the status. -- Endi S. Dewata