Refactor to use google/wire instead of fx #133
Conversation
- refactored the custom cmd Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
…orage initialization** - Remove dependency on `go.uber.org/fx` and associated dependency injection setup. - Simplify `main.go` by removing `fx.App` and directly executing the root command. - Modify `cmd/root/root.go`: - Rename `options` struct to `Options` to make it exportable. - Update `AddFlags` method to use exported fields. - Add `PersistentPreRun` function to start the pprof server if enabled. - Change `New` function signature to return `(*cobra.Command, error)` instead of just `*cobra.Command`. - Modify `cmd/server/server.go`: - Add `StorageType` and `StorageAddr` fields to options. - Implement `ProvideStorage` method to initialize storage based on `StorageType`. - Update `Run` method to initialize storage via `ProvideStorage`. - Remove `storage` parameter from the `New` function. - Adjust `New` function accordingly. - Update `go.mod` and `go.sum` to remove `fx` and related dependencies. - Delete unused `storages` module and its tests. - Move storage initialization logic into `cmd/server/server.go`. This refactoring removes the usage of the `go.uber.org/fx` dependency injection framework, simplifying the application startup process. Storage initialization is now handled within the server command and can be configured via command-line flags. This change streamlines the application and reduces unnecessary complexity. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
WalkthroughThe pull request introduces multiple changes across various files, primarily focusing on enhancing the build process and dependency management by integrating the Wire library. Key modifications include the addition of a Wire installation step in the GitHub Actions workflow and updates to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@naveensrinivasan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (29)
main.go (2)
11-15: LGTM! Clean error handling for initialization.The error handling is clear and follows Go best practices by providing a descriptive error message to stderr.
Consider using
log.Fatalfor slightly more detailed error output:- fmt.Fprintf(os.Stderr, "Error initializing root command: %v\n", err) - os.Exit(1) + log.Fatalf("Error initializing root command: %v", err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-15: main.go#L11-L15
Added lines #L11 - L15 were not covered by tests
Line range hint
1-19: Excellent refactoring from fx to Wire!The refactoring has successfully:
- Simplified the main function by removing complex initialization
- Eliminated unnecessary pprof setup
- Moved dependency injection to Wire, which is more compile-time safe than fx
This aligns perfectly with the PR objective and improves code maintainability.
Consider adding basic integration tests for the main function to address the code coverage warnings. While main package testing can be tricky, you could test the basic initialization flow using a test helper:
// main_test.go func TestMainInitialization(t *testing.T) { cmd, err := root.New() require.NoError(t, err) require.NotNil(t, cmd) }Would you like me to help create these tests?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-15: main.go#L11-L15
Added lines #L11 - L15 were not covered by tests
[warning] 17-19: main.go#L17-L19
Added lines #L17 - L19 were not covered by testscmd/server/wire.go (1)
22-24: Document possible error cases.The function handles storage provisioning but should document potential error scenarios.
Consider adding documentation:
+// ProvideStorage creates a storage instance using the options configuration. +// Returns an error if storage initialization fails or if the storage type is unsupported. func ProvideStorage(o *options) (graph.Storage, error) {cmd/leaderboard/leaderboard.go (1)
Line range hint
13-28: Add tests for the command initializationThe new command initialization code lacks test coverage. Consider adding tests to verify:
- Command creation with correct name and flags
- Proper subcommand registration
Would you like me to help create a test file with the necessary test cases? This could include:
- Testing command creation
- Verifying subcommand registration
- Checking flag configuration
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by testscmd/query/query.go (1)
10-10: Add test coverage for the refactored New() functionWhile removing the storage parameter aligns with the Wire-based dependency injection approach, we should ensure proper test coverage for this refactored function.
Would you like me to help create unit tests for this function? I can generate test cases that verify:
- Command creation and initialization
- Proper subcommand registration
- Command properties (Use, Short, Long descriptions)
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: cmd/query/query.go#L10
Added line #L10 was not covered by testsMakefile (2)
43-44: Enhance the wire target's robustnessThe wire target implementation could be more robust by:
- Checking if wire is installed
- Creating the output directory if it doesn't exist
Consider this enhanced implementation:
wire: - cd cmd/server && wire + which wire > /dev/null || (echo "wire not found. Run: go install github.com/google/wire/cmd/wire@latest" && exit 1) + mkdir -p cmd/server + cd cmd/server && wire
46-46: Consider removing redundant wire dependencyThe
wiretarget is already a dependency ofbuild, making its inclusion in thealltarget redundant.-all: build test docker-build go-mod-tidy git-porcelain wire +all: build test docker-build go-mod-tidy git-porcelaincmd/root/root.go (2)
15-17: Add unit tests for the Options structThe Options struct and its fields lack test coverage. Consider adding tests to verify the initialization and field assignments.
Would you like me to help generate unit tests for the Options struct?
25-27: Return meaningful errors from New functionThe function signature now includes error return but always returns nil. Consider handling potential initialization errors.
func New() (*cobra.Command, error) { - o := &Options{} + o := &Options{} + if err := validateOptions(o); err != nil { + return nil, fmt.Errorf("invalid options: %w", err) + } rootCmd := &cobra.Command{Also applies to: 49-49
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-27: cmd/root/root.go#L25-L27
Added lines #L25 - L27 were not covered by testscmd/ingest/sbom/sbom.go (1)
39-39: Consider improving the progress indication implementationWhile the storage parameter removal looks good, the progress indication code could be improved for better maintainability.
Consider these improvements:
+ const ( + DefaultAddr = "http://localhost:8089" + ProgressLineWidth = 80 + MaxPathDisplay = 50 + ) + + func printProgress(current, total int, path string) { + // Clear the line + fmt.Printf("\r\033[1;36m%-*s\033[0m", ProgressLineWidth, " ") + // Print progress + fmt.Printf("\r\033[1;36mIngested %d/%d SBOMs\033[0m | \033[1;34m%s\033[0m", + current, total, tools.TruncateString(path, MaxPathDisplay)) + }Then in the Run method:
- 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)) + printProgress(index+1, len(result), data.Path)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/sbom/sbom.go#L39
Added line #L39 was not covered by testsgo.mod (1)
Line range hint
3-3: Fix incorrect Go versionThe specified Go version
1.23.1is invalid as Go 1.23 has not been released yet. The latest stable version is Go 1.22.x.Apply this diff to update to a valid Go version:
-go 1.23.1 +go 1.22.1cmd/ingest/osv/osv.go (2)
57-58: Consider adding basic command initialization testsThe simplified New function aligns well with the wire-based dependency injection approach. While the change is good, consider adding basic tests to ensure proper command initialization.
Example test cases could verify:
- Default flag values
- Command usage and description
- Flag parsing behavior
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: cmd/ingest/osv/osv.go#L57-L58
Added lines #L57 - L58 were not covered by tests
Line range hint
38-42: Enhance error context for troubleshootingWhile error handling exists, the error messages could provide more context to aid in troubleshooting.
Consider enhancing the error context:
result, err := ingest.LoadDataFromPath(vulnsPath) if err != nil { - return fmt.Errorf("failed to load vulnerabilities: %w", err) + return fmt.Errorf("failed to load vulnerabilities from %s: %w", vulnsPath, err) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-38: cmd/ingest/osv/osv.go#L38
Added line #L38 was not covered by testspkg/tools/ingest/loader.go (1)
Line range hint
37-43: Fix duplicate error handlingThere's a duplicate error handling block that appends the same error twice to the
errorsslice. The second error check and append is redundant.Apply this fix:
subResult, err := LoadDataFromPath(entryPath) if err != nil { errors = append(errors, fmt.Errorf("failed to load data from path %s: %w", entryPath, err)) } else { result = append(result, subResult...) } - if err != nil { - errors = append(errors, fmt.Errorf("failed to ingest data from path %s: %w", entryPath, err)) - }pkg/tools/ingest/sbom_test.go (2)
Line range hint
71-81: Consider improving test cleanup with t.Cleanup()The current cleanup approach using direct
os.Removecalls at the end of the test could be made more robust usingt.Cleanup(). This ensures cleanup runs even if the test fails or panics.func TestIngestSBOM(t *testing.T) { createTestFiles(t) + t.Cleanup(func() { + _ = os.RemoveAll("../../../empty_dir") + _ = os.Remove("../../../invalid_sbom.json") + _ = os.Remove("../../../no_components_sbom.json") + }) tests := []struct { // ... test cases ... } // ... test logic ... - if err := os.RemoveAll("../../../empty_dir"); err != nil { - t.Fatal(err) - } - if err := os.Remove("../../../invalid_sbom.json"); err != nil { - t.Fatal(err) - } - if err := os.Remove("../../../no_components_sbom.json"); err != nil { - t.Fatal(err) - } }
Line range hint
89-116: Consider using testdata directory for test filesThe test files are currently created with hardcoded relative paths (
../../../). Consider using atestdatadirectory, which is the Go convention for test files.func createTestFiles(t *testing.T) { t.Helper() + const testDataDir = "testdata" + if err := os.MkdirAll(testDataDir, 0o755); err != nil { + t.Fatalf("Failed to create testdata directory: %v", err) + } // Create an empty directory - err := os.MkdirAll("../../../empty_dir", 0o755) + err := os.MkdirAll(filepath.Join(testDataDir, "empty_dir"), 0o755) if err != nil { t.Fatalf("Failed to create empty directory: %v", err) } // Create an invalid SBOM file invalidSBOM := []byte(`{"invalid": "json"}`) - err = os.WriteFile("../../../invalid_sbom.json", invalidSBOM, 0o644) + err = os.WriteFile(filepath.Join(testDataDir, "invalid_sbom.json"), invalidSBOM, 0o644) if err != nil { t.Fatalf("Failed to create invalid SBOM file: %v", err) } // ... rest of the function }Don't forget to update the test cases to use the new paths and add the import:
+import "path/filepath" func TestIngestSBOM(t *testing.T) { tests := []struct { name string sbomPath string want map[uint32]*graph.Node wantErr bool }{ { name: "non-existent file", - sbomPath: "non_existent_file.json", + sbomPath: filepath.Join("testdata", "non_existent_file.json"), wantErr: true, }, // ... update other test cases }cmd/query/custom/custom.go (1)
28-33: Add validation for the output flagWhile the flags are well-structured, consider adding validation for the output flag to ensure it only accepts "table" or "json" values at flag parsing time rather than during execution.
func (o *options) AddFlags(cmd *cobra.Command) { cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "maximum number of results to display") cmd.Flags().BoolVar(&o.showInfo, "show-info", true, "display the info column") cmd.Flags().StringVar(&o.addr, "addr", "http://localhost:8089", "address of the minefield server") - cmd.Flags().StringVar(&o.output, "output", "table", "output format (table or json)") + cmd.Flags().Var(newOutputFormatValue(&o.output, "table"), "output", "output format (table or json)") } +type outputFormat struct { + value *string +} + +func newOutputFormatValue(p *string, defaultValue string) *outputFormat { + *p = defaultValue + return &outputFormat{p} +} + +func (f *outputFormat) String() string { + return *f.value +} + +func (f *outputFormat) Set(s string) error { + if s != "table" && s != "json" { + return fmt.Errorf("must be either 'table' or 'json'") + } + *f.value = s + return nil +} + +func (f *outputFormat) Type() string { + return "string" +}pkg/tools/ingest/vuln_test.go (2)
Line range hint
15-22: Consider completing the storage dependency removalWhile removing the storage parameter from
LoadDataFromPathis a good step towards reducing storage coupling, theSBOM()function still requires the storage parameter. Consider extending the refactoring to make the data flow more consistent.A potential improvement could be to:
- Make
SBOM()storage-independent likeLoadDataFromPath- Move storage operations to a separate layer
- Return the processed data from
SBOM()instead of directly storing itThis would make the code more modular and easier to test.
Line range hint
36-43: Similar storage dependency exists in Vulnerabilities functionThe same architectural inconsistency appears here -
LoadDataFromPathis storage-independent butVulnerabilities()still requires storage.Consider applying the same refactoring pattern suggested above to maintain consistency across the codebase. This would:
- Improve separation of concerns
- Make the code more testable
- Reduce coupling with storage implementation
cmd/server/server.go (2)
38-42: Consider extracting the default storage address into a constant.The default Redis address "localhost:6379" is hardcoded in the AddFlags method. Consider moving it to the constants block for consistency with other defaults.
const ( defaultConcurrency = 10 defaultAddr = "localhost:8089" redisStorageType = "redis" + defaultRedisAddr = "localhost:6379" ) func (o *options) AddFlags(cmd *cobra.Command) { cmd.Flags().Int32Var(&o.concurrency, "concurrency", defaultConcurrency, "Maximum number of concurrent operations for leaderboard operations") cmd.Flags().StringVar(&o.addr, "addr", defaultAddr, "Network address and port for the server (e.g. localhost:8089)") cmd.Flags().StringVar(&o.StorageType, "storage-type", redisStorageType, "Type of storage to use (e.g., redis, sql)") - cmd.Flags().StringVar(&o.StorageAddr, "storage-addr", "localhost:6379", "Address for storage backend") + cmd.Flags().StringVar(&o.StorageAddr, "storage-addr", defaultRedisAddr, "Address for storage backend")
54-59: Add test coverage for storage initialization.The storage initialization code lacks test coverage. Consider adding tests for:
- Successful Redis storage initialization
- Unknown storage type error case
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-58: cmd/server/server.go#L54-L58
Added lines #L54 - L58 were not covered by testspkg/storages/e2e_test.go (1)
23-29: LGTM! Clean separation of concerns.The removal of
redisStorageparameter fromLoadDataFromPathis a good architectural improvement, separating data loading from storage concerns. The loaded data is then properly ingested into storage in a separate step.This change aligns well with the Single Responsibility Principle, making the code more maintainable and testable.
pkg/storages/redis_storage.go (2)
17-24: Improved error handling in Redis client initialization.The changes enhance reliability by adding connection verification and proper error handling. However, the Redis client configuration could be improved.
Consider adding these Redis client options for better reliability and performance:
func NewRedisStorage(addr string) (graph.Storage, error) { rdb := redis.NewClient(&redis.Options{ Addr: addr, + // Add reasonable timeouts + DialTimeout: 5 * time.Second, + ReadTimeout: 3 * time.Second, + WriteTimeout: 3 * time.Second, + // Add connection pool settings + PoolSize: 10, + MinIdleConns: 5, + // Add retry settings + MaxRetries: 3, + MinRetryBackoff: 8 * time.Millisecond, + MaxRetryBackoff: 512 * time.Millisecond, })Don't forget to add
timeto imports:import ( + "time" ... )
The shell script results provide clear evidence about the context usage patterns. Let me generate the final response:
Context handling needs improvement across the Redis storage implementation
The Redis storage implementation consistently uses
context.Background()for all operations, which prevents proper timeout and cancellation handling from callers. This is confirmed by multiple instances inpkg/storages/redis_storage.go:
GenerateID: Usescontext.Background()forIncroperationSaveNode: Usescontext.Background()for multipleSetoperationsGetNode: Usescontext.Background()forGetoperationGetNodesByGlob: Usescontext.Background()forKeysoperation- And many more similar patterns
Consider:
- Modifying the storage interface methods to accept a context parameter
- Propagating the context from the API layer through to the storage layer
- Adding proper timeout handling for Redis operations
🔗 Analysis chain
Line range hint
1-300: Consider improving context handling throughout the Redis operations.Most Redis operations in this file use
context.Background(). Consider accepting a context parameter to allow proper timeout and cancellation handling from the caller.Let's verify the impact of this refactor:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for fx to wire migration patterns and context usage # Check for remaining fx dependencies echo "Checking for remaining fx dependencies..." rg --type go "uber\.go/fx" # Check for wire provider implementations echo "Checking for wire provider implementations..." rg --type go "google\.golang\.org/wire" # Check for context usage patterns in storage implementations echo "Checking context usage patterns in storage implementations..." ast-grep --pattern 'context.Background()'Length of output: 4704
cmd/query/custom/custom_test.go (5)
17-62: Refactor tests to use assertion library for clarityConsider using the
assertfunctions from thetestifypackage to simplify your test assertions inTestOptions_AddFlags. This will make your tests more readable and concise by reducing boilerplate code.Apply this diff to refactor the assertions:
func TestOptions_AddFlags(t *testing.T) { o := &options{} cmd := &cobra.Command{} o.AddFlags(cmd) - // Test "max-output" flag - maxOutputFlag := cmd.Flags().Lookup("max-output") - if maxOutputFlag == nil { - t.Error("Expected 'max-output' flag to be defined") - } else { - if maxOutputFlag.DefValue != "10" { - t.Errorf("Expected default value of 'max-output' to be '10', got '%s'", maxOutputFlag.DefValue) - } - } + maxOutputFlag := cmd.Flags().Lookup("max-output") + assert.NotNil(t, maxOutputFlag, "Expected 'max-output' flag to be defined") + assert.Equal(t, "10", maxOutputFlag.DefValue, "Default value of 'max-output' should be '10'") - // Test "show-info" flag - showInfoFlag := cmd.Flags().Lookup("show-info") - if showInfoFlag == nil { - t.Error("Expected 'show-info' flag to be defined") - } else { - if showInfoFlag.DefValue != "true" { - t.Errorf("Expected default value of 'show-info' to be 'true', got '%s'", showInfoFlag.DefValue) - } - } + showInfoFlag := cmd.Flags().Lookup("show-info") + assert.NotNil(t, showInfoFlag, "Expected 'show-info' flag to be defined") + assert.Equal(t, "true", showInfoFlag.DefValue, "Default value of 'show-info' should be 'true'") // Repeat similar refactoring for other flags ("addr" and "output") }
64-115: Simplify assertions inTestNewCommandusingassertlibrarySimilar to
TestOptions_AddFlags, you can enhance readability inTestNewCommandby utilizing theassertfunctions from thetestifypackage for your assertions.Here's how you can refactor the assertions:
func TestNewCommand(t *testing.T) { cmd := New() - if cmd == nil { - t.Fatal("Expected New() to return a non-nil command") - } + assert.NotNil(t, cmd, "Expected New() to return a non-nil command") // Check the command's basic properties - if cmd.Use != "custom [script]" { - t.Errorf("Expected Use: 'custom [script]', got: '%s'", cmd.Use) - } + assert.Equal(t, "custom [script]", cmd.Use, "Unexpected command 'Use' value") - if cmd.Short != "Execute a custom query script" { - t.Errorf("Expected Short: 'Execute a custom query script', got: '%s'", cmd.Short) - } + assert.Equal(t, "Execute a custom query script", cmd.Short, "Unexpected command 'Short' value") // Repeat similar refactoring for other assertions }
292-294: Remove redundant buffer initialization inTestRunIn the
TestRunfunction, the initial call tocmd.SetOut(new(bytes.Buffer))is unnecessary since you immediately overwrite it by settingcmd.SetOut(outputBuf). Removing the redundant initialization will streamline the code.Apply this diff to remove the redundancy:
cmd := &cobra.Command{} -cmd.SetOut(new(bytes.Buffer)) // Capture output for assertions outputBuf := &bytes.Buffer{} cmd.SetOut(outputBuf)
117-170: Add test case for empty nodes inTestFormatTableConsider adding a test case where the
nodesslice is empty to ensure thatformatTablehandles this situation gracefully without errors.Here's an example of how you could implement this test case:
t.Run("EmptyNodes", func(t *testing.T) { var buf bytes.Buffer err := formatTable(&buf, []*apiv1.Node{}, 10, true) assert.NoError(t, err) output := buf.String() // Check that the output indicates no nodes are available assert.Contains(t, output, "No nodes to display") })
181-315: Enhance test coverage inTestRunby adding edge casesWhile
TestRuncovers multiple scenarios, consider adding additional test cases to cover edge conditions, such as:
- Testing with
maxOutputset to zero to see if the function handles it correctly.- Testing with
showInfoset tofalseand verifying that the "INFO" column is omitted in the output.- Testing with extremely large node sets to assess performance and output correctness.
Example of an additional test case:
{ name: "showInfo false with table output", script: "match (n) return n", output: "table", maxOutput: 10, showInfo: false, mockResponse: &apiv1.QueryResponse{ Nodes: []*apiv1.Node{ {Name: "node1", Type: "type1", Id: 1}, }, }, expectedOutput: []string{ "NAME", "TYPE", "ID", "node1", "type1", "1", }, // Ensure "INFO" column is not present },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/build.yaml(1 hunks)Makefile(2 hunks)api/v1/service_test.go(1 hunks)cmd/ingest/ingest.go(1 hunks)cmd/ingest/osv/osv.go(3 hunks)cmd/ingest/sbom/sbom.go(3 hunks)cmd/ingest/scorecard/scorecard.go(3 hunks)cmd/leaderboard/leaderboard.go(1 hunks)cmd/query/custom/custom.go(1 hunks)cmd/query/custom/custom_test.go(1 hunks)cmd/query/query.go(1 hunks)cmd/root/root.go(1 hunks)cmd/server/server.go(5 hunks)cmd/server/server_test.go(1 hunks)cmd/server/wire.go(1 hunks)cmd/server/wire_gen.go(1 hunks)codecov.yml(0 hunks)go.mod(1 hunks)main.go(1 hunks)pkg/storages/e2e_test.go(1 hunks)pkg/storages/fxmodule.go(0 hunks)pkg/storages/fxmodule_test.go(0 hunks)pkg/storages/redis_storage.go(1 hunks)pkg/tools/ingest/loader.go(5 hunks)pkg/tools/ingest/sbom_test.go(2 hunks)pkg/tools/ingest/vuln_test.go(2 hunks)
💤 Files with no reviewable changes (3)
- codecov.yml
- pkg/storages/fxmodule.go
- pkg/storages/fxmodule_test.go
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yaml
22-22: shellcheck reported issue in this script: SC2086:info:2:24: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 GitHub Check: codecov/patch
cmd/ingest/ingest.go
[warning] 15-15: cmd/ingest/ingest.go#L15
Added line #L15 was not covered by tests
[warning] 23-25: cmd/ingest/ingest.go#L23-L25
Added lines #L23 - L25 were not covered by tests
cmd/ingest/osv/osv.go
[warning] 38-38: cmd/ingest/osv/osv.go#L38
Added line #L38 was not covered by tests
[warning] 57-58: cmd/ingest/osv/osv.go#L57-L58
Added lines #L57 - L58 were not covered by tests
cmd/ingest/sbom/sbom.go
[warning] 39-39: cmd/ingest/sbom/sbom.go#L39
Added line #L39 was not covered by tests
[warning] 60-61: cmd/ingest/sbom/sbom.go#L60-L61
Added lines #L60 - L61 were not covered by tests
cmd/ingest/scorecard/scorecard.go
[warning] 39-39: cmd/ingest/scorecard/scorecard.go#L39
Added line #L39 was not covered by tests
[warning] 60-61: cmd/ingest/scorecard/scorecard.go#L60-L61
Added lines #L60 - L61 were not covered by tests
cmd/leaderboard/leaderboard.go
[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by tests
cmd/query/custom/custom.go
[warning] 45-50: cmd/query/custom/custom.go#L45-L50
Added lines #L45 - L50 were not covered by tests
[warning] 71-72: cmd/query/custom/custom.go#L71-L72
Added lines #L71 - L72 were not covered by tests
cmd/query/query.go
[warning] 10-10: cmd/query/query.go#L10
Added line #L10 was not covered by tests
[warning] 12-14: cmd/query/query.go#L12-L14
Added lines #L12 - L14 were not covered by tests
[warning] 18-19: cmd/query/query.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-22: cmd/query/query.go#L22
Added line #L22 was not covered by tests
cmd/root/root.go
[warning] 20-22: cmd/root/root.go#L20-L22
Added lines #L20 - L22 were not covered by tests
[warning] 25-27: cmd/root/root.go#L25-L27
Added lines #L25 - L27 were not covered by tests
[warning] 29-29: cmd/root/root.go#L29
Added line #L29 was not covered by tests
[warning] 32-38: cmd/root/root.go#L32-L38
Added lines #L32 - L38 were not covered by tests
[warning] 43-47: cmd/root/root.go#L43-L47
Added lines #L43 - L47 were not covered by tests
[warning] 49-49: cmd/root/root.go#L49
Added line #L49 was not covered by tests
cmd/server/server.go
[warning] 44-49: cmd/server/server.go#L44-L49
Added lines #L44 - L49 were not covered by tests
[warning] 54-58: cmd/server/server.go#L54-L58
Added lines #L54 - L58 were not covered by tests
[warning] 62-62: cmd/server/server.go#L62
Added line #L62 was not covered by tests
[warning] 74-74: cmd/server/server.go#L74
Added line #L74 was not covered by tests
[warning] 140-150: cmd/server/server.go#L140-L150
Added lines #L140 - L150 were not covered by tests
cmd/server/wire_gen.go
[warning] 16-25: cmd/server/wire_gen.go#L16-L25
Added lines #L16 - L25 were not covered by tests
[warning] 32-33: cmd/server/wire_gen.go#L32-L33
Added lines #L32 - L33 were not covered by tests
main.go
[warning] 11-15: main.go#L11-L15
Added lines #L11 - L15 were not covered by tests
[warning] 17-19: main.go#L17-L19
Added lines #L17 - L19 were not covered by tests
🔇 Additional comments (37)
main.go (1)
17-19: LGTM! Standard command execution pattern.
This follows the standard pattern for executing cobra commands.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-19: main.go#L17-L19
Added lines #L17 - L19 were not covered by tests
cmd/server/wire.go (4)
1-4: LGTM! Build tags are correctly configured.
The build tags are properly set up for Wire code generation, using both the new-style //go:build and legacy-style // +build tags for maximum compatibility.
6-10: LGTM! Imports are clean and necessary.
All imports are relevant for the Wire-based dependency injection implementation.
1-24: Verify complete migration from fx to Wire.
Let's ensure the migration from fx to Wire is complete and consistent.
#!/bin/bash
# Description: Verify complete migration from fx to Wire
# Expected: No remaining fx dependencies, all necessary Wire providers present
# Check for any remaining fx imports
rg "go.uber.org/fx" --type go
# Check for any remaining fx usage
rg "fx\." --type go
# Look for other Wire provider functions that might need updates
rg "wire\.Build\(" --type go
# Check for any TODO/FIXME comments related to DI
rg -i "todo.*(\bdi\b|\bdependency injection\b|\bfx\b)" --type go
rg -i "fixme.*(\bdi\b|\bdependency injection\b|\bfx\b)" --type go14-20: Add documentation for the Wire provider setup.
While the Wire setup is correct, adding documentation would help explain the dependency injection flow.
Consider adding a doc comment like:
+// InitializeServerCommand is the Wire injection entry point for the server command.
+// The actual implementation will be generated by Wire, connecting the storage provider
+// with the command initialization.
func InitializeServerCommand(o *options) (*cobra.Command, error) {✅ Verification successful
Add documentation for the Wire provider setup.
While the Wire setup is correct and all necessary components are in place (NewServerCommand, ProvideStorage, and wire.go), adding documentation would improve code clarity.
+// InitializeServerCommand is the Wire injection entry point for the server command.
+// It connects the storage provider with the server command initialization.
+// The actual implementation will be generated by Wire in wire_gen.go.
func InitializeServerCommand(o *options) (*cobra.Command, error) {🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Wire setup completeness
# Expected: Find all necessary Wire provider functions
# Check for other Wire provider functions
rg "wire\.Build\(" --type go -A 5 -B 5
# Verify NewServerCommand exists
ast-grep --pattern 'func NewServerCommand($_, $_) ($_, $_)'
Length of output: 249
Script:
#!/bin/bash
# Let's verify the Wire setup more thoroughly
# Check if NewServerCommand exists in the codebase
rg "func NewServerCommand" --type go -A 5
# Look for ProvideStorage function
rg "func ProvideStorage" --type go -A 5
# Check for any Wire-related imports
rg "github.com/google/wire" --type go
# Look for wire.go files which are commonly used for Wire setups
fd "wire.go$"
Length of output: 999
cmd/ingest/ingest.go (1)
15-15: Verify Wire provider setup for storage dependency
The removal of the storage parameter aligns with the migration to Wire for dependency injection. However, we should ensure the Wire provider is properly configured.
✅ Verification successful
Wire dependency injection is properly configured for storage
The storage dependency is correctly handled through Wire's dependency injection:
- Storage provider is defined in
cmd/server/wire.goviaProvideStorage - Wire build directive properly includes the storage provider
- Storage is correctly injected into commands and services that need it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Wire provider configuration for storage dependency
# Look for Wire provider definitions
ast-grep --pattern 'func Provide$$$Storage($$$) $$$Storage {
$$$
}'
# Check for Wire build tags
rg -l '//\+build wireinject'
# Look for wire.go files that might contain the provider setup
fd -g '*wire.go'
Length of output: 140
Script:
#!/bin/bash
# Check the wire.go file content to verify storage provider setup
cat cmd/server/wire.go
# Look for storage-related providers and injectors in the codebase
rg -A 5 "ProvideStorage|graph.Storage"
# Check for any wire build directives
rg "wire.Build" -A 5
Length of output: 5672
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-15: cmd/ingest/ingest.go#L15
Added line #L15 was not covered by tests
cmd/query/query.go (2)
12-14: LGTM! Well-documented command descriptions
The added Long description provides clear and comprehensive information about the command's purpose, following cobra best practices.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-14: cmd/query/query.go#L12-L14
Added lines #L12 - L14 were not covered by tests
18-22: Verify consistent refactoring across all subcommands
Let's ensure all subcommand New() functions have been consistently refactored to remove the storage parameter.
✅ Verification successful
Refactoring verification complete - all subcommands are consistent
The verification shows that all subcommand New() functions (custom, getMetadata, and globsearch) have been consistently refactored to remove the storage parameter. None of the implementations accept any parameters, and there are no remaining imports of graph.Storage in any of the query command files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any subcommand New() functions still use the storage parameter
# Search for New functions that still accept storage parameter
ast-grep --pattern 'func New(storage graph.Storage)' cmd/query/
# Also check imports to ensure no unnecessary graph.Storage imports remain
rg -l "graph\.Storage" cmd/query/
Length of output: 101
Script:
#!/bin/bash
# Let's check the implementation of New() functions in all subcommands
# First, find all subcommand directories
fd -t d . cmd/query/
# Then check the implementation of New() functions
ast-grep --pattern 'func New() $_' cmd/query/
# Also check all imports in these files to understand the dependencies
rg "^import \(" -A 10 cmd/query/
Length of output: 7569
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-19: cmd/query/query.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-22: cmd/query/query.go#L22
Added line #L22 was not covered by tests
Makefile (2)
5-5: LGTM: Correct dependency order
The build target correctly depends on wire, ensuring the dependency injection code is generated before compilation.
Line range hint 1-48: Verify wire integration in the CI pipeline
Ensure that the wire tool is properly installed in the CI environment before the build step.
✅ Verification successful
Since I can see that the wire tool is properly installed in the CI pipeline through the GitHub Actions workflow (.github/workflows/build.yaml), with the specific installation command go install github.com/google/wire/cmd/wire@latest, I can now generate the final response.
Wire tool is properly integrated in CI pipeline ✅
The wire tool installation is correctly handled in the CI environment through the GitHub Actions workflow at .github/workflows/build.yaml using the command go install github.com/google/wire/cmd/wire@latest.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if wire installation is handled in CI workflow
# Look for wire installation steps in GitHub Actions workflow
rg -l 'wire' .github/workflows/
rg 'go install.*wire' .github/workflows/
Length of output: 192
cmd/root/root.go (1)
43-47: LGTM! Successful migration to Wire dependency injection
The removal of the storage parameter from subcommands aligns well with the PR objective of migrating to Wire for dependency injection. This change:
- Reduces coupling between commands
- Makes the dependency graph more explicit
- Allows Wire to manage the object lifecycle
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-47: cmd/root/root.go#L43-L47
Added lines #L43 - L47 were not covered by tests
.github/workflows/build.yaml (1)
21-24: Verify Wire installation in the workflow
Let's ensure the Wire installation step is working correctly and the tool is accessible.
✅ Verification successful
Wire installation is correctly configured and actively used in the project
The verification confirms that Wire is actively used in the codebase with:
- Wire provider definitions in
cmd/server/wire.go - Generated Wire code in
cmd/server/wire_gen.go
The installation step in the workflow is necessary and properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Wire is being used in the codebase
# Expected: Find Wire provider definitions and generated code
# Look for Wire provider definitions
echo "Checking for Wire provider definitions..."
rg -l 'wire\.NewSet|wire\.Build|wire\.Bind'
# Look for Wire generated files
echo "Checking for Wire generated files..."
fd -g '*wire_gen.go'
# Look for Wire build tags
echo "Checking for Wire build tags..."
rg -l '//\+build wireinject'
Length of output: 390
🧰 Tools
🪛 actionlint
22-22: shellcheck reported issue in this script: SC2086:info:2:24: Double quote to prevent globbing and word splitting
(shellcheck)
cmd/ingest/sbom/sbom.go (1)
17-17: LGTM! Clean removal of storage dependency
The removal of the storage field from the options struct aligns well with the migration to Wire for dependency injection, reducing coupling in the process.
go.mod (1)
14-14: Verify complete migration from fx to wire
The addition of github.com/google/wire and removal of fx-related packages aligns with the PR objective. Let's verify the complete migration and proper setup.
cmd/ingest/scorecard/scorecard.go (4)
9-9: LGTM! Import alias change improves clarity
The change from v1 to apiv1 makes the API version more explicit and aligns with the package name.
17-17: LGTM! Simplified options struct
The removal of the storage field aligns with the PR objective to use Wire for dependency injection, making the struct more focused.
60-61: LGTM! Constructor simplified for Wire integration
The removal of the storage parameter aligns with using Wire for dependency injection, making the constructor simpler and more maintainable.
#!/bin/bash
# Check for existing constructor tests
fd -e go -g '*_test.go' --exec grep -l "New()" {}Would you like me to help create test cases for the modified constructor?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: cmd/ingest/scorecard/scorecard.go#L60-L61
Added lines #L60 - L61 were not covered by tests
39-39: Add test coverage for LoadDataFromPath call
This line handles critical data ingestion functionality but lacks test coverage. Please add tests to ensure the modified LoadDataFromPath call works correctly without the storage parameter.
Would you like me to help create test cases for this functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/scorecard/scorecard.go#L39
Added line #L39 was not covered by tests
cmd/ingest/osv/osv.go (1)
17-17: LGTM: Clean struct definition
The removal of the storage field aligns with the PR's objective to refactor dependency injection, resulting in a more focused and decoupled structure.
pkg/tools/ingest/loader.go (3)
19-19: LGTM! Good separation of concerns
The removal of the storage parameter improves the function's single responsibility and reduces coupling. This change aligns well with the PR's objective of refactoring dependency management.
71-71: LGTM! Clean implementation
The changes to processZipFile are well-implemented:
- Proper resource cleanup with deferred calls
- Thorough error handling with good context
- Consistent with the refactoring to remove storage dependency
Also applies to: 103-103
19-19: Verify all callers have been updated
Let's ensure all callers of LoadDataFromPath have been updated to match the new signature.
✅ Verification successful
Based on the search results, I can now generate the final response:
All callers have been properly updated
All 9 calls to LoadDataFromPath across the codebase are using the new signature with just the path parameter. No instances were found of calls still trying to pass a storage parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any calls to LoadDataFromPath that might still be passing a storage parameter
rg -p "LoadDataFromPath\([^)]*storage"
# Find all current calls to LoadDataFromPath for manual verification
rg -p "LoadDataFromPath\("
Length of output: 943
pkg/tools/ingest/sbom_test.go (1)
39-39: Verify consistency of LoadDataFromPath usage across the codebase
The removal of the storage parameter from LoadDataFromPath aligns with the PR's objective to refactor dependency injection. Let's verify this change is consistent across all callers.
✅ Verification successful
LoadDataFromPath refactoring is consistent across the codebase
The verification confirms that the LoadDataFromPath function is correctly defined with only a path parameter in pkg/tools/ingest/loader.go, and all its usages across the codebase (in test files, cmd files, and API files) consistently use the function with just the path parameter. No instances of the old signature with a storage parameter were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any remaining calls to LoadDataFromPath with a storage parameter
# This helps ensure the refactoring was complete across the codebase
# Search for any calls to LoadDataFromPath that might still include storage parameter
rg -p "LoadDataFromPath\([^)]*storage"
# Also check the function definition to confirm parameter list
ast-grep --pattern 'func LoadDataFromPath($_) {
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's try a different approach to verify the LoadDataFromPath usage
# First, find all occurrences of LoadDataFromPath function definition
rg -A 5 "^func LoadDataFromPath"
# Then find all calls to LoadDataFromPath
rg -A 2 "LoadDataFromPath\("
# Also check for any files importing the package that contains LoadDataFromPath
rg "\".*ingest\""
Length of output: 3561
cmd/server/server_test.go (2)
93-93: LGTM! Change aligns with Wire migration.
The removal of the storage parameter from New() is consistent with the PR's objective to migrate from fx to Wire for dependency injection.
93-93: Verify Wire-based storage initialization.
Since the storage parameter has been removed from New(), let's verify the new Wire-based initialization.
✅ Verification successful
Let me check the content of the wire files to verify the storage initialization.
Let me check how the storage is provided through the options struct.
Wire-based storage initialization is properly implemented
The storage initialization has been correctly moved to Wire-based dependency injection:
ProvideStoragefunction is defined in wire.go and properly injected- Storage configuration is handled through command-line flags (
--storage-typeand--storage-addr) - The options struct implements
ProvideStorage()method that creates the appropriate storage based on the configuration - Wire generates the initialization code in wire_gen.go that connects all dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for Wire provider set that includes storage initialization
# Search for Wire provider set in server package
ast-grep --pattern 'func provideStorage($$$) $$$'
# Search for Wire build directive for server command
ast-grep --pattern 'wire.Build($$$)'
# Look for wire.go file in server package
fd -g 'wire*.go' cmd/server/
Length of output: 163
Script:
#!/bin/bash
# Check wire.go and wire_gen.go content
cat cmd/server/wire.go cmd/server/wire_gen.go
# Also look for storage-related provider functions in server.go
ast-grep --pattern 'func provide$_Storage($$$) $$$'
# Look for storage initialization in New function
rg -A 5 "func New\(" cmd/server/
Length of output: 1550
Script:
#!/bin/bash
# Look for options struct and ProvideStorage method
rg -A 10 "type options struct" cmd/server/
rg -A 10 "func.*ProvideStorage" cmd/server/
# Check how options are configured
rg "o\." cmd/server/
Length of output: 2874
cmd/query/custom/custom.go (5)
18-25: LGTM! Clean struct refactoring
The options struct has been effectively refactored to support the new architecture, removing storage dependencies and adding necessary fields for client-server communication. The queryServiceClient field enables proper dependency injection for testing.
82-116: LGTM! Well-structured table formatting
The table formatting implementation is clean, efficient, and handles all edge cases properly. Good use of the tablewriter package and consistent handling of the maxOutput limit.
Line range hint 118-134: LGTM! Clean command setup
The command setup is well-structured with clear documentation and proper argument validation. The removal of the storage parameter aligns well with the PR's objective of refactoring away from fx.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-50: cmd/query/custom/custom.go#L45-L50
Added lines #L45 - L50 were not covered by tests
[warning] 71-72: cmd/query/custom/custom.go#L71-L72
Added lines #L71 - L72 were not covered by tests
43-50: Add test coverage for client initialization
The client initialization code path is currently not covered by tests. Consider adding test cases for:
- Client initialization with custom options
- Connection failures
- Timeout scenarios
#!/bin/bash
# Check existing test coverage for client initialization
rg -l "TestRun.*" . | xargs rg "queryServiceClient.*nil"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-50: cmd/query/custom/custom.go#L45-L50
Added lines #L45 - L50 were not covered by tests
67-79: Add test coverage for JSON formatting errors
The error handling for JSON formatting (lines 70-72) is not covered by tests. Consider adding test cases with malformed node data that would trigger JSON marshaling errors.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-72: cmd/query/custom/custom.go#L71-L72
Added lines #L71 - L72 were not covered by tests
pkg/tools/ingest/vuln_test.go (1)
Line range hint 15-43: Verify error handling for LoadDataFromPath
Since LoadDataFromPath was modified to remove the storage parameter, let's verify that error cases are still properly tested.
✅ Verification successful
Error handling for LoadDataFromPath is properly implemented and tested
The verification shows that error handling is comprehensive:
- The function handles various error cases including:
- Invalid path access
- Directory read failures
- JSON file read failures
- ZIP file processing errors
- Test coverage exists in
sbom_test.gowith explicit error checking viatest.wantErr - The removal of storage parameter hasn't impacted error handling as it wasn't involved in the path loading logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering LoadDataFromPath errors
rg -l "LoadDataFromPath.*error" --type go
# Look for existing error test cases
ast-grep --pattern 'func Test.*LoadDataFromPath.*\{
$$$
_, err := LoadDataFromPath($_)
$$$
if err != nil {
$$$
}
}'
Length of output: 202
Script:
#!/bin/bash
# Let's check the actual test file content to see error handling
rg -A 5 "LoadDataFromPath.*error" pkg/tools/ingest/loader.go
# Check test file for error cases
rg -A 10 "func Test.*LoadDataFromPath" pkg/tools/ingest/vuln_test.go
# Look for any other test files that might test LoadDataFromPath
fd ".*_test.go" | xargs rg "LoadDataFromPath"
Length of output: 834
Script:
#!/bin/bash
# Let's check the test files that use LoadDataFromPath for error cases
rg -B 5 -A 10 "LoadDataFromPath" pkg/tools/ingest/sbom_test.go
rg -B 5 -A 10 "LoadDataFromPath" pkg/tools/ingest/vuln_test.go
# Check if there are any negative test cases
rg -A 10 "func Test.*LoadDataFromPath.*Error" .
rg -A 10 "func Test.*LoadDataFromPath.*Invalid" .
# Look at the actual implementation to understand error handling
cat pkg/tools/ingest/loader.go
Length of output: 4297
cmd/server/server.go (4)
27-35: LGTM! Well-structured options and constants.
The new storage-related options and default constants are well-organized and provide good configuration flexibility.
62-62: LGTM! Improved error handling and constant usage.
Good improvements:
- Better error context with wrapping
- Consistent use of defaultAddr constant
Also applies to: 74-74
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: cmd/server/server.go#L62
Added line #L62 was not covered by tests
Line range hint 127-151: Consider consolidating command creation functions.
The codebase now has two ways to create server commands:
New()- Creates command with storage initialized via flagsNewServerCommand()- Accepts pre-initialized storage
This might lead to confusion. Consider:
- Documenting the intended use case for each function
- Deprecating one if they serve the same purpose
- Adding test coverage for NewServerCommand
Let's check if both functions are needed:
#!/bin/bash
# Search for usages of both functions
echo "Searching for New() usage:"
rg "server\.New\(\)" --type go
echo "Searching for NewServerCommand() usage:"
rg "server\.NewServerCommand\(" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-150: cmd/server/server.go#L140-L150
Added lines #L140 - L150 were not covered by tests
44-50: LGTM! Well-implemented storage provider.
The storage provider implementation follows good practices:
- Clear error messages
- Extensible switch statement for future storage types
- Proper error wrapping
However, let's verify if there are any other storage types planned:
✅ Verification successful
Storage implementation is well-designed and currently Redis-focused
The verification shows:
- A well-defined Storage interface in
pkg/graph/storage.gowith comprehensive methods - Redis is currently the only concrete implementation (
pkg/storages/redis_storage.go) - No TODOs or immediate plans for other storage types
- The switch statement design allows for future extensibility when needed
The review comment's approval is appropriate, as the implementation is clean and follows good practices while being focused on the current Redis-only requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other storage implementations
rg -t go "type.*Storage.*interface" pkg/
Length of output: 279
Script:
#!/bin/bash
# Let's try different approaches to find storage implementations
# Search for storage interface definition
rg -t go "interface.*Storage" -A 5
# Search for storage type implementations
rg -t go "type.*Storage.*struct"
# Search for storage-related files
fd -e go storage
# Search for NewRedisStorage usage and definition
rg -t go "NewRedisStorage"
Length of output: 520
Script:
#!/bin/bash
# Let's check the storage interface definition and any other storage types
cat pkg/graph/storage.go
# Also check if there are any TODOs or comments about future storage types
rg -t go "TODO.*storage"
Length of output: 857
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-49: cmd/server/server.go#L44-L49
Added lines #L44 - L49 were not covered by tests
pkg/storages/e2e_test.go (1)
Line range hint 30-36: LGTM! Consistent refactoring pattern.
The changes maintain consistency with the previous segment, following the same pattern of separating data loading from storage operations.
Let's verify that this refactoring pattern is consistent across the codebase:
✅ Verification successful
Refactoring pattern is consistently implemented across the codebase
The verification confirms that:
LoadDataFromPathis consistently used to load data without storage operations- Storage operations are properly separated using
ingest.SBOMandingest.Vulnerabilitiesfunctions - This pattern is followed in all relevant parts of the codebase:
- Test files (pkg/storages/e2e_test.go, pkg/tools/ingest/*_test.go)
- Command files (cmd/ingest/*)
- API service (api/v1/service.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that LoadDataFromPath is consistently called without storage parameter
# and that SBOM/Vulnerabilities functions are used for storage operations
# Check for any remaining direct storage operations in LoadDataFromPath
rg -A 5 "LoadDataFromPath.*Storage"
# Verify the pattern of separate load and ingest steps
ast-grep --pattern 'LoadDataFromPath($_)
$$$
ingest.(SBOM|Vulnerabilities)($_, $_)'
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to verify the refactoring pattern
# First, let's find all usages of LoadDataFromPath
rg "LoadDataFromPath" -B 2 -A 5
# Then check for any direct storage operations in the ingest package
fd "ingest.*\.go" --exec rg "LoadDataFromPath" -B 2 -A 5 {}
# Also check the pattern of storage operations
rg "ingest\.(SBOM|Vulnerabilities)" -B 2 -A 5
Length of output: 7502
api/v1/service_test.go (1)
76-76: Let me gather more information about the LoadDataFromPath usage to ensure the refactoring is complete and consistent.
#!/bin/bash
# Get the actual implementation and usage of LoadDataFromPath
ast-grep --pattern 'func LoadDataFromPath($$$) {
$$$
}'
# Get the actual call sites with context
rg "LoadDataFromPath" -B 2 -A 2 --type goSigned-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
cmd/ingest/sbom/sbom_test.go (1)
7-29: Basic test structure is good, but could be enhancedThe test covers essential command properties with clear assertions. However, consider adding:
- Setup and cleanup
- More comprehensive Args validation
- Actual RunE behavior verification
Here's a suggested enhancement:
func TestNew(t *testing.T) { + // Setup + t.Parallel() + cmd := New() + + t.Run("command properties", func(t *testing.T) { if cmd.Use != "sbom [path to sbom file/dir]" { - t.Errorf("expected Use to be 'sbom [path to sbom file/dir]', got %s", cmd.Use) + t.Fatalf("Use mismatch:\nwant: 'sbom [path to sbom file/dir]'\ngot: %q", cmd.Use) } if cmd.Short != "Ingest an sbom into the graph " { - t.Errorf("expected Short to be 'Ingest an sbom into the graph ', got %s", cmd.Short) + t.Fatalf("Short description mismatch:\nwant: 'Ingest an sbom into the graph '\ngot: %q", cmd.Short) } - if cmd.Args == nil || cmd.Args(nil, []string{"arg1"}) != nil { - t.Errorf("expected Args to be cobra.ExactArgs(1)") - } + t.Run("args validation", func(t *testing.T) { + tests := []struct { + name string + args []string + wantErr bool + }{ + {"no args", []string{}, true}, + {"one arg", []string{"file.json"}, false}, + {"too many args", []string{"file1.json", "file2.json"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := cmd.Args(nil, tt.args) + if (err != nil) != tt.wantErr { + t.Errorf("Args() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } + }) if cmd.DisableAutoGenTag != true { - t.Errorf("expected DisableAutoGenTag to be true") + t.Fatal("DisableAutoGenTag should be true") } - if cmd.RunE == nil { - t.Errorf("expected RunE to be set") - } + t.Run("RunE execution", func(t *testing.T) { + if cmd.RunE == nil { + t.Fatal("RunE should not be nil") + } + + // Test actual RunE behavior + err := cmd.RunE(cmd, []string{"testdata/sample.json"}) + if err != nil { + t.Fatalf("RunE failed: %v", err) + } + }) + }) }cmd/ingest/scorecard/scorecard_test.go (2)
18-20: Consider enhancing Args validation test coverageThe current Args validation could be more comprehensive and clearer.
Consider this improved version:
- if cmd.Args == nil || cmd.Args(nil, []string{"arg1"}) != nil { - t.Errorf("expected Args to be cobra.ExactArgs(1)") - } + testCases := []struct { + name string + args []string + wantErr bool + }{ + {"valid args", []string{"arg1"}, false}, + {"no args", []string{}, true}, + {"too many args", []string{"arg1", "arg2"}, true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := cmd.Args(nil, tc.args) + if (err != nil) != tc.wantErr { + t.Errorf("Args() error = %v, wantErr %v", err, tc.wantErr) + } + }) + }
22-24: LGTM with minor suggestionThe test is correct, but the error message could be more descriptive.
- t.Errorf("expected DisableAutoGenTag to be true") + t.Errorf("expected DisableAutoGenTag to be true, got %v", cmd.DisableAutoGenTag)Makefile (1)
37-38: Consider adding version compatibility checkWhile the installation check is good, consider adding a version check to ensure compatibility:
check-wire: - @command -v wire >/dev/null 2>&1 || { echo >&2 "wire is not installed. Please install wire. go install github.com/google/wire/cmd/wire@latest"; exit 1; } + @command -v wire >/dev/null 2>&1 || { echo >&2 "wire is not installed. Please install wire. go install github.com/google/wire/cmd/wire@latest"; exit 1; } + @wire version | grep -q "^wire" || { echo >&2 "wire version check failed. Please ensure you have a compatible version."; exit 1; }cmd/ingest/ingest_test.go (1)
9-28: Consider enhancing test coverage and maintainability.The test effectively validates basic command properties and subcommands. However, consider these improvements:
- Extract the expected subcommands slice as a package-level constant for reusability
- Add validation for other important command properties (e.g., RunE, PreRunE)
- Consider testing error cases
Here's a suggested refactor:
+const ( + cmdUse = "ingest" + cmdShort = "ingest metadata into the graph" +) + +var expectedSubcommands = []string{ + "osv [path to vulnerability file/dir]", + "sbom [path to sbom file/dir]", + "scorecard [path to scorecard file/dir]", +} + func TestNewCommand(t *testing.T) { cmd := New() assert.NotNil(t, cmd, "Ingest command should not be nil") - assert.Equal(t, "ingest", cmd.Use, "Command 'Use' should be 'ingest'") - assert.Equal(t, "ingest metadata into the graph", cmd.Short, "Command 'Short' description should match") + assert.Equal(t, cmdUse, cmd.Use, "Command 'Use' should match") + assert.Equal(t, cmdShort, cmd.Short, "Command 'Short' description should match") + assert.NotNil(t, cmd.RunE, "Command should have a RunE function") + assert.NotNil(t, cmd.PreRunE, "Command should have a PreRunE function") subcommands := cmd.Commands() subcommandUses := []string{} for _, subcmd := range subcommands { subcommandUses = append(subcommandUses, subcmd.Use) } - expectedSubcommands := []string{ - "osv [path to vulnerability file/dir]", - "sbom [path to sbom file/dir]", - "scorecard [path to scorecard file/dir]", - } assert.ElementsMatch(t, expectedSubcommands, subcommandUses, "Subcommands should match expected list") }.github/workflows/grype.yaml (2)
Line range hint
2-4: Consider optimizing workflow triggersThe removal of path filters means this workflow will run on all PR changes, not just Docker-related ones. While this ensures comprehensive security scanning, it might increase CI usage unnecessarily.
Consider adding path filters if you want to run scans only when Docker-related files change:
on: pull_request: branches: [ main ] paths: - 'Dockerfile' - 'docker-compose*.yml' - '.docker/**' - '.github/workflows/grype.yaml'
52-52: Add newline at end of fileAdd a newline character at the end of the file to comply with POSIX standards.
sarif_file: results.sarif +🧰 Tools
🪛 yamllint
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
cmd/ingest/osv/osv_test.go (1)
10-23: Consider adding more test cases for better coverage.While the table-driven test pattern is a good choice, having only one test case might not provide sufficient coverage. Consider adding test cases for:
- Edge cases (e.g., invalid paths)
- Different flag configurations
- Error scenarios
Also, the test case name "creates command with correct configuration" could be more specific about what configuration is being tested.
Example additional test cases:
tests := []struct { name string wantUse string wantShort string wantFlagCount int }{ { - name: "creates command with correct configuration", + name: "default configuration with standard flags", wantUse: "osv [path to vulnerability file/dir]", wantShort: "Graph vulnerability data into the graph, and connect it to existing library nodes", wantFlagCount: 1, }, + { + name: "configuration with custom addr flag", + wantUse: "osv [path to vulnerability file/dir]", + wantShort: "Graph vulnerability data into the graph, and connect it to existing library nodes", + wantFlagCount: 1, + }, }cmd/root/root.go (2)
16-23: LGTM! Consider adding unit tests.The Options struct and AddFlags method are well-designed with appropriate visibility and clear flag descriptions. However, static analysis indicates these changes aren't covered by tests.
Would you like me to help generate unit tests for the Options struct and AddFlags method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-23: cmd/root/root.go#L21-L23
Added lines #L21 - L23 were not covered by tests
33-49: Consider enhancing pprof server robustness.While the implementation is solid, consider these improvements:
- Add timeout to shutdown context
- Use structured logging instead of fmt.Printf
PersistentPostRun = func(cmd *cobra.Command, args []string) { - if err := srv.Shutdown(context.Background()); err != nil { - fmt.Printf("pprof server shutdown error: %v\n", err) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := srv.Shutdown(ctx); err != nil { + log.Printf("pprof server shutdown error: %v", err) } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by testsgo.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.1specified in the go.mod file is invalid. The latest stable version of Go is 1.22.x.Please update to a valid Go version:
-go 1.23.1 +go 1.22.1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/build.yaml(1 hunks).github/workflows/grype.yaml(2 hunks)Makefile(2 hunks)cmd/ingest/ingest_test.go(1 hunks)cmd/ingest/osv/osv_test.go(1 hunks)cmd/ingest/sbom/sbom_test.go(1 hunks)cmd/ingest/scorecard/scorecard_test.go(1 hunks)cmd/root/root.go(1 hunks)cmd/server/wire.go(1 hunks)cmd/server/wire_gen.go(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yaml
- cmd/server/wire.go
🧰 Additional context used
📓 Learnings (1)
cmd/root/root.go (1)
Learnt from: naveensrinivasan
PR: bitbomdev/minefield#133
File: cmd/root/root.go:20-22
Timestamp: 2024-11-15T13:47:35.287Z
Learning: In the `cmd/root/root.go` file of the Go codebase, the pprof server is not enabled by default; it starts only when the `--pprof` flag is provided. Therefore, unless explicitly enabled by the user, the pprof server will not run.
🪛 yamllint
.github/workflows/grype.yaml
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: codecov/patch
cmd/root/root.go
[warning] 21-23: cmd/root/root.go#L21-L23
Added lines #L21 - L23 were not covered by tests
[warning] 26-28: cmd/root/root.go#L26-L28
Added lines #L26 - L28 were not covered by tests
[warning] 30-30: cmd/root/root.go#L30
Added line #L30 was not covered by tests
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 52-56: cmd/root/root.go#L52-L56
Added lines #L52 - L56 were not covered by tests
[warning] 58-58: cmd/root/root.go#L58
Added line #L58 was not covered by tests
cmd/server/wire_gen.go
[warning] 16-25: cmd/server/wire_gen.go#L16-L25
Added lines #L16 - L25 were not covered by tests
[warning] 30-31: cmd/server/wire_gen.go#L30-L31
Added lines #L30 - L31 were not covered by tests
🔇 Additional comments (20)
cmd/server/wire_gen.go (2)
1-12: LGTM! Ensure wire generation is properly configured in the build process.
The Wire build constraints and generation directives are correctly configured. Since this is a generated file, ensure that the wire generation step is properly integrated into your build process.
Let's verify the Wire setup:
✅ Verification successful
Wire setup is properly configured and integrated in the build process
The verification confirms:
- Wire source file exists at
cmd/server/wire.go - Wire tool is installed in the GitHub workflow via
go install github.com/google/wire/cmd/wire@latest - The generated code correctly implements the Storage interface from
pkg/graph/storage.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Wire tool installation and generation setup
# Check if wire.go exists (source file for generation)
fd -t f "wire.go$" cmd/server
# Check if wire generation is included in Makefile
rg "wire" Makefile
# Check if wire is installed in the GitHub workflow
rg "wire" .github/workflows/
Length of output: 2160
30-32: Verify Storage interface implementation.
The storage provision looks straightforward, but let's ensure the implementation satisfies the graph.Storage interface completely.
✅ Verification successful
Let me verify the Redis storage implementation to ensure it satisfies the Storage interface.
Storage interface implementation is complete and correct
The Redis storage implementation in pkg/storages/redis_storage.go correctly implements all 17 methods required by the graph.Storage interface:
- NameToID
- SaveNode
- GetNode
- GetNodes
- GetNodesByGlob
- GetAllKeys
- SaveCache
- SaveCaches
- RemoveAllCaches
- ToBeCached
- AddNodeToCachedStack
- GetCache
- GetCaches
- ClearCacheStack
- GenerateID
- GetCustomData
- AddOrUpdateCustomData
The wire provider correctly returns this implementation when Redis storage type is selected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Storage interface implementation
# Find Storage interface definition
ast-grep --pattern 'type Storage interface {
$$$
}'
# Find ProvideStorage implementation in options
rg -A 5 "func.*ProvideStorage" --type go
Length of output: 2160
Script:
#!/bin/bash
# Find Redis storage implementation
ast-grep --pattern 'type RedisStorage struct {
$$$
}'
# Find method implementations in Redis storage
rg -A 2 'func \(.*RedisStorage\)' --type go
Length of output: 4347
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-31: cmd/server/wire_gen.go#L30-L31
Added lines #L30 - L31 were not covered by tests
cmd/ingest/sbom/sbom_test.go (2)
1-6: LGTM! Clean and minimal imports
The package declaration and imports are appropriate for a test file.
8-8: Verify wire integration and fx removal
Since this PR involves migrating from fx to wire, we should verify the proper setup.
cmd/ingest/scorecard/scorecard_test.go (4)
1-5: LGTM!
The package declaration and imports are clean and appropriate for the test file.
10-12: LGTM!
Good error message that includes both expected and actual values for easy debugging.
7-9: Verify error handling for New() function
Let's verify if the New() function can return an error that should be handled in the test.
26-28: Consider adding RunE behavior tests
While checking for nil is good, consider adding tests for the actual behavior of the RunE function.
Let's check if there are other tests covering RunE behavior:
Makefile (4)
3-3: LGTM: Correct dependency chain
The build target correctly depends on the wire target, ensuring generated code is available before compilation.
43-43: LGTM: Comprehensive target chain
The all target includes all necessary dependencies in a logical order.
45-45: Previous review comment addressed
The wire target has been correctly added to .PHONY as suggested in the previous review.
Line range hint 3-45: Verify wire integration completeness
Let's verify that all necessary wire-related files and configurations are in place:
✅ Verification successful
Wire integration is properly configured and complete
The verification confirms that all necessary wire components are in place:
- Wire configuration file at
cmd/server/wire.go - Generated wire file at
cmd/server/wire_gen.go - Provider function
ProvideStorageis properly defined - Wire dependency is correctly specified in
go.mod - Makefile includes appropriate wire-related targets (
wire,check-wire)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify wire integration completeness
# Check for wire.go files
echo "Checking for wire.go files:"
fd wire.go
# Check for wire_gen.go files (generated files)
echo -e "\nChecking for wire_gen.go files:"
fd wire_gen.go
# Check for wire provider definitions
echo -e "\nChecking for wire provider patterns:"
rg "func Provide|wire.Build" -A 3
# Verify go.mod has wire dependency
echo -e "\nVerifying wire dependency in go.mod:"
rg "google/wire" go.mod
Length of output: 918
cmd/ingest/ingest_test.go (2)
1-8: LGTM! Package and imports are appropriate.
The test file uses standard testing packages and the widely-adopted testify/assert package for assertions.
30-33: 🛠️ Refactor suggestion
Enhance wire dependency resolution testing.
The current test only verifies command initialization but doesn't actually test the wire dependency resolution. Consider:
- Testing actual dependency injection
- Verifying that all required dependencies are properly wired
- Adding error cases (e.g., missing required dependencies)
Here's a suggested enhancement:
func TestWireDependencyResolution(t *testing.T) {
- cmd := New()
- assert.NotNil(t, cmd, "Ingest command should initialize without error")
+ t.Run("successful initialization", func(t *testing.T) {
+ cmd := New()
+ assert.NotNil(t, cmd, "Ingest command should initialize without error")
+
+ // Verify that required dependencies are properly injected
+ assert.NotNil(t, cmd.RunE, "RunE function should be injected")
+ assert.NotNil(t, cmd.PreRunE, "PreRunE function should be injected")
+ })
+
+ // Add more test cases for dependency verification
+ // Example: t.Run("verify storage dependency", func(t *testing.T) { ... })
}Let's verify the wire setup in the codebase:
.github/workflows/grype.yaml (1)
16-26: LGTM! Well-structured notification step
The notification implementation follows best practices:
- Uses the latest stable version of github-script
- Properly handles failure conditions
- Provides clear error messaging
cmd/ingest/osv/osv_test.go (3)
1-8: LGTM! Good choice of testing packages.
The package declaration is correct, and the imports are appropriate for the test requirements. Using testify/assert will make the test assertions more readable and maintainable.
25-42: LGTM! Well-structured test implementation.
The test implementation is thorough and well-organized:
- Good use of subtests for better error reporting
- Comprehensive verification of command properties
- Proper flag count verification using VisitAll
43-48: Verify DefaultAddr constant availability.
The test references DefaultAddr but this constant isn't defined in the test file. Ensure it's properly imported or defined.
✅ Verification successful
DefaultAddr constant is properly defined
The constant DefaultAddr is defined in cmd/ingest/osv/osv.go with the value "http://localhost:8089". Since the test file is in the same package, it has access to this constant without requiring an explicit import.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DefaultAddr is defined in the package
# Search for DefaultAddr definition
rg -t go "DefaultAddr.*=.*" cmd/ingest/osv/
Length of output: 149
go.mod (2)
14-24: LGTM on dependency changes
The changes align well with the PR objective of migrating from fx to wire:
- Added wire for dependency injection
- Removed fx-related dependencies (fx, dig, multierr, zap)
- Moved pflag from indirect to direct dependencies which makes sense if it's now being used directly
The dependency structure looks clean and appropriate for the migration.
14-14: Verify wire version and security advisories
Let's verify that we're using the latest stable version of wire and check for any security concerns.
✅ Verification successful
Wire version is up-to-date and secure
The project is using wire v0.6.0 which is the latest stable release, and there are no security advisories reported for this package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest wire version and any security advisories
# Get latest wire version from GitHub releases
echo "Latest wire version:"
gh api repos/google/wire/releases/latest --jq .tag_name
# Check for security advisories
echo -e "\nSecurity advisories:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/google/wire") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 527
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
main.go (1)
12-14: Enhance error handling with descriptive messagesWhile the error handling is functional, it could be more informative for debugging purposes.
Consider adding error details before exiting:
if err := rootCmd.Execute(); err != nil { + fmt.Fprintf(os.Stderr, "Error executing root command: %v\n", err) os.Exit(1) }Regarding the test coverage warnings: While it's generally good practice to have high test coverage, testing the
main()function is often skipped as it primarily serves as an entry point and delegates actual functionality to other packages. The critical test coverage should focus on therootpackage implementation instead.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-14: main.go#L12-L14
Added lines #L12 - L14 were not covered by testscmd/root/root.go (1)
33-49: Consider enhancing server management.While the current implementation is solid, consider these improvements:
- Extract server setup to a separate method for better testability
- Add timeout to shutdown context
func New() *cobra.Command { o := &Options{} + setupPprofServer := func(cmd *cobra.Command) { + srv := &http.Server{Addr: o.PprofAddr} + go func() { + fmt.Printf("Starting pprof server on %s\n", o.PprofAddr) + if err := srv.ListenAndServe(); err != http.ErrServerClosed { + fmt.Printf("pprof server error: %v\n", err) + } + }() + cmd.PersistentPostRun = func(cmd *cobra.Command, args []string) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := srv.Shutdown(ctx); err != nil { + fmt.Printf("pprof server shutdown error: %v\n", err) + } + } + } rootCmd := &cobra.Command{ // ... existing command setup ... PersistentPreRun: func(cmd *cobra.Command, args []string) { o.AddFlags(cmd) if o.PprofEnabled { - srv := &http.Server{Addr: o.PprofAddr} - go func() { - fmt.Printf("Starting pprof server on %s\n", o.PprofAddr) - if err := srv.ListenAndServe(); err != http.ErrServerClosed { - fmt.Printf("pprof server error: %v\n", err) - } - }() - cmd.PersistentPostRun = func(cmd *cobra.Command, args []string) { - if err := srv.Shutdown(context.Background()); err != nil { - fmt.Printf("pprof server shutdown error: %v\n", err) - } - } + setupPprofServer(cmd) } }, }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Makefile(2 hunks)cmd/root/root.go(1 hunks)main.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/root/root.go (1)
Learnt from: naveensrinivasan
PR: bitbomdev/minefield#133
File: cmd/root/root.go:20-22
Timestamp: 2024-11-15T13:47:35.287Z
Learning: In the `cmd/root/root.go` file of the Go codebase, the pprof server is not enabled by default; it starts only when the `--pprof` flag is provided. Therefore, unless explicitly enabled by the user, the pprof server will not run.
🪛 GitHub Check: codecov/patch
cmd/root/root.go
[warning] 21-23: cmd/root/root.go#L21-L23
Added lines #L21 - L23 were not covered by tests
[warning] 26-28: cmd/root/root.go#L26-L28
Added lines #L26 - L28 were not covered by tests
[warning] 30-30: cmd/root/root.go#L30
Added line #L30 was not covered by tests
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 52-56: cmd/root/root.go#L52-L56
Added lines #L52 - L56 were not covered by tests
[warning] 58-58: cmd/root/root.go#L58
Added line #L58 was not covered by tests
main.go
[warning] 10-10: main.go#L10
Added line #L10 was not covered by tests
[warning] 12-14: main.go#L12-L14
Added lines #L12 - L14 were not covered by tests
🔇 Additional comments (8)
main.go (3)
4-7: LGTM! Clean import declarations
The imports have been properly cleaned up, removing unused dependencies while maintaining only the essential ones needed for the refactored implementation.
1-14: Architectural benefits of migrating to Wire
The refactoring from fx to Wire brings several advantages:
- Compile-time dependency injection helps catch configuration errors earlier
- Reduced runtime overhead as the dependency graph is resolved at compile-time
- Simpler runtime initialization with less magical behavior
However, ensure that:
- All dependency providers are properly defined in wire.go
- The wire generation is included in the build process
- The migration is complete across all packages
Let's verify the wire setup:
#!/bin/bash
# Check for wire.go files and their providers
fd wire.go --exec cat {} \;
# Verify wire is included in build process
cat Makefile🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: main.go#L10
Added line #L10 was not covered by tests
[warning] 12-14: main.go#L12-L14
Added lines #L12 - L14 were not covered by tests
10-10: Consider handling potential initialization errors
The root command initialization should include error handling if root.New() can return an error.
Let's verify the implementation of root.New():
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: main.go#L10
Added line #L10 was not covered by tests
Makefile (2)
37-38: LGTM! Clear error message with installation instructions.
The check-wire target provides a user-friendly verification of wire installation with clear instructions.
40-41: Verify wire generation coverage
The wire generation is only running in cmd/server. Let's verify if there are other directories that might need wire generation.
✅ Verification successful
Wire generation is correctly configured
The verification shows that wire generation is only needed in cmd/server:
- Only one
wire.gofile exists atcmd/server/wire.go - The wireinject build tag is only present in this same file
- No other wire.Build calls were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for wire.go files in other directories that might need wire generation
# Look for wire.go files
echo "Searching for wire.go files:"
fd "wire.go$"
# Look for potential wire build tags in go files
echo -e "\nSearching for wire build tags in go files:"
rg "//\s*\+build\s+wireinject" --type go
# Look for wire.Build calls in go files
echo -e "\nSearching for wire.Build calls:"
ast-grep --pattern 'wire.Build($$$)'
Length of output: 403
cmd/root/root.go (3)
16-23: LGTM! Clean struct definition and flag configuration.
The exported Options struct and its AddFlags method are well-structured with clear flag descriptions and sensible defaults.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-23: cmd/root/root.go#L21-L23
Added lines #L21 - L23 were not covered by tests
26-49: LGTM! Well-structured command setup with proper error handling.
The command initialization and pprof server implementation show good attention to detail, including proper error handling and graceful shutdown.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-28: cmd/root/root.go#L26-L28
Added lines #L26 - L28 were not covered by tests
[warning] 30-30: cmd/root/root.go#L30
Added line #L30 was not covered by tests
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
52-58: LGTM! Clean subcommand registration.
The subcommand registration is consistent with the removal of the storage parameter across the codebase.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-56: cmd/root/root.go#L52-L56
Added lines #L52 - L56 were not covered by tests
[warning] 58-58: cmd/root/root.go#L58
Added line #L58 was not covered by tests
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cmd/root/root.go (1)
16-19: Consider exporting the options typeSince the struct contains exported fields and is part of the package's API, consider renaming
optionstoOptionsfor consistency with Go conventions.-type options struct { +type Options struct { PprofAddr string PprofEnabled bool }go.mod (1)
Line range hint
3-3: Fix invalid Go versionThe specified Go version
1.23.1is invalid. Go versions follow semantic versioning, and the latest stable version is 1.22.x.Apply this diff to fix the version:
-go 1.23.1 +go 1.22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/root/root.go(1 hunks)go.mod(1 hunks)pkg/graph/visualizer.go(0 hunks)pkg/graph/visualizer_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pkg/graph/visualizer.go
- pkg/graph/visualizer_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/root/root.go
[warning] 22-23: cmd/root/root.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 26-26: cmd/root/root.go#L26
Added line #L26 was not covered by tests
[warning] 28-28: cmd/root/root.go#L28
Added line #L28 was not covered by tests
[warning] 30-30: cmd/root/root.go#L30
Added line #L30 was not covered by tests
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 52-56: cmd/root/root.go#L52-L56
Added lines #L52 - L56 were not covered by tests
[warning] 58-58: cmd/root/root.go#L58
Added line #L58 was not covered by tests
🔇 Additional comments (3)
cmd/root/root.go (1)
33-49:
Fix potential race condition in command run hooks
The PersistentPostRun is set conditionally inside PersistentPreRun. This could lead to a race condition if another goroutine also modifies these hooks. Additionally, if a subcommand defines its own PersistentPostRun, it would be overwritten.
Consider this safer approach:
PersistentPreRun: func(cmd *cobra.Command, args []string) {
o.AddFlags(cmd)
if o.PprofEnabled {
srv := &http.Server{Addr: o.PprofAddr}
go func() {
fmt.Printf("Starting pprof server on %s\n", o.PprofAddr)
if err := srv.ListenAndServe(); err != http.ErrServerClosed {
fmt.Printf("pprof server error: %v\n", err)
}
}()
- cmd.PersistentPostRun = func(cmd *cobra.Command, args []string) {
- if err := srv.Shutdown(context.Background()); err != nil {
- fmt.Printf("pprof server shutdown error: %v\n", err)
- }
- }
}
},
+PersistentPostRun: func(cmd *cobra.Command, args []string) {
+ if o.PprofEnabled {
+ srv := &http.Server{Addr: o.PprofAddr}
+ if err := srv.Shutdown(context.Background()); err != nil {
+ fmt.Printf("pprof server shutdown error: %v\n", err)
+ }
+ }
+},Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-41: cmd/root/root.go#L33-L41
Added lines #L33 - L41 were not covered by tests
[warning] 43-46: cmd/root/root.go#L43-L46
Added lines #L43 - L46 were not covered by tests
go.mod (2)
13-13: LGTM: Wire and pflag dependencies added
The addition of github.com/google/wire v0.6.0 aligns with the PR objective to refactor dependency injection. The pflag package is a common companion for command-line applications.
Also applies to: 18-18
Line range hint 1-54: Verify logger replacement after zap removal
The removal of go.uber.org/zap suggests that the logging implementation has changed. We should verify that proper logging is still in place.
Consider documenting the migration strategy
The switch from fx to Wire represents a significant architectural change in how dependencies are managed. While Wire offers compile-time dependency injection compared to fx's runtime DI, this change might impact:
- Application startup and configuration
- Testing and mocking capabilities
- Error handling patterns
Consider documenting:
- The rationale behind the switch
- Migration patterns for other teams
- Any changes in error handling strategy (especially with the removal of
multierr)
✅ Verification successful
Let me try a different search strategy to verify the logging implementation changes.
This should help us understand what logging solution is currently being used in the codebase after the removal of zap.
Standard library logging is in use after zap removal
The codebase has been simplified to use Go's standard library log package, primarily in cmd/server/server.go for basic server logging. The logging implementation is minimal but sufficient for the current server operations:
- Server startup notification
- Error logging for server issues
- Graceful shutdown logging
There's also a commented-out log statement in pkg/tools/ingest/sbom.go that isn't active.
While logrus is present in go.mod, it appears to be an indirect dependency from other packages rather than direct usage, as evidenced by its presence only in test data and dependency files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for logging implementations
echo "Searching for logging imports..."
rg -l "import.*log"
# Search for log usage patterns
echo "Searching for logging usage..."
ast-grep --pattern 'log.$_($_)'
Length of output: 240
Script:
#!/bin/bash
# Search for any logging-related imports including standard library
echo "Searching for standard library log imports..."
rg -l "\"log\""
echo "Searching for logrus usage since it's in dependencies..."
rg -l "github.com/sirupsen/logrus"
echo "Searching for logging function calls..."
rg -n "log\." -t go
echo "Searching for logger variable declarations..."
ast-grep --pattern 'var $_ *log.$_'
ast-grep --pattern 'logger := $_'
Length of output: 1248
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores
Refactor