✨ call Prepare() for every provider #635
Conversation
Signed-off-by: Pranav Gaikwad <[email protected]>
WalkthroughAggregates provider-specific conditions returned by rule loading, updates Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Parser as LoadRules
participant Acc as ProviderConditions
participant Provider as Provider.Prepare
participant Engine as RuleEngine
Main->>Parser: LoadRules(ruleFiles...)
Parser-->>Main: ruleSets, needProviders, provConditions, err
Main->>Acc: merge provConditions into providerConditions
Note over Main,Provider: Post-load provider preparation (non-blocking errors)
loop for each provider ∈ providerConditions ∩ needProviders
Main->>Provider: Prepare(ctx, aggregatedConditions)
alt success
Provider-->>Main: ok
else failure
Provider-->>Main: error (logged)
end
end
Main->>Engine: Execute rules with prepared providers
Engine-->>Main: Results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-08T15:06:52.056ZApplied to files:
📚 Learning: 2025-06-26T14:08:58.555ZApplied to files:
⏰ 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). (1)
🔇 Additional comments (5)
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 |
![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
🧹 Nitpick comments (1)
go.mod (1)
138-148: Personal forkreplacedirectives likely need an explicit planThe
replaceentries pointgithub.zerozr99.workers.dev/konveyor/analyzer-lsp(and the Java external provider module) github.com/pranavgaikwad/analyzer-lsp.... That’s fine for local development, but if this is merged tomainit permanently couples Kantra to a personal fork/version.If this is only to test the new
Prepare/conditions plumbing, consider either:
- Switching back to an upstream tag/commit before merging, or
- Documenting that Kantra intentionally depends on this fork and why.
Please confirm the intended long‑term source for these modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/analyze-bin.go(3 hunks)cmd/analyze-hybrid.go(5 hunks)go.mod(1 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (7)
cmd/analyze-hybrid.go (4)
269-283: Network provider BinaryPath comment-only change is fine
BinaryPath: ""with the clarified comment still correctly indicates network mode; no behavioral change here.
395-407: Builtin provider config change is reasonableSetting
Location: a.inputandAnalysisMode: provider.AnalysisMode(a.mode)explicitly for the builtin provider in hybrid mode aligns it with the containerless setup and looks correct.
815-822: Provider preparation loop is correct once aggregation is made race‑freePreparing each
needProviders[name]with its aggregatedconditionsis the right place and scope for the newPreparephase. OnceproviderConditionsis populated without races, this block looks good; error logging without aborting matches the rest of the flow.
856-859: Progress cancellation comment/spacing change is benignThe explicit
progressCancel()then<-progressDonepattern is unchanged behaviorally; only the comment/spacing were adjusted. No issues here.cmd/analyze-bin.go (3)
265-291: Sequential aggregation of provider conditions looks correctIn the containerless flow,
providerConditions := map[string][]provider.ConditionsByCap{}(Line 269) is built synchronously inside thefor _, f := range a.rulesloop. The aggregation loop overprovConditions(Lines 285-290) correctly:
- Initializes an empty slice per provider on first sighting.
- Appends all
ConditionsByCapentries across rule files.Since rule loading here is not parallelized, there’s no concurrency concern for this map.
277-299: ProviderPreparephase is wired in as intendedUnpacking
provConditionsfromparser.LoadRules(Line 277) and later iteratingproviderConditionsto callprovider.Prepare(ctx, conditions)for each provider inneedProviders(Lines 293-299) correctly ensures that:
- Every provider actually used by the loaded rules is prepared.
- Preparation happens after all rules are loaded and conditions aggregated.
- Failures in
Prepareare logged but don’t abort analysis, which matches the existing “best-effort” philosophy elsewhere.If you ever want provider‑prep failures to be fatal, this would be the place to switch from log‑and‑continue to returning an error.
331-334: Progress cancellation comment-only tweak is fineThe explicit
progressCancel()followed by<-progressDonematches the hybrid flow and keeps the progress reporter goroutine cleaned up; the change here is purely comment/spacing.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (5.40%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #635 +/- ##
==========================================
- Coverage 36.41% 36.24% -0.18%
==========================================
Files 34 34
Lines 4257 4277 +20
==========================================
Hits 1550 1550
- Misses 2535 2555 +20
Partials 172 172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
⛔ Snyk checks have failed. 118 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/analyze-hybrid.go(6 hunks)go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
Repo: konveyor/kantra PR: 524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
cmd/analyze-hybrid.go
📚 Learning: 2025-06-26T14:08:58.555Z
Learnt from: aufi
Repo: konveyor/kantra PR: 500
File: cmd/static-report.go:88-94
Timestamp: 2025-06-26T14:08:58.555Z
Learning: In the konveyor/kantra codebase, there's a known pattern where Go maps are iterated by value-only (for _, item := range map), causing modifications to struct fields like `Extras = nil` to not persist. This appears in multiple places including static-report functionality and is acknowledged as a systemic issue to be addressed later rather than fixed piecemeal.
Applied to files:
cmd/analyze-hybrid.go
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (2)
cmd/analyze-hybrid.go (1)
743-797: Data race fix successfully implemented.The previous data race on
providerConditionshas been correctly resolved. The code now:
- Adds
provConditionsto theruleLoadResultstruct (line 747)- Sends each goroutine's
provConditionsvia the channel (lines 766-772)- Merges all results on the main goroutine after completion (lines 791-796)
This ensures all map writes happen on a single goroutine, eliminating the race condition.
go.mod (1)
138-139: I need to gather more information about PR #975 and verify the modules are actually available in the Go registry.Let me verify module availability and check PR #975 details directly:
Let me get more details on PR #975 and verify the module files/commit history:Based on my verification:
The dependency versions are correctly aligned with the required PR.
PR #975 was merged on 2025-11-21T20:21:39Z, and commit 25a951d7e1d9 exists in the analyzer-lsp repository with a timestamp of 2025-11-22T14:52:07Z. The pseudo-version encoding in go.mod (
v0.9.0-alpha.1.0.20251122145207-25a951d7e1d9) correctly reflects this commit's timestamp and short SHA, confirming the dependency references the post-merge state of analyzer-lsp.Both dependencies are alpha pseudo-versions pointing to the same commit, which is appropriate for tracking development commits before official tagged releases.
3 participants
Requires konveyor/analyzer-lsp#975
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.