Please review the patch with fixes for review comments on patch 53. On Mon, 2013-04-22 at 14:41 -0500, Endi Sukma Dewata wrote:
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.
--Removed the synchronized keywords
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.
-- Added the constant variables in CertRequestInfo
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.
-- Corrected. Null value for result is given as a SUCCESS
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.
-- Printing the operation result seperately. --Abhishek