add command timeout with context.Cause for better error handling #65
Conversation
📝 WalkthroughWalkthroughReplaced timeout handling in the scanning service to use Changes
Sequence Diagram(s)sequenceDiagram
participant Server as RunServer
participant Env as Env Setup
participant TestCmd as testHPSMSetup (exec)
participant HPSM as HPSM Content Endpoint
Server->>Env: setupEnvVars()
Env-->>Server: env vars set (maybe API key)
Server->>TestCmd: testHPSMSetup(SCANOSS_FILE_CONTENTS_URL, optional key)
TestCmd->>HPSM: wget request (URL + optional X-Session header)
HPSM-->>TestCmd: HTTP response (success/failure)
TestCmd-->>Server: success -> keep HPSM enabled / failure -> disable HPSM and log
Server->>Server: start REST server (if continued)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cmd/server.go`:
- Around line 112-117: The code currently logs the sensitive
cfg.Scanning.HPSMcontentsAPIkey when os.Setenv("SCANOSS_API_KEY", ...) fails;
remove the key from logs and only log a non-sensitive message and the error.
Update the failure branch around os.Setenv in the block that checks
cfg.Scanning.HPSMEnabled to call zlog.S (e.g., Infof/Errorf) with a message like
"Failed to set SCANOSS_API_KEY environment variable" and include only the error
(no key value), so references are to cfg.Scanning.HPSMcontentsAPIkey, os.Setenv
and zlog.S.Infof/Errorf for locating the code to change.
🧹 Nitpick comments (2)
pkg/config/server_config.go (1)
70-70: Field naming inconsistency and truncated comment.
- The field name
HPSMcontentsAPIkeyuses inconsistent casing. Following Go conventions for acronyms, considerHPSMContentsAPIKey.- The comment appears truncated:
"HPSM ser"— it seems to be cut off mid-word.Suggested fix
- HPSMcontentsAPIkey string `env:"SCANOSS_FILE_CONTENTS_APIKEY"` // API key used to access the file contents from SCANOSS_FILE_CONTENTS_URL with HPSM ser + HPSMContentsAPIKey string `env:"SCANOSS_FILE_CONTENTS_APIKEY"` // API key used to access the file contents from SCANOSS_FILE_CONTENTS_URL with HPSM serviceNote: If you rename the field, update all usages in
pkg/cmd/server.go(e.g.,cfg.Scanning.HPSMcontentsAPIkey→cfg.Scanning.HPSMContentsAPIKey).pkg/cmd/server.go (1)
175-218: Replacewgetdependency with Go'snet/httpclient for better portability and to fix ineffective stderr capture.While
wgetis available on Alpine Linux by default, using an external command introduces portability risks for Windows, macOS, minimal containers, and other environments withoutwgetpre-installed. Additionally, the-qflag suppresseswget's diagnostic output (including errors sent to stderr), making the stderr capture on lines 207–212 ineffective.The hardcoded MD5
8109a183e06165144dc8d97b791c130fon line 184 should include a comment explaining what file it represents.Using Go's
net/httppackage eliminates the external dependency, improves cross-platform compatibility, and simplifies error handling:Proposed refactor using net/http
func testHPSMSetup(s *zap.SugaredLogger) error { url := os.Getenv("SCANOSS_FILE_CONTENTS_URL") if url == "" { return fmt.Errorf("SCANOSS_FILE_CONTENTS_URL is not set") } if !strings.HasSuffix(url, "/") { url += "/" } // Known test file MD5 (e.g., from a commonly available open-source file) url += "8109a183e06165144dc8d97b791c130f" req, err := http.NewRequest(http.MethodGet, url, nil) if err != nil { return fmt.Errorf("failed to create request: %w", err) } apiKey := os.Getenv("SCANOSS_API_KEY") if apiKey != "" { req.Header.Set("X-Session", apiKey) } else { s.Debug("No SCANOSS_API_KEY set; proceeding without authentication header.") } client := &http.Client{Timeout: 10 * time.Second} s.Debugf("HPSM test request: GET %s", url) resp, err := client.Do(req) if err != nil { s.Debugf("HPSM connection test failed for URL %s: %v", url, err) return err } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { s.Debugf("HPSM connection test failed for URL %s: HTTP %d", url, resp.StatusCode) return fmt.Errorf("HTTP %d", resp.StatusCode) } s.Infof("HPSM setup test successful for URL: %s", url) return nil }This requires adding
"net/http"and"time"imports.
| if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 { | ||
| err := os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey) | ||
| if err != nil { | ||
| zlog.S.Infof("Failed to set SCANOSS_API_KEY value to %v: %v", cfg.Scanning.HPSMcontentsAPIkey, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Security: Avoid logging sensitive API key values.
The API key is logged if os.Setenv fails. Secrets should not appear in logs as they may be persisted and exposed.
Proposed fix
if cfg.Scanning.HPSMEnabled && len(cfg.Scanning.HPSMcontentsAPIkey) > 0 {
err := os.Setenv("SCANOSS_API_KEY", cfg.Scanning.HPSMcontentsAPIkey)
if err != nil {
- zlog.S.Infof("Failed to set SCANOSS_API_KEY value to %v: %v", cfg.Scanning.HPSMcontentsAPIkey, err)
+ zlog.S.Infof("Failed to set SCANOSS_API_KEY: %v", err)
}
}🤖 Prompt for AI Agents
In `@pkg/cmd/server.go` around lines 112 - 117, The code currently logs the
sensitive cfg.Scanning.HPSMcontentsAPIkey when os.Setenv("SCANOSS_API_KEY", ...)
fails; remove the key from logs and only log a non-sensitive message and the
error. Update the failure branch around os.Setenv in the block that checks
cfg.Scanning.HPSMEnabled to call zlog.S (e.g., Infof/Errorf) with a message like
"Failed to set SCANOSS_API_KEY environment variable" and include only the error
(no key value), so references are to cfg.Scanning.HPSMcontentsAPIkey, os.Setenv
and zlog.S.Infof/Errorf for locating the code to change.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.