Speed up CA spawn by batching pki-server CLI calls. - #5372
Conversation
Batch group-member adds and fold cert request/create/import into a single ca-cert-create per cert; fix Java CLI routing for hyphenated subcommand names like add-batch. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR optimizes PKI spawn performance through CLI routing improvements, batch group member operations, and refactored certificate creation flows for DS-enabled deployments. It includes benchmark tooling to measure and validate these improvements. ChangesCore Features: CLI, Batch Operations, Deployment
Benchmark Analysis and Measurement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces optimizations to the PKI deployment and CLI routing, notably adding a batch group member addition feature (add-batch) to reduce JVM invocations and refactoring certificate creation to support direct CSR import. Benchmark scripts and baseline results are also included to document these performance gains. The review feedback highlights several key improvements: resolving a potential NullPointerException in CLI.java command routing, adding defensive checks for member_id in Python, handling string-type dns_names to prevent character-splitting bugs, ensuring safe binary writes for CSR files, and validating all groups beforehand in the batch CLI to prevent partial updates.
| // repeat for the remaining parts | ||
| current = module.getCLI(); | ||
| i = j + 1; | ||
| i = matchedJ + 1; |
There was a problem hiding this comment.
If no matching module is found during the command routing, module will remain null. This will lead to a NullPointerException when calling module.getCLI() on line 181.
Consider adding a null check before invoking module.getCLI() to throw a descriptive CLIException instead:
if (module == null) {
throw new CLIException("Unknown command: " + command);
}| if not group_ids: | ||
| return |
There was a problem hiding this comment.
Add a defensive check to ensure member_id is not None or empty before constructing the command list. This prevents a cryptic TypeError when calling subprocess.check_call with a None value.
if not group_ids:
return
if not member_id:
raise ValueError("member_id must be specified")| if dns_names: | ||
| cmd.extend(['--dns-names', ','.join(dns_names)]) |
There was a problem hiding this comment.
If dns_names is passed as a single string instead of a list of strings, calling ','.join(dns_names) will split the string by characters (e.g., 'example.com' becomes 'e,x,a,m,p,l,e,.,c,o,m'). Adding a check for isinstance(dns_names, str) prevents this common bug.
| if dns_names: | |
| cmd.extend(['--dns-names', ','.join(dns_names)]) | |
| if dns_names: | |
| if isinstance(dns_names, str): | |
| cmd.extend(['--dns-names', dns_names]) | |
| else: | |
| cmd.extend(['--dns-names', ','.join(dns_names)]) |
| with open(csr_path, 'w', encoding='utf-8') as f: | ||
| f.write(request_pem) |
There was a problem hiding this comment.
To prevent a potential TypeError if pki.nssdb.convert_csr returns bytes instead of str (or vice versa depending on the Python 3 environment/library version), write the file in binary mode ('wb') and encode the string if necessary.
| with open(csr_path, 'w', encoding='utf-8') as f: | |
| f.write(request_pem) | |
| with open(csr_path, 'wb') as f: | |
| if isinstance(request_pem, str): | |
| f.write(request_pem.encode('utf-8')) | |
| else: | |
| f.write(request_pem) |
| for (int i = 1; i < cmdArgs.length; i++) { | ||
| String groupID = cmdArgs[i]; | ||
|
|
||
| Group group = ugSubsystem.getGroupFromName(groupID); | ||
|
|
||
| if (group == null) { | ||
| throw new GroupNotFoundException(groupID); | ||
| } | ||
|
|
||
| group.addMemberName(memberID); | ||
| ugSubsystem.modifyGroup(group); | ||
| } |
There was a problem hiding this comment.
To prevent partial updates and ensure atomicity where possible, validate that all specified groups exist before performing any modifications. Currently, if a group in the middle of the list does not exist, the previous groups will have already been modified, leaving the system in a partially configured state.
java.util.List<Group> groups = new java.util.ArrayList<>();
for (int i = 1; i < cmdArgs.length; i++) {
String groupID = cmdArgs[i];
Group group = ugSubsystem.getGroupFromName(groupID);
if (group == null) {
throw new GroupNotFoundException(groupID);
}
groups.add(group);
}
for (Group group : groups) {
group.addMemberName(memberID);
ugSubsystem.modifyGroup(group);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
benchmarks/run-startup-baseline.sh (1)
9-13: ⚡ Quick winAdd preflight checks for required scripts and container availability.
Failing early with clear errors avoids partial logs and ambiguous failures during timed runs.
Proposed fix
CREATE=${CREATE:-../IDM-CI/bash/pki-create.sh} SPAWN=${SPAWN:-../IDM-CI/bash/pki-spawn.sh} CFG=${CFG:-/usr/share/pki/server/examples/installation/ca.cfg} SKIP_BUILD=${SKIP_BUILD:-} + +[[ -x "$CREATE" ]] || { echo "Missing or non-executable CREATE script: $CREATE" >&2; exit 1; } +[[ -x "$SPAWN" ]] || { echo "Missing or non-executable SPAWN script: $SPAWN" >&2; exit 1; } +docker inspect pki >/dev/null 2>&1 || { echo "Container 'pki' is not available." >&2; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/run-startup-baseline.sh` around lines 9 - 13, Add preflight checks at the top of run-startup-baseline.sh to fail fast with clear errors: verify that the files referenced by CREATE and SPAWN exist and are executable and that CFG exists/readable (use the CREATE, SPAWN, CFG variables), and short-circuit if SKIP_BUILD is set. Also check that the container runtime is available (e.g., docker or podman command exists and `docker ps`/`podman ps` succeeds) and print explicit error messages and exit non-zero if any check fails. Ensure messages name the failing variable (CREATE/SPAWN/CFG) and include suggested fixes so timed runs stop early with clear logs.base/common/src/main/java/org/dogtagpki/cli/CLI.java (1)
149-182: ⚡ Quick winAdd a regression test for longest-match routing.
This changes a core dispatch rule; please lock it down with coverage for overlapping names like
addvsadd-batchso future CLI additions do not silently regress routing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/common/src/main/java/org/dogtagpki/cli/CLI.java` around lines 149 - 182, Add a regression test that verifies the CLI dispatch chooses the longest matching module name when names overlap (e.g., "add" vs "add-batch"); exercise the parsing/dispatch logic that builds moduleName and uses current.getModule(...) (the loop using variables moduleName, matchedJ, j and assigning module via CLIModule m) and assert that the returned module is the longer match (add-batch) and that i/remaining dispatch behave correctly; implement the test in the CLI test suite to invoke the CLI entry point with both "add" and "add-batch" forms and fail if the shorter module is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@base/server/src/main/java/org/dogtagpki/server/cli/SubsystemGroupMemberAddBatchCLI.java`:
- Around line 71-81: The loop in SubsystemGroupMemberAddBatchCLI currently calls
ugSubsystem.getGroupFromName, group.addMemberName and ugSubsystem.modifyGroup
for each group as it iterates, causing partial updates if a later group is
invalid or modifyGroup fails; change the logic to first validate all target
groupIDs by calling ugSubsystem.getGroupFromName for each cmdArgs[i] and abort
if any are null, then perform the modifications; implement the update phase to
either (a) apply changes only after validation (calling group.addMemberName and
ugSubsystem.modifyGroup in a second loop) or (b) if possible use a transactional
API on ugSubsystem, and if modifyGroup can still fail ensure you revert
previously modified groups by calling group.removeMemberName on successful ones
in a catch block to avoid partial application (refer to
SubsystemGroupMemberAddBatchCLI, groupID, getGroupFromName, Group.addMemberName,
Group.removeMemberName, and ugSubsystem.modifyGroup).
In `@benchmarks/analyze-spawn-debug.py`:
- Around line 79-80: The current computation sets t0/t1 from the first/last raw
lines which can be None if header/footer lack timestamps; instead, map
parse_line_ts over all lines (using the same parse_line_ts function), filter out
None results, then set t0 to the minimum timestamp and t1 to the maximum
timestamp from that parsed list (fall back to None if no parsed timestamps
exist) so wall-clock bounds come from actual parsed timestamps rather than raw
first/last lines.
- Line 192: The current median calculation uses med = sorted(durs)[len(durs) //
2], which selects the upper middle element for even-length lists; change it to
compute the true median by sorting durs and, when len(durs) is even, averaging
the two middle elements (or simply use statistics.median on durs) so med
reflects the true median; update the assignment where med is computed in
benchmarks/analyze-spawn-debug.py (the sorted(durs) / med logic).
In `@benchmarks/spawn-debug-analysis.txt`:
- Line 1: The benchmark artifact contains an absolute local path
"/home/agaragna/Projects/pki/benchmarks/spawn-debug-timestamped.log"; update the
artifact generation or the committed file (benchmarks/spawn-debug-analysis.txt)
to use a repo-relative path (e.g., "benchmarks/spawn-debug-timestamped.log") or
a placeholder variable, ensuring any tooling that writes this log uses
repository-root-relative paths instead of absolute user-specific paths; modify
the producer of this entry (the script or tool that writes the log name) to
compute the relative path before writing.
In `@benchmarks/spawn-optimized-results.md`:
- Line 98: The ts timestamp format currently uses the integer epoch token (%s)
after seconds which does not produce sub-second fractions; in the pipeline line
that pipes to ts (the "--debug 2>&1 | ts ..." invocation) replace the "%S.%s"
pattern with the dotted fractional variant of seconds (use "%.S" so seconds +
fraction are emitted correctly) to ensure true sub-second timestamps are
produced.
---
Nitpick comments:
In `@base/common/src/main/java/org/dogtagpki/cli/CLI.java`:
- Around line 149-182: Add a regression test that verifies the CLI dispatch
chooses the longest matching module name when names overlap (e.g., "add" vs
"add-batch"); exercise the parsing/dispatch logic that builds moduleName and
uses current.getModule(...) (the loop using variables moduleName, matchedJ, j
and assigning module via CLIModule m) and assert that the returned module is the
longer match (add-batch) and that i/remaining dispatch behave correctly;
implement the test in the CLI test suite to invoke the CLI entry point with both
"add" and "add-batch" forms and fail if the shorter module is chosen.
In `@benchmarks/run-startup-baseline.sh`:
- Around line 9-13: Add preflight checks at the top of run-startup-baseline.sh
to fail fast with clear errors: verify that the files referenced by CREATE and
SPAWN exist and are executable and that CFG exists/readable (use the CREATE,
SPAWN, CFG variables), and short-circuit if SKIP_BUILD is set. Also check that
the container runtime is available (e.g., docker or podman command exists and
`docker ps`/`podman ps` succeeds) and print explicit error messages and exit
non-zero if any check fails. Ensure messages name the failing variable
(CREATE/SPAWN/CFG) and include suggested fixes so timed runs stop early with
clear logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86ab2ce9-a164-41aa-99d9-2823b04097d1
📒 Files selected for processing (14)
base/common/src/main/java/org/dogtagpki/cli/CLI.javabase/server/python/pki/server/cli/__init__.pybase/server/python/pki/server/cli/group.pybase/server/python/pki/server/deployment/__init__.pybase/server/python/pki/server/subsystem.pybase/server/src/main/java/org/dogtagpki/server/cli/SubsystemGroupMemberAddBatchCLI.javabase/server/src/main/java/org/dogtagpki/server/cli/SubsystemGroupMemberCLI.javabenchmarks/analyze-spawn-debug.pybenchmarks/run-startup-baseline.shbenchmarks/spawn-debug-analysis-optimized.txtbenchmarks/spawn-debug-analysis.txtbenchmarks/spawn-optimized-results.mdbenchmarks/spawn-validation-results.mdbenchmarks/startup-baseline.md
| for (int i = 1; i < cmdArgs.length; i++) { | ||
| String groupID = cmdArgs[i]; | ||
|
|
||
| Group group = ugSubsystem.getGroupFromName(groupID); | ||
|
|
||
| if (group == null) { | ||
| throw new GroupNotFoundException(groupID); | ||
| } | ||
|
|
||
| group.addMemberName(memberID); | ||
| ugSubsystem.modifyGroup(group); |
There was a problem hiding this comment.
Avoid partial group updates on later failures.
This persists each membership change as you iterate. If one of the later groupIDs is invalid or modifyGroup() fails, the earlier groups stay updated and the batch leaves the subsystem in a partially applied state.
Suggested direction
- for (int i = 1; i < cmdArgs.length; i++) {
- String groupID = cmdArgs[i];
-
- Group group = ugSubsystem.getGroupFromName(groupID);
-
- if (group == null) {
- throw new GroupNotFoundException(groupID);
- }
-
- group.addMemberName(memberID);
- ugSubsystem.modifyGroup(group);
- }
+ java.util.List<Group> groups = new java.util.ArrayList<>();
+ for (int i = 1; i < cmdArgs.length; i++) {
+ String groupID = cmdArgs[i];
+ Group group = ugSubsystem.getGroupFromName(groupID);
+ if (group == null) {
+ throw new GroupNotFoundException(groupID);
+ }
+ groups.add(group);
+ }
+
+ for (Group group : groups) {
+ group.addMemberName(memberID);
+ ugSubsystem.modifyGroup(group);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (int i = 1; i < cmdArgs.length; i++) { | |
| String groupID = cmdArgs[i]; | |
| Group group = ugSubsystem.getGroupFromName(groupID); | |
| if (group == null) { | |
| throw new GroupNotFoundException(groupID); | |
| } | |
| group.addMemberName(memberID); | |
| ugSubsystem.modifyGroup(group); | |
| java.util.List<Group> groups = new java.util.ArrayList<>(); | |
| for (int i = 1; i < cmdArgs.length; i++) { | |
| String groupID = cmdArgs[i]; | |
| Group group = ugSubsystem.getGroupFromName(groupID); | |
| if (group == null) { | |
| throw new GroupNotFoundException(groupID); | |
| } | |
| groups.add(group); | |
| } | |
| for (Group group : groups) { | |
| group.addMemberName(memberID); | |
| ugSubsystem.modifyGroup(group); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@base/server/src/main/java/org/dogtagpki/server/cli/SubsystemGroupMemberAddBatchCLI.java`
around lines 71 - 81, The loop in SubsystemGroupMemberAddBatchCLI currently
calls ugSubsystem.getGroupFromName, group.addMemberName and
ugSubsystem.modifyGroup for each group as it iterates, causing partial updates
if a later group is invalid or modifyGroup fails; change the logic to first
validate all target groupIDs by calling ugSubsystem.getGroupFromName for each
cmdArgs[i] and abort if any are null, then perform the modifications; implement
the update phase to either (a) apply changes only after validation (calling
group.addMemberName and ugSubsystem.modifyGroup in a second loop) or (b) if
possible use a transactional API on ugSubsystem, and if modifyGroup can still
fail ensure you revert previously modified groups by calling
group.removeMemberName on successful ones in a catch block to avoid partial
application (refer to SubsystemGroupMemberAddBatchCLI, groupID,
getGroupFromName, Group.addMemberName, Group.removeMemberName, and
ugSubsystem.modifyGroup).
| t0 = parse_line_ts(lines[0]) if lines else None | ||
| t1 = parse_line_ts(lines[-1]) if lines else None |
There was a problem hiding this comment.
Compute wall-clock bounds from parsed timestamps, not first/last raw lines.
If header/footer lines lack timestamps, t0/t1 becomes None or inaccurate despite valid data in the body.
Proposed fix
- t0 = parse_line_ts(lines[0]) if lines else None
- t1 = parse_line_ts(lines[-1]) if lines else None
+ parsed_ts = [parse_line_ts(line) for line in lines]
+ parsed_ts = [ts for ts in parsed_ts if ts is not None]
+ t0 = parsed_ts[0] if parsed_ts else None
+ t1 = parsed_ts[-1] if parsed_ts else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t0 = parse_line_ts(lines[0]) if lines else None | |
| t1 = parse_line_ts(lines[-1]) if lines else None | |
| parsed_ts = [parse_line_ts(line) for line in lines] | |
| parsed_ts = [ts for ts in parsed_ts if ts is not None] | |
| t0 = parsed_ts[0] if parsed_ts else None | |
| t1 = parsed_ts[-1] if parsed_ts else None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/analyze-spawn-debug.py` around lines 79 - 80, The current
computation sets t0/t1 from the first/last raw lines which can be None if
header/footer lack timestamps; instead, map parse_line_ts over all lines (using
the same parse_line_ts function), filter out None results, then set t0 to the
minimum timestamp and t1 to the maximum timestamp from that parsed list (fall
back to None if no parsed timestamps exist) so wall-clock bounds come from
actual parsed timestamps rather than raw first/last lines.
| durs = inv_durations[sub] | ||
| s = sum(durs) | ||
| total_ps_time += s | ||
| med = sorted(durs)[len(durs) // 2] |
There was a problem hiding this comment.
Use a true median for even-sized duration lists.
Current median logic picks the upper middle element for even n, which skews reported medians.
Proposed fix
+import statistics
...
- med = sorted(durs)[len(durs) // 2]
+ med = statistics.median(durs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| med = sorted(durs)[len(durs) // 2] | |
| med = statistics.median(durs) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/analyze-spawn-debug.py` at line 192, The current median
calculation uses med = sorted(durs)[len(durs) // 2], which selects the upper
middle element for even-length lists; change it to compute the true median by
sorting durs and, when len(durs) is even, averaging the two middle elements (or
simply use statistics.median on durs) so med reflects the true median; update
the assignment where med is computed in benchmarks/analyze-spawn-debug.py (the
sorted(durs) / med logic).
| @@ -0,0 +1,65 @@ | |||
| Log: /home/agaragna/Projects/pki/benchmarks/spawn-debug-timestamped.log | |||
There was a problem hiding this comment.
Avoid committing absolute local paths in benchmark artifacts.
Use repo-relative paths to keep results portable and avoid exposing local machine details.
Proposed fix
-Log: /home/agaragna/Projects/pki/benchmarks/spawn-debug-timestamped.log
+Log: benchmarks/spawn-debug-timestamped.log📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Log: /home/agaragna/Projects/pki/benchmarks/spawn-debug-timestamped.log | |
| Log: benchmarks/spawn-debug-timestamped.log |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/spawn-debug-analysis.txt` at line 1, The benchmark artifact
contains an absolute local path
"/home/agaragna/Projects/pki/benchmarks/spawn-debug-timestamped.log"; update the
artifact generation or the committed file (benchmarks/spawn-debug-analysis.txt)
to use a repo-relative path (e.g., "benchmarks/spawn-debug-timestamped.log") or
a placeholder variable, ensuring any tooling that writes this log uses
repository-root-relative paths instead of absolute user-specific paths; modify
the producer of this entry (the script or tool that writes the log name) to
compute the relative path before writing.
| -s CA \ | ||
| -D pki_audit_signing_nickname= \ | ||
| -D pki_ds_url=ldap://ds.example.com:3389 \ | ||
| --debug 2>&1 | ts '%Y-%m-%dT%H:%M:%S.%s' \ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In moreutils ts, what do %sand%Nrepresent, and what format should be used to emit ISO timestamp with true sub-second precision (e.g.,YYYY-MM-DDTHH:MM:SS.)?
💡 Result:
In the moreutils ts utility, the standard strftime(3) format specifiers apply [1][2]. However, to achieve sub-second resolution, moreutils provides extended versions of certain specifiers [1][3]. %s and %N: - %s represents the number of seconds since the Unix Epoch (seconds since 1970-01-01 00:00:00 UTC) [1][3]. - %N is a standard strftime specifier for nanoseconds [1], though it is not explicitly mentioned as an extended feature of ts. More critically, ts provides the extended specifier %.s, which represents seconds since the epoch with sub-second (microsecond) resolution [1][3]. To emit an ISO 8601 timestamp with true sub-second precision, you should use the extended specifier %.S (which adds the fractional second to the standard %S specifier) [1][2]. The recommended format string is: '%Y-%m-%dT%H:%M:%.S' Example usage: command | ts '%Y-%m-%dT%H:%M:%.S' This will produce output in the format YYYY-MM-DDTHH:MM:SS. [1][3]. The %.S specifier provides microsecond resolution [2][3].
Citations:
- 1: https://manpages.debian.org/unstable/moreutils/ts.1.en.html
- 2: https://unix.stackexchange.com/questions/272433/piping-into-moreutils-ts-with-nanosecond-precision
- 3: https://www.mankier.com/1/ts
🌐 Web query:
moreutils ts format specifier %N and %s; what specifier emits fractional seconds/sub-second precision (e.g., uses %.S or %.s)?
💡 Result:
In moreutils ts, fractional/sub-second output is produced by the dotted variants:
%.S= like%S(seconds), but with a fractional part (sub-second resolution)%.s= like%s(Unix epoch seconds), but with a fractional part%.T= like%T(%H:%M:%S), but with a fractional part (e.g.1:15:30.00001) [1]
%N (nanoseconds) is not a ts format specifier (it’s a GNU date extension and won’t work in ts); use %.S/%.s/%.T instead. [2]
Sources: [1] ts(1) man page (Debian) (manpages.debian.org), [2] Unix & Linux SE discussion of ts and %N (unix.stackexchange.com)
Citations:
- 1: https://manpages.debian.org/testing/moreutils/ts.1.en.html
- 2: https://unix.stackexchange.com/questions/272433/piping-into-moreutils-ts-with-nanosecond-precision?utm_source=openai
Fix moreutils ts fractional timestamp format (%s isn’t sub-second)
moreutils ts uses %s for Unix epoch seconds (integer), so ...:%S.%s does not emit a true sub-second fraction. Use the dotted fractional variants instead (e.g., %.S for %S with fractional part).
Suggested adjustment
- --debug 2>&1 | ts '%Y-%m-%dT%H:%M:%S.%s' \
+ --debug 2>&1 | ts '%Y-%m-%dT%H:%M:%.S' \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --debug 2>&1 | ts '%Y-%m-%dT%H:%M:%S.%s' \ | |
| --debug 2>&1 | ts '%Y-%m-%dT%H:%M:%.S' \ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/spawn-optimized-results.md` at line 98, The ts timestamp format
currently uses the integer epoch token (%s) after seconds which does not produce
sub-second fractions; in the pipeline line that pipes to ts (the "--debug 2>&1 |
ts ..." invocation) replace the "%S.%s" pattern with the dotted fractional
variant of seconds (use "%.S" so seconds + fraction are emitted correctly) to
ensure true sub-second timestamps are produced.
| print('Usage: pki-server %s-group-member-add-batch [OPTIONS] <member ID> <group ID>...' | ||
| % self.parent.parent.parent.name) |
There was a problem hiding this comment.
The pki-server <subsystem>-group-member-add-batch <member ID> <group ID> doesn't really go well with existing commands:
$ pki-server <subsystem>-group-add <group ID>
$ pki-server <subsystem>-group-member-find <group ID>
$ pki-server <subsystem>-group-member-add <group ID> <member ID>
$ pki-server <subsystem>-group-member-del <group ID> <member ID>
The pki-server <subsystem>-group-* commands are meant for groups, so the first param is always the <group ID>.
Since we want to add a user to multiple groups (i.e. roles) I'd suggest to update the following command instead to accept multiple groups/roles:
$ pki-server <subsystem>-user-role-add <user ID> <role ID> [<additional role IDs>...]
Note: The command is defined in both Python UserRoleAddCLI class and Java SubsystemUserRoleAddCLI class. The Python class is needed to add it as a subcommand of pki-server, but the actual implementation is in the Java class.
2 participants
Batch group-member adds and fold cert request/create/import into a single ca-cert-create per cert; fix Java CLI routing for hyphenated subcommand names like add-batch.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation