Prepares the tool for integration with kubecompare MCP#12
Merged
mvazquezc merged 3 commits intoopenshift-kni:mainfrom Apr 20, 2026
Merged
Prepares the tool for integration with kubecompare MCP#12mvazquezc merged 3 commits intoopenshift-kni:mainfrom
mvazquezc merged 3 commits intoopenshift-kni:mainfrom
Conversation
…ternal use of the required methods. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Mario Vazquez <mavazque@redhat.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: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdd bytes-based constructors/validators to expose analyzer/rules as a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Client
participant Analyzer as Analyzer (pkg/analyzer)
participant Rules as Rules Engine (pkg/rules)
participant Reporter as Reporter (pkg/report)
CLI->>Analyzer: NewFromBytes(rulesYAML, version)
Note right of Analyzer: validate YAML and regex patterns
Analyzer->>Rules: NewEngineFromBytes(rulesYAML, version)
Rules->>Rules: parse YAML, validate regexes
Rules-->>Analyzer: Engine or error
CLI->>Analyzer: Analyze(diff, format, mode)
Analyzer->>Reporter: Render(report, format, mode)
Reporter-->>CLI: output (text/html or text)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rules/engine.go (1)
57-63:⚠️ Potential issue | 🟡 MinorPreserve source file context when
NewEngineWithVersiondelegates to byte-based constructor.If YAML parsing fails, the current returned error loses the originating
rulesFile, which makes field debugging harder in multi-source flows.💡 Suggested fix
func NewEngineWithVersion(rulesFile, version string) (*Engine, error) { data, err := os.ReadFile(rulesFile) if err != nil { return nil, fmt.Errorf("failed to read rules file: %w", err) } - return NewEngineFromBytes(data, version) + engine, err := NewEngineFromBytes(data, version) + if err != nil { + return nil, fmt.Errorf("failed to initialize rules engine from %q: %w", rulesFile, err) + } + return engine, nil }As per coding guidelines,
**/*.go: "Wrap errors with context using fmt.Errorf("context: %w", err)".Also applies to: 68-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rules/engine.go` around lines 57 - 63, NewEngineWithVersion currently returns whatever NewEngineFromBytes returns and loses the original rulesFile context; change NewEngineWithVersion so after calling NewEngineFromBytes(data, version) it checks the error and, if non-nil, wraps it with fmt.Errorf("reading/parsing %s: %w", rulesFile, err) (or similar) to include the rulesFile name; apply the same pattern to NewEngine (the no-version variant) so both functions wrap and return errors from NewEngineFromBytes with the source file context preserved.
🧹 Nitpick comments (3)
AGENTS.md (1)
38-39: Consider using one term consistently: “regex” or “regexp”.The section currently mixes both forms; standardizing one term improves readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 38 - 39, The doc mixes “regex” and “regexp”; pick one term (e.g., use “regex”) and update all occurrences for consistency—replace mentions in the paragraph that reference rules.ValidateRulesRegexpPatterns, analyzer.New, and the --validate-rules-only path so they all use the chosen term (e.g., “regex patterns in condition rules (global_rules, rules)”) and ensure any code-symbol names or function references remain unchanged while only harmonizing the descriptive wording.pkg/rules/engine.go (1)
3-13: Reorder imports to standard → external → internal.
gopkg.in/yaml.v3should be grouped beforegithub.com/openshift-kni/rds-analyzer/pkg/types.💡 Suggested import grouping
import ( "fmt" "os" "path/filepath" "regexp" "strings" - "github.com/openshift-kni/rds-analyzer/pkg/types" - "gopkg.in/yaml.v3" + + "github.com/openshift-kni/rds-analyzer/pkg/types" )As per coding guidelines,
**/*.go: "Order imports as: standard library first, then external dependencies, then internal packages".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rules/engine.go` around lines 3 - 13, Reorder the import block so standard library packages (fmt, os, path/filepath, regexp, strings) come first, then external dependencies like gopkg.in/yaml.v3, and finally internal packages such as github.com/openshift-kni/rds-analyzer/pkg/types; update the import grouping in the import declaration in pkg/rules/engine.go accordingly so gopkg.in/yaml.v3 appears before the internal types import.pkg/analyzer/analyzer_test.go (1)
261-410: Consider consolidating theNewFromBytesscenario tests into a table-driven test.The current coverage is good, but these cases are repetitive and would be easier to extend/maintain in one table-driven block.
As per coding guidelines,
**/*_test.go: Use table-driven tests for multiple test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/analyzer/analyzer_test.go` around lines 261 - 410, Multiple TestNewFromBytes_* tests are repetitive; consolidate them into a single table-driven test that iterates cases for NewFromBytes (and subsequent Analyze calls) to reduce duplication. Create a slice of test cases with fields like name, rulesBytes, version, expectError (or expectOutputContains for Analyze), format/mode for Analyze, and expected HTML/text content; loop over cases and for each call NewFromBytes (or New for the file case), assert error behavior, and when applicable call Analyze and check buffer contents (use existing symbols NewFromBytes, New, Analyze, and types.ValidationReport). Replace the individual TestNewFromBytes_* functions with this single table-driven test to cover valid/invalid YAML, invalid version/regex, HTML output, reporting mode, and parity with New, using subtests t.Run for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 13-22: The fenced code block in the README-like section is missing
a language identifier which triggers markdownlint MD040; update the opening
fence (the ``` that precedes the directory tree) to include an explicit language
identifier (use "text") so it becomes ```text, keeping the block content
unchanged; this fix targets the fenced code block that contains the pkg/ ├──
analyzer/ ... internal/ └── cli/ tree in AGENTS.md to satisfy the linter.
---
Outside diff comments:
In `@pkg/rules/engine.go`:
- Around line 57-63: NewEngineWithVersion currently returns whatever
NewEngineFromBytes returns and loses the original rulesFile context; change
NewEngineWithVersion so after calling NewEngineFromBytes(data, version) it
checks the error and, if non-nil, wraps it with fmt.Errorf("reading/parsing %s:
%w", rulesFile, err) (or similar) to include the rulesFile name; apply the same
pattern to NewEngine (the no-version variant) so both functions wrap and return
errors from NewEngineFromBytes with the source file context preserved.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 38-39: The doc mixes “regex” and “regexp”; pick one term (e.g.,
use “regex”) and update all occurrences for consistency—replace mentions in the
paragraph that reference rules.ValidateRulesRegexpPatterns, analyzer.New, and
the --validate-rules-only path so they all use the chosen term (e.g., “regex
patterns in condition rules (global_rules, rules)”) and ensure any code-symbol
names or function references remain unchanged while only harmonizing the
descriptive wording.
In `@pkg/analyzer/analyzer_test.go`:
- Around line 261-410: Multiple TestNewFromBytes_* tests are repetitive;
consolidate them into a single table-driven test that iterates cases for
NewFromBytes (and subsequent Analyze calls) to reduce duplication. Create a
slice of test cases with fields like name, rulesBytes, version, expectError (or
expectOutputContains for Analyze), format/mode for Analyze, and expected
HTML/text content; loop over cases and for each call NewFromBytes (or New for
the file case), assert error behavior, and when applicable call Analyze and
check buffer contents (use existing symbols NewFromBytes, New, Analyze, and
types.ValidationReport). Replace the individual TestNewFromBytes_* functions
with this single table-driven test to cover valid/invalid YAML, invalid
version/regex, HTML output, reporting mode, and parity with New, using subtests
t.Run for each case.
In `@pkg/rules/engine.go`:
- Around line 3-13: Reorder the import block so standard library packages (fmt,
os, path/filepath, regexp, strings) come first, then external dependencies like
gopkg.in/yaml.v3, and finally internal packages such as
github.com/openshift-kni/rds-analyzer/pkg/types; update the import grouping in
the import declaration in pkg/rules/engine.go accordingly so gopkg.in/yaml.v3
appears before the internal types import.
🪄 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: Pro Plus
Run ID: 85375ee4-275c-4038-b42a-e40ea4461dec
📒 Files selected for processing (19)
AGENTS.mdinternal/cli/root.gopkg/analyzer/analyzer.gopkg/analyzer/analyzer_test.gopkg/parser/diff.gopkg/parser/diff_test.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.gopkg/rules/regex_validation_error.gopkg/rules/types.gopkg/rules/version.gopkg/rules/yaml_regex_validate.gopkg/types/compare.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/analyzer/analyzer_test.go (2)
314-332: Use a descriptive test variable name instead ofaThe single-letter identifier makes the new subtests harder to scan. Rename to
analyzerfor clarity.♻️ Proposed refactor
- a, err := NewFromBytes(tt.rulesData, tt.version) + analyzer, err := NewFromBytes(tt.rulesData, tt.version) if tt.wantErr { if err == nil { t.Fatal("expected error, got nil") } if tt.errContains != "" && !strings.Contains(err.Error(), tt.errContains) { t.Errorf("expected error containing %q, got: %v", tt.errContains, err) } return } if err != nil { t.Fatalf("unexpected error: %v", err) } - if a == nil { + if analyzer == nil { t.Fatal("expected analyzer, got nil") } - if tt.wantVersion != "" && a.GetTargetVersion() != tt.wantVersion { - t.Errorf("expected version %s, got %s", tt.wantVersion, a.GetTargetVersion()) + if tt.wantVersion != "" && analyzer.GetTargetVersion() != tt.wantVersion { + t.Errorf("expected version %s, got %s", tt.wantVersion, analyzer.GetTargetVersion()) }- a, err := NewFromBytes([]byte(testRulesYAML), "") + analyzer, err := NewFromBytes([]byte(testRulesYAML), "") if err != nil { t.Fatalf("Failed to create analyzer: %v", err) } @@ - if err := a.Analyze(&buf, report, tt.format, tt.mode); err != nil { + if err := analyzer.Analyze(&buf, report, tt.format, tt.mode); err != nil { t.Fatalf("Analyze failed: %v", err) }As per coding guidelines, "
**/*.go: Use meaningful variable names; avoid single letters except for loops`."Also applies to: 408-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/analyzer/analyzer_test.go` around lines 314 - 332, Rename the single-letter test variable "a" to a descriptive name "analyzer" where NewFromBytes(tt.rulesData, tt.version) is called and subsequently used (references include err check, nil check, and calling GetTargetVersion()), and apply the same rename to the other test block mentioned (the occurrences around lines 408-427) so subtests read clearly and adhere to the naming guideline.
261-261: Align test names to explicit scenario styleConsider renaming
TestNewFromBytesandTestNewFromBytes_Analyzeto scenario-explicit forms (for example,TestNewFromBytes_TableCasesandTestNewFromBytes_AnalyzeModes) for easier test intent discovery.As per coding guidelines, "
**/*_test.go: Name test functions TestFunctionName_Scenario`."Also applies to: 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/analyzer/analyzer_test.go` at line 261, Rename the vague test functions to explicit scenario-style names: change TestNewFromBytes to TestNewFromBytes_TableCases and change TestNewFromBytes_Analyze to TestNewFromBytes_AnalyzeModes (or other descriptive suffixes per the guideline), then update any references to these function names (calls, t.Run subtests, or test lists) and adjust comments to match; ensure the new names are exported as Test* and run `go test` to confirm no references remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/analyzer/analyzer_test.go`:
- Around line 314-332: Rename the single-letter test variable "a" to a
descriptive name "analyzer" where NewFromBytes(tt.rulesData, tt.version) is
called and subsequently used (references include err check, nil check, and
calling GetTargetVersion()), and apply the same rename to the other test block
mentioned (the occurrences around lines 408-427) so subtests read clearly and
adhere to the naming guideline.
- Line 261: Rename the vague test functions to explicit scenario-style names:
change TestNewFromBytes to TestNewFromBytes_TableCases and change
TestNewFromBytes_Analyze to TestNewFromBytes_AnalyzeModes (or other descriptive
suffixes per the guideline), then update any references to these function names
(calls, t.Run subtests, or test lists) and adjust comments to match; ensure the
new names are exported as Test* and run `go test` to confirm no references
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 901927d6-a82e-4315-98d2-847f6d4833cb
📒 Files selected for processing (3)
AGENTS.mdpkg/analyzer/analyzer_test.gopkg/rules/engine.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/rules/engine.go
Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Mario Vazquez <mavazque@redhat.com>
mvazquezc
force-pushed
the
prepare-mcp-integration
branch
from
a0e5809 to
d3aac28
Compare
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.