Code cleanup and refactoring#14
Merged
mvazquezc merged 3 commits intoopenshift-kni:mainfrom Apr 28, 2026
Merged
Conversation
Signed-off-by: Mario Vazquez <mavazque@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mvazquezc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughConsolidates ANSI/color handling into report packages, promotes several helpers to package scope, makes analyzer output-mode validation explicit with an error on unsupported modes, updates HTML escaping, and adjusts tests to match refactored helpers and escaping behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/report/html.go`:
- Around line 450-468: The current condByID map groups ConditionResult by
condResult.RuleID so multiple matched conditions for the same rule overwrite
each other; change the map to key by a condition-unique identifier (use the
ConditionResult's unique field if available, e.g. condResult.ID or
condResult.ConditionID; if none exists, use a composite key like
condResult.RuleID + "#" + index) when populating condByID from
ruleResult.Conditions, and then look up cond := condByID[id] using that
condition-unique id when building ruleTags so each RuleTagData (ID, Comment,
Impact, ImpactCSS via getImpactCSS) preserves the specific condition's metadata.
In `@pkg/report/text.go`:
- Line 332: The fmt.Fprint/Fprintf/Fprintln calls (e.g., fmt.Fprint(g.writer,
ColorGreen+line+ColorReset) in the package) currently ignore write errors;
replace these direct calls with small wrapper methods on the generator (e.g.,
g.write, g.writef, g.writeln) that call fmt.Fprint/Fprintf/Fprintln into
g.writer, capture the first non-nil error into a field like g.writeErr and
short-circuit further writes, and then change Generate(...) to return g.writeErr
instead of nil; apply the same replacement for the other occurrences noted (the
other fmt.* calls around the file/lines) so all write failures are propagated.
- Around line 490-499: In matchingRuleIDs, empty or whitespace
condResult.MatchedText causes strings.Contains to always return true; update the
function (matchingRuleIDs) to trim condResult.MatchedText into trimmedMatched
and skip the condition if trimmedMatched == "" before calling strings.Contains,
so only non-empty matches are considered (preserve the existing seen map logic
and RuleID deduplication for condResult.RuleID).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 3194d3e7-7c20-40d6-8a61-57381488110c
📒 Files selected for processing (10)
pkg/analyzer/analyzer.gopkg/parser/diff.gopkg/report/html.gopkg/report/html_test.gopkg/report/reporting.gopkg/report/reporting_test.gopkg/report/text.gopkg/report/text_test.gopkg/rules/engine.gopkg/rules/engine_test.go
💤 Files with no reviewable changes (1)
- pkg/parser/diff.go
Comment on lines
+450
to
+468
| // Build a lookup from rule ID to condition details. | ||
| condByID := make(map[string]rules.ConditionResult) | ||
| for _, condResult := range ruleResult.Conditions { | ||
| if condResult.ConditionType == diffType && condResult.Matched { | ||
| trimmedMatched := strings.TrimSpace(condResult.MatchedText) | ||
| if strings.Contains(trimmedLine, trimmedMatched) || strings.Contains(trimmedMatched, trimmedLine) { | ||
| if !seen[condResult.RuleID] { | ||
| seen[condResult.RuleID] = true | ||
| ruleTags = append(ruleTags, RuleTagData{ | ||
| ID: condResult.RuleID, | ||
| Comment: condResult.Comment, | ||
| Impact: condResult.Impact, | ||
| ImpactCSS: getImpactCSS(condResult.Impact), | ||
| }) | ||
| } | ||
| if condResult.Matched { | ||
| if _, exists := condByID[condResult.RuleID]; !exists { | ||
| condByID[condResult.RuleID] = condResult | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var ruleTags []RuleTagData | ||
| for _, id := range ids { | ||
| cond := condByID[id] | ||
| ruleTags = append(ruleTags, RuleTagData{ | ||
| ID: id, | ||
| Comment: cond.Comment, | ||
| Impact: cond.Impact, | ||
| ImpactCSS: getImpactCSS(cond.Impact), | ||
| }) |
There was a problem hiding this comment.
Preserve per-condition metadata when building rule tags.
Line 451 stores a single ConditionResult per RuleID, and Line 462 reuses it for every matched line. If one rule has multiple matched conditions, the UI can show the wrong tooltip/impact for that specific line.
💡 Suggested fix
func getMatchingRulesHTML(line, diffType string, ruleResult rules.EvaluationResult) []RuleTagData {
ids := matchingRuleIDs(line, diffType, ruleResult)
if len(ids) == 0 {
return nil
}
- // Build a lookup from rule ID to condition details.
- condByID := make(map[string]rules.ConditionResult)
- for _, condResult := range ruleResult.Conditions {
- if condResult.Matched {
- if _, exists := condByID[condResult.RuleID]; !exists {
- condByID[condResult.RuleID] = condResult
- }
- }
- }
-
var ruleTags []RuleTagData
+ trimmedLine := strings.TrimSpace(line)
for _, id := range ids {
- cond := condByID[id]
+ var cond rules.ConditionResult
+ found := false
+ for _, c := range ruleResult.Conditions {
+ if !c.Matched || c.RuleID != id || c.ConditionType != diffType {
+ continue
+ }
+ trimmedMatched := strings.TrimSpace(c.MatchedText)
+ if strings.Contains(trimmedLine, trimmedMatched) || strings.Contains(trimmedMatched, trimmedLine) {
+ cond = c
+ found = true
+ break
+ }
+ }
+ if !found {
+ continue
+ }
+
ruleTags = append(ruleTags, RuleTagData{
ID: id,
Comment: cond.Comment,
Impact: cond.Impact,
ImpactCSS: getImpactCSS(cond.Impact),
})
}
return ruleTags
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/report/html.go` around lines 450 - 468, The current condByID map groups
ConditionResult by condResult.RuleID so multiple matched conditions for the same
rule overwrite each other; change the map to key by a condition-unique
identifier (use the ConditionResult's unique field if available, e.g.
condResult.ID or condResult.ConditionID; if none exists, use a composite key
like condResult.RuleID + "#" + index) when populating condByID from
ruleResult.Conditions, and then look up cond := condByID[id] using that
condition-unique id when building ruleTags so each RuleTagData (ID, Comment,
Impact, ImpactCSS via getImpactCSS) preserves the specific condition's metadata.
| hasNeedsReview = true | ||
| } | ||
| fmt.Fprint(g.writer, parser.ColorGreen+line+parser.ColorReset) | ||
| fmt.Fprint(g.writer, ColorGreen+line+ColorReset) |
There was a problem hiding this comment.
Propagate write failures instead of silently continuing
These fmt.Fprint calls ignore write errors, and Generate(...) still returns nil. On broken pipes/files, report output can be truncated without surfacing failure.
Suggested direction
type TextGenerator struct {
ruleEngine *rules.Engine
writer io.Writer
+ writeErr error
}
+
+func (g *TextGenerator) write(parts ...string) {
+ if g.writeErr != nil {
+ return
+ }
+ _, g.writeErr = fmt.Fprint(g.writer, strings.Join(parts, ""))
+}Then replace direct fmt.Fprint/Fprintf/Fprintln incrementally with wrapper methods and return g.writeErr from Generate.
Also applies to: 351-351, 409-409
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 332-332: Error return value of fmt.Fprint is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/report/text.go` at line 332, The fmt.Fprint/Fprintf/Fprintln calls (e.g.,
fmt.Fprint(g.writer, ColorGreen+line+ColorReset) in the package) currently
ignore write errors; replace these direct calls with small wrapper methods on
the generator (e.g., g.write, g.writef, g.writeln) that call
fmt.Fprint/Fprintf/Fprintln into g.writer, capture the first non-nil error into
a field like g.writeErr and short-circuit further writes, and then change
Generate(...) to return g.writeErr instead of nil; apply the same replacement
for the other occurrences noted (the other fmt.* calls around the file/lines) so
all write failures are propagated.
Signed-off-by: Mario Vazquez <mavazque@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/report/text.go (1)
332-332:⚠️ Potential issue | 🟠 MajorPropagate writer failures instead of silently continuing.
These writes still ignore returned errors, so output can be truncated without surfacing failure, and
Generate(...)still returnsnil.Suggested fix
type TextGenerator struct { ruleEngine *rules.Engine writer io.Writer + writeErr error } +func (g *TextGenerator) write(parts ...string) { + if g.writeErr != nil { + return + } + _, g.writeErr = fmt.Fprint(g.writer, strings.Join(parts, "")) +} + func (g *TextGenerator) Generate(w io.Writer, report types.ValidationReport) error { g.writer = w + g.writeErr = nil ... - return nil + return g.writeErr } - fmt.Fprint(g.writer, ColorGreen+line+ColorReset) + g.write(ColorGreen, line, ColorReset)Also applies to: 351-351, 409-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/report/text.go` at line 332, The writes in the text report code are ignoring fmt.Fprint errors (e.g., the call that prints ColorGreen+line+ColorReset and the other writes at the other noted locations), so change those calls to check and return or wrap the error instead of discarding it; update the containing function (e.g., Generate(...) / the writer methods in pkg/report/text.go) to propagate the fmt.Fprint / fmt.Fprintln errors up to the caller (return the error or wrap with context) so that Generate(...) no longer returns nil when a write fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/report/text.go`:
- Line 332: The writes in the text report code are ignoring fmt.Fprint errors
(e.g., the call that prints ColorGreen+line+ColorReset and the other writes at
the other noted locations), so change those calls to check and return or wrap
the error instead of discarding it; update the containing function (e.g.,
Generate(...) / the writer methods in pkg/report/text.go) to propagate the
fmt.Fprint / fmt.Fprintln errors up to the caller (return the error or wrap with
context) so that Generate(...) no longer returns nil when a write fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 7ad7dbcb-69bd-49c2-9582-055612d9734b
📒 Files selected for processing (2)
pkg/report/html.gopkg/report/text.go
Signed-off-by: Mario Vazquez <mavazque@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.