Please review the attached patch with fixes for comments given on Patch
27.
--Abhishek
On Wed, 2012-08-01 at 17:44 -0500, Endi Sukma Dewata wrote:
> On 7/31/2012 10:46 AM, Abhishek Koneru wrote:
> > Please review the attached patch, which has the fix for Ticket 150. Implementing interface for all the UI search - request functionality in CLI.
> > Also attached the sample cert request xml form file.
> >
> > --Abhishek Koneru
>
> The patch needs rebasing and there's a conflict. As discussed, go ahead
> and override my recent changes to cert-find because it doesn't seem to
> be needed anymore. Some comments:
>
> 1. The CertFindCLI.printHelp() generates the following help message:
>
> usage: cert-find<filename>(Optional) [OPTIONS...]
>
> I think we can use the [...] to indicate the optional filename, so it
> will look like this (also note the spacing):
>
> usage: cert-find [filename] [OPTIONS...]
>
> 2. Optional: We don't have this yet, but we might want to reserve the
> command line argument for search keyword which can be used to search all
> fields:
>
> pki cert-find abhishek
>
> It would match 'abhishek' in username, email, subject DN, etc. If we
> decide to do this, we would use an option to specify the file name:
>
> usage: cert-find [keyword] [OPTIONS...]
>
> --input <filename> File containing the search constraints.
>
> 3. The code in CertFindCLI.java:68 needs formatting.
>
> 4. The if-then condition in line 75 can be simplified as follows:
>
> if (<filename specified>) {
> // load searchData from file
> } else {
> // create default searchData
> }
>
> // modify searchData based on the options
>
> The code in line 99-101 is no longer needed.
>
> 5. In line 104 it's not necessary to use a loop to iterate through all
> options.
>
> // modify searchData based on the options
> applyOptions(cmd, searchData)
>
> In applyOptions() it can go through all possible options sequentially:
>
> if (!cmd.hasOption("minSerialNumber")) {
> searchData.setSerialNumberRangeInUse(true);
> searchData.setSerialFrom(cmd.getOptionValue("minSerialNumber"));
> }
>
> if (!cmd.hasOption("maxSerialNumber")) {
> searchData.setSerialNumberRangeInUse(true);
> searchData.setSerialTo(cmd.getOptionValue("maxSerialNumber"));
> }
>
> It's not necessary to trim() the value because they are are already
> trimmed unless they are quoted.
>
> 6. In line 111 it should show the exception message.
>
> 7. In line 114 it should check the certInfos size too:
>
> if (certs.getCertInfos() == null || certs.getCertInfos().isEmpty()) {
> // no matches found
> }
>
> 8. In addOptions() the option descriptions are not consistent. Let's
> capitalize the first word only, e.g. "Minimum serial number", and use no
> space before colon, one space after that.
>
> 9. If an option has an argument (the third param is true) we should
> specify the argument name, for example:
>
> option = new Option(null, "validNotBeforeFrom", true,
> "Valid not before start date");
> option.setArgName("date")
> options.addOption(option);
>
> It will appear as:
>
> --validNotBeforeFrom <date> Valid not before start date
>
> 10. The option description should include the default value (if any) and
> acceptable values (if not obvious from the description) or example, for
> example the date format. This might require investigating the server
> code. Feel free to file a ticket for this.
>
> 11. The CertSearchData.valueOf() should take a more generic input, e.g.
> Reader, so it can be used to read other inputs, not just files.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0027-2-Feature-Search-certificate-request-interface-in-CLI..patch
Type: text/x-patch
Size: 18607 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120802/6a38eb50/attachment.bin>