-
Notifications
You must be signed in to change notification settings - Fork 53
✨ call Prepare() on each provider #899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdated Go module dependency versions and moved protobuf to indirect; parseRules now returns provider condition maps and NewPipeAnalyzer calls each provider's Prepare(ctx, conditions) before initializing the engine. Changes
Sequence DiagramsequenceDiagram
participant NewPipeAnalyzer
participant parseRules
participant ProviderClient as Provider
participant Engine
NewPipeAnalyzer->>parseRules: load discovery & violation rules
parseRules-->>NewPipeAnalyzer: discoveryRulesets, violationRulesets, neededProviders, providerConditions
rect rgb(235,245,255)
Note over NewPipeAnalyzer,ProviderClient: prepare providers with returned conditions
loop for each provider in providerConditions
NewPipeAnalyzer->>ProviderClient: Prepare(ctx, conditions)
ProviderClient-->>NewPipeAnalyzer: ok / error
end
end
NewPipeAnalyzer->>Engine: initialize engine with prepared providers
Engine-->>NewPipeAnalyzer: ready / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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 |
⛔ Snyk checks have failed. 14 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
69-113: Consider improving diagnosability aroundPreparefailuresWiring
providerConditionsthrough and only callingPreparefor providers that both have conditions and appear inneededProviderslooks sound. However, ifPreparefails, the plainreturn nil, errloses which provider caused the failure; wrapping it with the provider key would make debugging much easier, e.g.:if err := neededProviders[k].Prepare(ctx, v); err != nil { return nil, fmt.Errorf("prepare provider %q: %w", k, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kai_analyzer_rpc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
kai_analyzer_rpc/go.mod(1 hunks)kai_analyzer_rpc/pkg/service/pipe_analyzer.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
🔇 Additional comments (3)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (2)
33-45: Provider config validation matches stated constraintsThe updated checks and error messages correctly enforce that providers must have a non-empty
Address, noBinaryPath, and a non-emptyPipeName, which aligns with the “existing provider via pipe” requirement. No issues spotted here.
147-167:providerConditionsaggregation and newparseRulesreturn look correctThe new
providerConditionsmap is safely initialized, and mergingprovConditionsviaappendper provider correctly accumulates conditions across all rule files without nil-map issues. The updated return tuple (discoveryRulesets, violationRulesets, neededProviders, providerConditions, err) is consistent with the caller at Line 69.Also applies to: 205-205
kai_analyzer_rpc/go.mod (1)
54-54: No issues found; protobuf dependency placement is correct.The verification confirms that
google.golang.org/protobuf v1.36.6has zero direct usage in the kai_analyzer_rpc codebase. No imports, package references, or protobuf types appear in any of the 13 Go files. The dependency is correctly marked as// indirectbecause it is a transitive dependency pulled in by grpc and genproto packages, not directly consumed by kai_analyzer_rpc code.Likely an incorrect or invalid review comment.
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
2b95ceb to
fb93f0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
155-162: Add logging to Prepare() calls for consistency with ProviderInit pattern.The
Prepare()method is available in the current analyzer-lsp dependency version (v0.9.0-alpha.1.0.20251121202139-b3f742caf596), so the dependency concern is resolved. However, the code lacks logging for thePrepare()calls, whereasProviderInitcalls (lines 120-132) include informational logging. For consistent observability, consider adding log statements around thePrepare()invocations:// Call Prepare() on all providers with the provider conditions for k, v := range providerConditions { if _, ok := neededProviders[k]; ok { + l.Info("preparing provider", "provider", k) if err := neededProviders[k].Prepare(ctx, v); err != nil { return nil, err } + l.Info("provider prepared", "provider", k) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kai_analyzer_rpc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
kai_analyzer_rpc/go.mod(3 hunks)kai_analyzer_rpc/pkg/service/pipe_analyzer.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kai_analyzer_rpc/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Trunk Check Runner
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
🔇 Additional comments (3)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (3)
62-62: LGTM: Error message formatting improvements.The lowercase error messages now follow Go conventions where error strings should not be capitalized unless they begin with proper nouns or acronyms.
Also applies to: 66-66
96-96: LGTM: parseRules call updated to capture provider conditions.The function call correctly captures the new
providerConditionsreturn value, which is subsequently used to invokePrepare()on each provider.
197-217: LGTM: Provider conditions collection and aggregation logic is sound.The implementation correctly:
- Extends the function signature to return provider conditions
- Aggregates conditions from multiple rule files by provider name
- Handles error cases with appropriate nil returns
- Returns the collected conditions to the caller
The aggregation logic (lines 212-217) properly accumulates conditions across rule files, ensuring each provider receives all its conditions from all loaded rules.
Also applies to: 255-255
Signed-off-by: Pranav Gaikwad <[email protected]>
shawn-hurley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once CI is green good to go!
Requires konveyor/kai#899 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved document symbol resolution and navigation with enhanced error handling. * Refined definition lookup to better support multiple result types, providing more reliable "Go to Definition" functionality. * **Chores** * Updated provider initialization configuration for internal compatibility. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Pranav Gaikwad <[email protected]>
Requires konveyor/analyzer-lsp#975
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.