Sheridan C Rawlins
Sheridan C Rawlins
## 🔴 CRITICAL: Check context cancellation (gdpr/vendorlist-fetching.go:98-106) If the init timeout expires, goroutines continue spawning wastefully. Check `ctx.Done()` before spawning new goroutines: ```go for i := firstVersion; i < latestVersion;...
## 🔴 CRITICAL: Handle Wait() errors (gdpr/vendorlist-fetching.go:108-111) Errors from errgroup should be checked and logged: ```go if err := wgLatestVersion.Wait(); err != nil { glog.Errorf("Error during vendor list preload (latest...
## 🟡 HIGH: Nested errgroup pattern is complex (gdpr/vendorlist-fetching.go:82-111) The nested errgroup pattern works but is unnecessarily complex. Consider simplifying to a single errgroup: ```go var eg errgroup.Group if conf.MaxConcurrencyInitFetchSpecificVersion...
## 🟡 HIGH: Clarify configuration documentation (config/config.go:244-248) The comment "0 or negative means no limit" is misleading. The code checks `> 0`, so when value is ≤ 0, `SetLimit()` is...
## 🟡 HIGH: Missing concurrent test coverage (gdpr/vendorlist-fetching_test.go:256-262) The test sets concurrency to 2 but doesn't verify that actual parallelism occurs. Consider adding tests for: 1. **Context cancellation during preload**...
## 🟢 MEDIUM: Add failure tracking (gdpr/vendorlist-fetching.go:112) The success message is logged even if many fetches failed. Consider tracking and reporting failures: ```go // Track successes and failures var successCount,...
## 🟢 MEDIUM: Unrelated change? (gdpr/vendorlist-fetching_test.go:295-297) The removal of `vendorListFallbackExpected` appears unrelated to the concurrency feature. Can you explain why this is included in this PR? If it's cleanup of...
I'm also hitting this - I expected version=~> 1.2.3 to work (or escaping the > and space) but… it seems to get * error downloading 'tfr://...?version=~+0.0.20': Error downloading module from...