Add functionality to inspect nodes #104
Conversation
WalkthroughThe changes encompass updates to the Changes
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (4)
cmd/query/output/output.go (1)
22-25: Improve flag documentation and remove empty line.The flag description should provide more details about the expected format and behavior.
func (o *options) AddFlags(cmd *cobra.Command) { - - cmd.Flags().StringVar(&o.outputFile, "output-file", "", "output file") + cmd.Flags().StringVar(&o.outputFile, "output-file", "", "path to write JSON output (if not specified, writes to stdout)") }pkg/tools/ingest/sbom.go (1)
Line range hint
1-134: Consider these improvements for better maintainability and performance.
- Use
golang.org/x/sync/errgroupfor concurrent processing and proper error handling- Rename
nameToIdto something more descriptive likenodeIdentifierMapping- Consider batching edge operations for better performance
Here's an example of using errgroup for concurrent processing:
import "golang.org/x/sync/errgroup" func SBOM(sbomPath string, storage graph.Storage, progress func(count int, path string)) (int, error) { // ... existing code ... if info.IsDir() { g := new(errgroup.Group) for _, entry := range entries { entry := entry // https://golang.org/doc/faq#closures_and_goroutines g.Go(func() error { entryPath := filepath.Join(sbomPath, entry.Name()) subCount, err := SBOM(entryPath, storage, progress) if err != nil { return fmt.Errorf("failed to ingest SBOM from path %s: %w", entryPath, err) } atomic.AddInt32(&count, int32(subCount)) if progress != nil { progress(int(atomic.LoadInt32(&count)), entryPath) } return nil }) } if err := g.Wait(); err != nil { return count, err } } // ... rest of the code ... }README.md (2)
62-91: Add examples for the new node inspection functionality.While the example commands are clear and well-documented, consider adding examples demonstrating the new node inspection functionality, including:
- Viewing node information in stdout
- Writing node information to a file using the new output command
Example addition:
minefield query custom "dependents library pkg:dep2@1.0.0" --visualize + +# Inspect node information in stdout +minefield query output pkg:dep2@1.0.0 + +# Write node information to a file +minefield query output pkg:dep2@1.0.0 --output-file=node_info.txt🧰 Tools
🪛 Markdownlint
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
80-80: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
75-85: Maintain consistent list style throughout the document.The unordered lists use a mix of dashes (-) and asterisks (). For better maintainability, use asterisks () consistently throughout the document.
Apply this change:
- - This command generates a ranked list of packages, ordered by the number of other packages that depend on them. + * This command generates a ranked list of packages, ordered by the number of other packages that depend on them. - - This command queries the dependents for a specific package, in this case `dep2`. + * This command queries the dependents for a specific package, in this case `dep2`. - - These queries output the intersection of two queries, finding package dependencies shared between each pair. + * These queries output the intersection of two queries, finding package dependencies shared between each pair.🧰 Tools
🪛 Markdownlint
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
80-80: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- README.md (1 hunks)
- cmd/query/custom/custom.go (0 hunks)
- cmd/query/output/output.go (1 hunks)
- cmd/query/query.go (2 hunks)
- pkg/tools/ingest/sbom.go (2 hunks)
💤 Files with no reviewable changes (1)
- cmd/query/custom/custom.go
🧰 Additional context used
🪛 Markdownlint
README.md
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
80-80: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
🔇 Additional comments (3)
cmd/query/query.go (1)
6-6: LGTM! Clean integration of the new output command.The changes follow the existing command structure pattern and maintain consistency with the codebase. The new output functionality is properly integrated into the command hierarchy.
Also applies to: 20-20
cmd/query/output/output.go (2)
72-77: Make file permissions configurable and more secure.The file permissions are hard-coded to 0644. Consider making this configurable or using a more restrictive default.
[security]+const defaultFilePerms = 0600 // More restrictive default if o.outputFile != "" { - err = os.WriteFile(o.outputFile, jsonOutput, 0644) + err = os.WriteFile(o.outputFile, jsonOutput, defaultFilePerms) if err != nil { return fmt.Errorf("failed to write output file: %v", err) }
84-98:⚠️ Potential issueRemove unused storage parameter and improve command description.
The
storageparameter is not used in the implementation, and the command description could be more informative.-func New(storage graph.Storage) *cobra.Command { +func New() *cobra.Command { o := &options{ - storage: storage, } cmd := &cobra.Command{ Use: "output [node name]", - Short: "Output the node, with its metadata", + Short: "Display or save node information", + Long: `Query and output detailed information about a node, including its metadata. + The output can be displayed in the terminal or saved to a file.`, + Example: " minefield query output mynode --output-file=node-info.json", Args: cobra.ExactArgs(1), RunE: o.Run, DisableAutoGenTag: true, }Likely invalid or redundant comment.
- Updated the README to reflect changes - Removed the output dir from query custom, and moved it to its own command - We can either see the node in stdout or write it to a file Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
neilnaveen
force-pushed
the
neil/outputNodes
branch
from
5ec00ea
to
95259a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
cmd/query/globsearch/globsearch.go (3)
Line range hint
15-21: Consider enhancing the max-output flag configuration.While the current implementation works, consider these improvements:
- Add validation to prevent negative values for max-output
- Consider making the default limit configurable or higher for better usability
func (o *options) AddFlags(cmd *cobra.Command) { - cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "max output length") + cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "maximum number of results to display (must be positive)") + cmd.MarkFlagRequired("max-output") + cmd.PreRunE = func(cmd *cobra.Command, args []string) error { + if o.maxOutput <= 0 { + return fmt.Errorf("max-output must be positive, got %d", o.maxOutput) + } + return nil + } }
Line range hint
23-63: Several improvements needed in the Run implementation.The implementation has multiple areas that could be enhanced for better reliability and maintainability:
- Add timeout to HTTP client:
- httpClient := &http.Client{} + httpClient := &http.Client{ + Timeout: 30 * time.Second, + }
- Fix the off-by-one error in output limiting:
- if count > o.maxOutput { + if count >= o.maxOutput {
- Move the default address to a constant:
+const defaultServiceAddr = "http://localhost:8089" + func (o *options) Run(cmd *cobra.Command, args []string) error { pattern := args[0] httpClient := &http.Client{} addr := os.Getenv("BITBOMDEV_ADDR") if addr == "" { - addr = "http://localhost:8089" + addr = defaultServiceAddr }
Line range hint
65-81: Enhance command usage documentation.While the command configuration is correct, the usage description could be more helpful to users.
cmd := &cobra.Command{ - Use: "globsearch [pattern]", - Short: "Search for nodes by glob pattern", + Use: "globsearch [glob-pattern]", + Short: "Search for nodes by glob pattern (e.g., '*.jar' or 'com.example.*')", + Example: " minefield globsearch '*.jar'\n minefield globsearch 'org.apache.*' --max-output 20", Args: cobra.ExactArgs(1), RunE: o.Run, DisableAutoGenTag: true, }README.md (1)
75-75: Maintain consistent list style in markdown.For consistency with the rest of the document, consider using asterisks (*) instead of dashes (-) for unordered lists.
- - This command generates a ranked list of packages, ordered by the number of other packages that depend on them. + * This command generates a ranked list of packages, ordered by the number of other packages that depend on them. - - This command queries the dependents for a specific package, in this case `dep2`. + * This command queries the dependents for a specific package, in this case `dep2`. - - These queries output the intersection of two queries, finding package dependencies shared between each pair. + * These queries output the intersection of two queries, finding package dependencies shared between each pair.Also applies to: 80-80, 85-85
🧰 Tools
🪛 Markdownlint
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- README.md (1 hunks)
- cmd/query/custom/custom.go (1 hunks)
- cmd/query/globsearch/globsearch.go (1 hunks)
- cmd/query/output/output.go (1 hunks)
- cmd/query/query.go (2 hunks)
- pkg/tools/ingest/sbom.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/query/custom/custom.go
- cmd/query/query.go
- pkg/tools/ingest/sbom.go
🧰 Additional context used
🪛 Markdownlint
README.md
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
80-80: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
🔇 Additional comments (4)
cmd/query/globsearch/globsearch.go (2)
Line range hint
1-13: LGTM! Package structure and imports are well-organized.The package rename from
querytoglobsearchbetter reflects its specific functionality, improving code organization.
Line range hint
23-63: Verify the service address configuration across the codebase.The hardcoded service address and environment variable name should be consistent across the codebase.
✅ Verification successful
Service address configuration is consistent across the codebase
The verification shows that both the environment variable
BITBOMDEV_ADDRand the default addresslocalhost:8089(orhttp://localhost:8089) are consistently used across all client components:
- Cache service client
- Graph service client
- Query service client
- Leaderboard service client
- Service startup configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of the service address and environment variable echo "Searching for service address usage:" rg -A 2 "localhost:8089" echo "Searching for environment variable usage:" rg -A 2 "BITBOMDEV_ADDR"Length of output: 2840
README.md (2)
74-91: LGTM! Clear and accurate command examples.The command examples are well-structured and accurately reflect the new functionality for:
- Viewing node information (leaderboard and query commands)
- Consistent terminology using "library" instead of "PACKAGE"
- Visualization options
🧰 Tools
🪛 Markdownlint
75-75: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
80-80: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
85-85: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
66-68: Verify the example path exists.The example uses
testdata/smalldirectory. Let's verify this path exists in the repository.✅ Verification successful
Path
testdata/smallexists with valid SBOM filesThe directory
testdata/smallexists and contains three SBOM files (dep1.json, libA.json, libB.json), making the example command in the README accurate and functional.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the testdata/small directory exists and contains SBOM files if fd -t d "small$" testdata/; then echo "Directory exists. Checking contents:" fd . testdata/small/ else echo "Warning: testdata/small directory not found" fiLength of output: 225
Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes