Fix ingestion CLI #124
Fix ingestion CLI #124
Conversation
- Refactored cmd/ingest, since commands did not match their functionality. Signed-off-by: neilnaveen <[email protected]>
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
cmd/ingest/sbom/sbom.go (4)
31-33: Consider improving terminal output handling robustness.The current implementation has potential issues with terminal compatibility and width handling:
- Fixed 80-character clear might not work for all terminal widths
- No fallback for terminals that don't support ANSI escape codes
- Progress updates might cause issues in non-interactive terminals or logs
Consider using a terminal handling library like
github.com/fatih/colororgithub.jparrowsec.cn/mattn/go-isattyto:
- Detect terminal capabilities
- Handle terminal width dynamically
- Provide fallback for non-interactive terminals
+import ( + "os" + "github.com/mattn/go-isatty" +) -fmt.Printf("\r\033[1;36m%-80s\033[0m", " ") -fmt.Printf("\r\033[1;36mIngested %d/%d SBOMs\033[0m | \033[1;34m%s\033[0m", index+1, len(result), tools.TruncateString(data.Path, 50)) +if isatty.IsTerminal(os.Stdout.Fd()) { + fmt.Printf("\r\033[1;36mIngested %d/%d SBOMs\033[0m | \033[1;34m%s\033[0m", index+1, len(result), tools.TruncateString(data.Path, 50)) +} else { + fmt.Printf("Ingested %d/%d SBOMs: %s\n", index+1, len(result), data.Path) +}
36-36: Improve consistency in success and error messages.The success message uses plural form ("SBOMs") while error messages still use singular form ("SBOM"). Also, consider including the total count in the success message for better feedback.
-fmt.Println("\nSBOMs ingested successfully") +fmt.Printf("\nSuccessfully ingested %d SBOMs\n", len(result))And for consistency, update the error messages:
-return fmt.Errorf("failed to ingest SBOM: %w", err) +return fmt.Errorf("failed to ingest SBOMs: %w", err)
45-46: Enhance command documentation and remove trailing space.The command description could be more specific, and adding examples would improve usability. Also, there's an unnecessary trailing space in the Short description.
- Use: "sbom [path to sbom file/dir]", - Short: "Ingest an sbom into the graph ", + Use: "sbom [path to sbom file/dir]", + Short: "Ingest an SBOM into the graph", + Long: "Ingest Software Bill of Materials (SBOM) data from a file or directory into the graph database", + Example: ` + # Ingest a single SBOM file + minefield ingest sbom path/to/sbom.json + + # Ingest all SBOMs from a directory + minefield ingest sbom path/to/sboms/`,
Line range hint
12-14: Consider renaming for architectural clarity.Since the command description now mentions "graph" but the interface is named "Storage", consider aligning the naming for better architectural clarity.
type options struct { - storage graph.Storage + graph graph.Storage }This would make it clearer that we're working with a graph database implementation of the storage interface.
cmd/ingest/scorecard/scorecard.go (1)
Line range hint
56-58: Remove unused functionprintProgressThis function appears to be unused and still references "SBOMs" instead of "Scorecards".
The function should be removed as it's no longer used and contains outdated terminology.
cmd/ingest/osv/osv.go (2)
29-31: Consider handling non-ANSI terminalsWhile the ANSI color formatting improves readability, it might not work correctly in all terminal environments.
Consider detecting terminal capabilities or adding a flag to disable colors:
+func supportsANSI() bool { + term := os.Getenv("TERM") + return term != "dumb" && term != "" +} func (o *options) Run(_ *cobra.Command, args []string) error { + useColors := supportsANSI() // ... existing code ... - fmt.Printf("\r\033[1;36m%-80s\033[0m", " ") - fmt.Printf("\r\033[K\033[1;36mIngested %d/%d vulnerabilities\033[0m | \033[1;34mCurrent: %s\033[0m", + if useColors { + fmt.Printf("\r\033[1;36m%-80s\033[0m", " ") + fmt.Printf("\r\033[K\033[1;36mIngested %d/%d vulnerabilities\033[0m | \033[1;34mCurrent: %s\033[0m", + } else { + fmt.Printf("\rIngested %d/%d vulnerabilities | Current: %s", + }
Line range hint
53-55: Remove unusedprintProgressfunctionThe
printProgressfunction appears to be unused and contains outdated terminology ("Graphed" instead of "Ingested").-func printProgress(count int, path string) string { - return fmt.Sprintf("\033[1;36mGraphed %d vulnerabilities\033[0m | \033[1;34mCurrent: %s\033[0m", count, tools.TruncateString(path, 50)) -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cmd/ingest/osv/osv.go(2 hunks)cmd/ingest/sbom/sbom.go(2 hunks)cmd/ingest/scorecard/scorecard.go(3 hunks)
🔇 Additional comments (4)
cmd/ingest/scorecard/scorecard.go (2)
30-32: LGTM! Clean progress indication implementation
The progress indication is well-implemented with:
- Line clearing for smooth updates
- Color coding for better readability
- Path truncation to prevent overflow
35-35: LGTM! Clear success message
The success message correctly reflects the scorecard context.
cmd/ingest/osv/osv.go (2)
19-21: LGTM! Variable name improvement
The rename from vulnsDir to vulnsPath better reflects that the argument can be either a file or directory path.
33-33: LGTM! Clear success message
The success message clearly indicates the completion of the ingestion process.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation