NOJIRA: Dummy PR automatic addressing code review#369
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neha037 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds must-gather controller logging for job age, cleanup counts, and configured timeouts, and validates image stream references before tag lookup. ChangesMust-gather controller updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (11 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
|
|
||
| // formatDuration converts a duration to a human-readable string like "2h 30m 15s". | ||
| func formatDuration(d time.Duration) string { | ||
| totalSeconds := int(d.Seconds()) | ||
| hours := totalSeconds / 3600 | ||
| minutes := (totalSeconds % 3600) / 60 | ||
| seconds := totalSeconds % 60 | ||
|
|
||
| if hours > 0 { | ||
| return fmt.Sprintf("%dh %dm %ds", hours, minutes, seconds) | ||
| } | ||
| if minutes > 0 { | ||
| return fmt.Sprintf("%dm %ds", minutes, seconds) | ||
| } | ||
| return fmt.Sprintf("%ds", seconds) | ||
| } |
There was a problem hiding this comment.
Just use duration.String()
There was a problem hiding this comment.
Done. Replaced custom formatDuration function with Go's built-in duration.String() method in controllers/mustgather/template.go:117.
AI-assisted response via Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
controllers/mustgather/mustgather_controller.go (4)
692-701: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop unnecessary
fmt.Sprintfcalls with no format arguments.
goerror.New(fmt.Sprintf("imageStreamRef.name must not be empty"))has no format directives;fmt.Sprintfis redundant here.♻️ Proposed fix
func validateImageStreamRef(ref *mustgatherv1alpha1.ImageStreamTagRef) error { if len(ref.Name) == 0 { - return goerror.New(fmt.Sprintf("imageStreamRef.name must not be empty")) + return goerror.New("imageStreamRef.name must not be empty") } if len(ref.Tag) == 0 { - return goerror.New(fmt.Sprintf("imageStreamRef.tag must not be empty")) + return goerror.New("imageStreamRef.tag must not be empty") } return nil }🤖 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 `@controllers/mustgather/mustgather_controller.go` around lines 692 - 701, The validateImageStreamRef helper is wrapping static error strings with unnecessary fmt.Sprintf calls. In validateImageStreamRef, replace the goerror.New(fmt.Sprintf(...)) calls for the Name and Tag checks with direct string-based error construction, since there are no format arguments; keep the same validation behavior while simplifying the error creation.
688-688: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
time.Sinceinstead oftime.Now().Sub(...).
time.Since(t)is the idiomatic shorthand fortime.Now().Sub(t).♻️ Proposed fix
- age := time.Now().Sub(job.CreationTimestamp.Time) + age := time.Since(job.CreationTimestamp.Time)🤖 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 `@controllers/mustgather/mustgather_controller.go` at line 688, Replace the age calculation in the mustgather controller with the idiomatic time.Since form instead of calling time.Now().Sub on job.CreationTimestamp.Time. Update the code in the logic that computes job age to use time.Since with the same timestamp value so the behavior stays identical while matching Go conventions.
703-712: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer structured key/value logging over pre-formatted messages.
Elsewhere in this file,
reqLogger.Infouses structured key-value pairs (e.g.reqLogger.Info("failed to get job", "job", instance.Name)), whereaslogCleanupSummarybuilds the whole message viafmt.Sprintf. This is inconsistent with the surrounding logging style and loses structured-log queryability (e.g., filtering bypodCount).♻️ Proposed fix
func logCleanupSummary(reqLogger logr.Logger, jobName string, podCount int) { - if podCount == 0 { - msg := fmt.Sprintf("cleanup complete for job %s: no pods were deleted", jobName) - reqLogger.Info(msg) - } else { - msg := fmt.Sprintf("cleanup complete for job %s: deleted %d pod(s)", jobName, podCount) - reqLogger.Info(msg) - } + reqLogger.Info("cleanup complete", "job", jobName, "podsDeleted", podCount) }🤖 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 `@controllers/mustgather/mustgather_controller.go` around lines 703 - 712, logCleanupSummary currently formats the entire cleanup message with fmt.Sprintf, which is inconsistent with the structured logging used elsewhere in mustgather_controller.go. Update logCleanupSummary to call reqLogger.Info with a constant message and key/value pairs for jobName and podCount (and any needed status field for the zero-pod case) so the cleanup summary remains queryable and consistent with the surrounding logging style.
687-690: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
formatDurationinstead ofDuration.String().
getJobAgeandtemplate.go's newformatDurationboth format atime.Durationfor logging but produce inconsistent output styles (age.String()yields Go's default"2h3m4.567s"format vs.formatDuration's"2h 30m 15s"). Consolidating avoids duplicated duration-formatting logic and keeps log output consistent across the reconciler.♻️ Proposed consolidation
func getJobAge(job *batchv1.Job) string { - age := time.Now().Sub(job.CreationTimestamp.Time) - return age.String() + return formatDuration(time.Since(job.CreationTimestamp.Time)) }🤖 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 `@controllers/mustgather/mustgather_controller.go` around lines 687 - 690, The getJobAge helper is formatting a duration directly with Duration.String(), which duplicates the duration formatting logic already centralized in formatDuration and produces inconsistent log output. Update getJobAge to reuse formatDuration for the computed age so the reconciler’s job-age logging matches the style used in template.go and keeps duration formatting in one place.controllers/mustgather/template.go (1)
430-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate duration-formatting logic vs.
getJobAgeinmustgather_controller.go.Same concern raised in
mustgather_controller.go—getJobAgeusesDuration.String()while this uses a custom"Xh Ym Zs"format, producing inconsistent duration representations across the same package's logs. Consider havinggetJobAgecall this helper (or vice versa) for a single source of truth.🤖 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 `@controllers/mustgather/template.go` around lines 430 - 445, The duration formatting logic is duplicated and inconsistent between formatDuration and getJobAge, so unify them to a single source of truth. Update getJobAge in mustgather_controller.go to use formatDuration (or have formatDuration delegate to the same logic) so all duration strings in this package share the same human-readable format. Keep the change centered around the existing getJobAge and formatDuration symbols so the representation stays consistent across logs.
🤖 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.
Nitpick comments:
In `@controllers/mustgather/mustgather_controller.go`:
- Around line 692-701: The validateImageStreamRef helper is wrapping static
error strings with unnecessary fmt.Sprintf calls. In validateImageStreamRef,
replace the goerror.New(fmt.Sprintf(...)) calls for the Name and Tag checks with
direct string-based error construction, since there are no format arguments;
keep the same validation behavior while simplifying the error creation.
- Line 688: Replace the age calculation in the mustgather controller with the
idiomatic time.Since form instead of calling time.Now().Sub on
job.CreationTimestamp.Time. Update the code in the logic that computes job age
to use time.Since with the same timestamp value so the behavior stays identical
while matching Go conventions.
- Around line 703-712: logCleanupSummary currently formats the entire cleanup
message with fmt.Sprintf, which is inconsistent with the structured logging used
elsewhere in mustgather_controller.go. Update logCleanupSummary to call
reqLogger.Info with a constant message and key/value pairs for jobName and
podCount (and any needed status field for the zero-pod case) so the cleanup
summary remains queryable and consistent with the surrounding logging style.
- Around line 687-690: The getJobAge helper is formatting a duration directly
with Duration.String(), which duplicates the duration formatting logic already
centralized in formatDuration and produces inconsistent log output. Update
getJobAge to reuse formatDuration for the computed age so the reconciler’s
job-age logging matches the style used in template.go and keeps duration
formatting in one place.
In `@controllers/mustgather/template.go`:
- Around line 430-445: The duration formatting logic is duplicated and
inconsistent between formatDuration and getJobAge, so unify them to a single
source of truth. Update getJobAge in mustgather_controller.go to use
formatDuration (or have formatDuration delegate to the same logic) so all
duration strings in this package share the same human-readable format. Keep the
change centered around the existing getJobAge and formatDuration symbols so the
representation stays consistent across logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 58dddf2e-0ad1-4311-b39c-4333d4395062
📒 Files selected for processing (2)
controllers/mustgather/mustgather_controller.gocontrollers/mustgather/template.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #369 +/- ##
==========================================
- Coverage 82.32% 81.97% -0.35%
==========================================
Files 8 8
Lines 905 921 +16
==========================================
+ Hits 745 755 +10
- Misses 148 151 +3
- Partials 12 15 +3
🚀 New features to boost your workflow:
|
…pe-pr-agent - Use time.Since instead of time.Now().Sub for idiomatic duration calculation - Remove unnecessary fmt.Sprintf calls in validateImageStreamRef - Convert logCleanupSummary to structured logging with key/value pairs
|
Done. Applied the following code quality improvements in commit fd50b5e:
Note on formatDuration: The suggestions about unifying duration formatting (lines 687-690 and template.go 430-445) were already addressed in commit 1c11252, which removed the custom formatDuration function in favor of using .String() consistently throughout the codebase. AI-assisted response via Claude Code |
|
/retest |
|
@neha037: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit