10:46 a.m.
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
5:44 p.m.
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
6:23 p.m.
New subject: [PATCH] 27-2 Fixes for review comments on [PATCH] 27
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.
1:01 p.m.
New subject: [PATCH] 27-2 Fixes for review comments on [PATCH] 27
On 8/2/2012 6:23 PM, Abhishek Koneru wrote:
Please review the attached patch with fixes for comments given on
Patch
27.
Some more comments:
> 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.
I think we should use a Reader class instead of InputStream when
handling text data, the caller can use FileReader instead of
FileInputStream.
12. In CertFindCLI.java:111 let's add a space after the dot to separate
the exception message.
13. In CertFindCLI.java:162,265,267 I think we should use "uid" instead
of "id" as the parameter name to make it clearer.
14. In CertFindCLI.java:162 we should add an argument name "user id".
15. In CertFindCLI.java:187,190,296,298,300,302 the current
"revokedFrom" and "revokedTill" might be interpreted differently.
Let's
use the same parameter names as the field names in CertSearchData, in
this case "revokedOnFrom" and "revokedOnTo" (we can revise it again
later). And the following parameter descriptions might be better.
* revokedOnFrom: Revoked on or after this date
* revokedOnTo: Revoked on or before this date
16. In CertFindCLI.java:183 the "revokedBy" takes a "user id" as an
argument instead of "serial number".
17. Similarly, in CertFindCLI.java:201 the "issuedBy" takes a "user
id".
18. In CertFindCLI.java:149,152 let's use "serial number" as the command
argument instead of just "number".
19. In CertFindCLI.java:210-224 the certType* parameters take either on
or off value. So the arg names should be "on|off".
You can also go to the Agent's Certificate Manger -> Search for
Certificates to see how the parameters are used in the UI.
One more thing, I think we don't need those ...InUse fields in the
CertSearchData because they can be implied from the parameters we
specify. They can be removed in a separate ticket.
--
Endi S. Dewata
4281
days inactive
4287
days old
3 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata