-
Notifications
You must be signed in to change notification settings - Fork 37
🐛 Use containerless list-targets/sources by default #566
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
Signed-off-by: Dylan <[email protected]>
WalkthroughAdds runLocal-aware branching to label listing in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as kantra analyze
participant Analyze as analyze.go
participant Container as Container Label Lister
participant Local as Containerless Label Lister
User->>CLI: run --list-sources / --list-targets
CLI->>Analyze: execute handler
alt runLocal == true (containerless)
Analyze->>Local: listLabelsContainerless(ctx)
Local-->>Analyze: labels / error
Analyze-->>CLI: output / return
else runLocal == false (container)
Analyze->>Container: ListLabels(cmd.Context())
Container-->>Analyze: labels / error
Analyze-->>CLI: output / return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
LGTM
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) 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 #566 +/- ##
==========================================
+ Coverage 24.29% 24.79% +0.50%
==========================================
Files 32 32
Lines 4779 4787 +8
==========================================
+ Hits 1161 1187 +26
+ Misses 3492 3471 -21
- Partials 126 129 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Dylan <[email protected]>
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)
cmd/analyze-bin_test.go (1)
557-673
: LGTM: Integration tests verify runLocal behavior.These tests properly verify that the containerless listing respects the
--run-local
flag and defaults totrue
, which aligns with the PR objectives to fix issue #565.Optional: Extract flag parsing to reduce duplication.
The manual flag parsing logic at lines 632-643 and 750-761 is duplicated. Consider extracting it into a helper function:
func parseTestFlags(a *analyzeCommand, cmdArgs []string) { for _, arg := range cmdArgs { switch { case arg == "--list-targets": a.listTargets = true case arg == "--list-sources": a.listSources = true case arg == "--run-local=true": a.runLocal = true case arg == "--run-local=false": a.runLocal = false } } }Then call it from both test functions to improve maintainability.
Also applies to: 675-791
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/analyze-bin_test.go
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/analyze-bin_test.go (1)
cmd/command-context.go (1)
AnalyzeCommandContext
(21-38)
⏰ 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 (5)
cmd/analyze-bin_test.go (5)
4-4
: LGTM: Appropriate imports for new test functionality.The
context
andstrings
packages are correctly imported and used throughout the new containerless label listing tests.Also applies to: 7-7
385-555
: LGTM: Comprehensive test coverage for containerless label listing.This test thoroughly covers the core containerless label listing functionality with multiple scenarios:
- Valid target and source labels
- Mixed label types
- Edge cases (no matches, empty directories)
- Proper output format verification
The test structure is well-organized with proper setup, cleanup, and assertions.
793-873
: LGTM: Thorough error handling verification.The error handling tests properly cover critical failure scenarios:
- Missing kantra directory
- Missing rulesets directory
- Empty rulesets directory (should succeed with no output)
Error message assertions ensure failures are reported correctly.
875-963
: LGTM: Output format and sorting verification.This test ensures the containerless listing produces properly formatted output with:
- Correct header text
- Expected line count
- Alphabetically sorted technologies
The sorting verification at lines 956-959 correctly checks ascending order.
966-973
: LGTM: Simple and correct helper function.The
sliceContains
helper provides a clean way to check for flag presence in test arguments. The implementation is straightforward and correct.
* 🐛 Use containerless list-targets/sources by default Signed-off-by: Dylan <[email protected]> * Add tests Signed-off-by: Dylan <[email protected]> --------- Signed-off-by: Dylan <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
* 🐛 Use containerless list-targets/sources by default * Add tests --------- Signed-off-by: Dylan <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Co-authored-by: Dylan Murray <[email protected]>
Fixes #565
Summary by CodeRabbit
Bug Fixes
Performance
Tests