[Pki-devel] [PATCH] 27 Dogtag 10 Beta - Ticket 150 . Implement Cert search/list request in CLI

Endi Sukma Dewata edewata at redhat.com
Wed Aug 1 22:44:18 UTC 2012


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.

-- 
Endi S. Dewata




More information about the Pki-devel mailing list