Repository navigation
Add environment variable validation on startup - #23
Conversation
|
Warning Rate limit exceeded@AnkanMisra has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 14 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds startup environment validation: Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (6)
-
gateway/main.go, line 57-89 (link)logic:
validateConfig()runs beforegodotenv.Load(), so env vars from.envwon't be loaded yet and validation will fail even with a valid.envfile -
gateway/main.go, line 80 (link)syntax: typo: "sing" should be "using"
-
gateway/main.go, line 67 (link)syntax: missing space in format string - should be
" - Port: %s\n", port -
gateway/main.go, line 70 (link)syntax: missing space in format string - should be
" - Model: %s\n", model -
gateway/main.go, line 73 (link)syntax: missing space in format string - should be
" - Verifier: %s\n", verifier -
gateway/main.go, line 76 (link)syntax: missing space in format string - should be
" - Chain ID: %s\n", chainID
1 file reviewed, 6 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (1)
gateway/main.go (1)
41-55: LGTM! Well-structured validation function.The validation logic is clear and extensible. The function correctly checks for required environment variables and provides a clear error message.
|
@ankitadhara28 please in the desciption say which issue are you closing or fixing using |
|
@greptile review this pr |
There was a problem hiding this comment.
Additional Comments (5)
-
gateway/main.go, line 63 (link)syntax: Incorrect printf formatting - extra space before
err.Error()causes malformed output -
gateway/main.go, line 69-81 (link)syntax: Inconsistent indentation (spaces instead of tabs) and missing spaces in format strings
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
gateway/main.go, line 88 (link)syntax: Inconsistent indentation (spaces instead of tabs)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
gateway/main.go, line 82-90 (link)syntax: Missing space after colon in format string (should be
"default: 3000")Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
gateway/main.go, line 92 (link)style: Extra blank line with trailing whitespace
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 file reviewed, 5 comments
AnkanMisra
left a comment
AnkanMisra
left a comment
There was a problem hiding this comment.
Solve the issue that i found and please add unit test cause the issue #18 requires: "Include unit tests for the validation function"
|
Hi @AnkanMisra |
|
This PR adds startup validation for required environment variables . Closes #18 |
There was a problem hiding this comment.
@ankitadhara28 Code Looks Good But you missed 2 details
- Unit tests for
validateConfig()
- No test file added
- CHAIN_ID warning when unset
- Currently warns about PORT, MODEL, VERIFIER_URL but not CHAIN_ID
After that ask for a Re-Review from me by up clicking the re-review button so i would get pinged
| func TestValidateConfig_WithRequiredEnv(t *testing.T) { | ||
| // Set required environment variables | ||
| os.Setenv("OPENROUTER_API_KEY", "test-key") | ||
|
|
||
| err := validateConfig() | ||
| if err != nil { | ||
| t.Fatalf("Expected no error when all required environment variables are set, got: %v", err) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add cleanup and consider testing optional variable behavior.
Similar to the previous test, this test modifies global environment state without cleanup. Additionally, according to the PR objectives, validateConfig() should warn about missing optional variables (PORT, MODEL, VERIFIER_URL, CHAIN_ID) and log a config summary on success. Consider adding tests to verify this behavior.
🔎 Recommended fix using `t.Setenv` and optional test coverage
Fix 1: Use t.Setenv for automatic cleanup
func TestValidateConfig_WithRequiredEnv(t *testing.T) {
// Set required environment variables
- os.Setenv("OPENROUTER_API_KEY", "test-key")
+ t.Setenv("OPENROUTER_API_KEY", "test-key")
err := validateConfig()
if err != nil {
t.Fatalf("Expected no error when all required environment variables are set, got: %v", err)
}
}Fix 2 (optional): Add test coverage for optional variables
Consider adding a test to verify warning behavior when optional variables are missing:
func TestValidateConfig_OptionalVariables(t *testing.T) {
// Set required env var
t.Setenv("OPENROUTER_API_KEY", "test-key")
// Unset optional vars to trigger warnings
t.Setenv("PORT", "")
t.Setenv("MODEL", "")
t.Setenv("VERIFIER_URL", "")
err := validateConfig()
if err != nil {
t.Fatalf("Expected no error with optional vars missing, got: %v", err)
}
// Note: Verifying log output would require capturing stdout/stderr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestValidateConfig_WithRequiredEnv(t *testing.T) { | |
| // Set required environment variables | |
| os.Setenv("OPENROUTER_API_KEY", "test-key") | |
| err := validateConfig() | |
| if err != nil { | |
| t.Fatalf("Expected no error when all required environment variables are set, got: %v", err) | |
| } | |
| } | |
| func TestValidateConfig_WithRequiredEnv(t *testing.T) { | |
| // Set required environment variables | |
| t.Setenv("OPENROUTER_API_KEY", "test-key") | |
| err := validateConfig() | |
| if err != nil { | |
| t.Fatalf("Expected no error when all required environment variables are set, got: %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In gateway/config_test.go around lines 18 to 26, the test sets a global env var
without cleanup and doesn't cover optional-variable warning behavior; update the
test to use t.Setenv("OPENROUTER_API_KEY", "test-key") so the environment is
restored automatically, and add an additional test (e.g.,
TestValidateConfig_OptionalVariables) that sets only the required
OPENROUTER_API_KEY and explicitly clears PORT, MODEL, VERIFIER_URL, CHAIN_ID
(via t.Setenv to empty) then calls validateConfig() asserting no error and
captures/logs output (or use a log capture helper) to assert warnings for
missing optional vars and that a config summary is logged on success.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/main.go (1)
91-92: Fix indentation and improve spacing consistency.Line 91 appears to use spaces for indentation instead of tabs, which is inconsistent with Go conventions and the surrounding code. Additionally, the warning message on line 92 would read better with a space before the parenthesis.
🔎 Suggested formatting fixes
- if os.Getenv("CHAIN_ID") == "" { - fmt.Println("[WARN] CHAIN_ID not set, using default: 8453(base)") + if os.Getenv("CHAIN_ID") == "" { + fmt.Println("[WARN] CHAIN_ID not set, using default: 8453 (base)") }After applying this change, please run
gofmt -w gateway/main.goto ensure consistent formatting throughout the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/main.go
🔇 Additional comments (3)
gateway/main.go (3)
41-55: LGTM! Clean validation logic.The validation function correctly checks for required environment variables and returns a clear error message. The design using a slice makes it easy to add more required variables in the future.
56-68: Excellent fail-fast validation!The validation correctly runs after loading the .env file and provides clear, actionable error messages before exiting with a non-zero status. This addresses the critical ordering issue from previous reviews.
69-81: Configuration summary looks good!The success logging provides a helpful non-sensitive configuration summary with proper formatting. The conditional printing ensures only set values are displayed.
|
@greptile final review |
There was a problem hiding this comment.
Additional Comments (2)
-
gateway/main.go, line 91 (link)syntax: inconsistent indentation (4 spaces instead of tab)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
gateway/config_test.go, line 18-26 (link)style: test doesn't clean up env var after setting it, could affect other tests
2 files reviewed, 2 comments
|
Hi @AnkanMisra , |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/config_test.go (1)
14-21: Test cleanup improved; consider expanding coverage for optional variables.The use of
t.Setenvproperly addresses the cleanup concern from the previous review.Per the PR objectives,
validateConfig()should warn about missing optional variables (PORT, MODEL, VERIFIER_URL, CHAIN_ID) and log a config summary on success. Consider adding a test that verifies this behavior for more complete coverage of the acceptance criteria.🔎 Example test for optional variable warnings
func TestValidateConfig_OptionalVariables(t *testing.T) { // Set required env var t.Setenv("OPENROUTER_API_KEY", "test-key") // Explicitly unset optional vars to trigger warnings t.Setenv("PORT", "") t.Setenv("MODEL", "") t.Setenv("VERIFIER_URL", "") t.Setenv("CHAIN_ID", "") err := validateConfig() if err != nil { t.Fatalf("expected no error with optional vars missing, got: %v", err) } // Note: Verifying log output requires capturing stdout/stderr // or using a testing logger }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/config_test.go
🔇 Additional comments (1)
gateway/config_test.go (1)
5-12: Test pollution issue resolved!The use of
t.Setenvproperly addresses the previous concern about environment variable cleanup. The test correctly verifies thatvalidateConfig()returns an error when the required environment variable is missing.
@ankitadhara28
You are 7 commit behind of the latest main branch please update to the latest changes in main branch You need to |
AnkanMisra
added
level:beginner
Closes #18
Summary
Adds startup validation for required environment variables to fail fast and provide clear error messages.
Changes
Testing
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Summary
Added startup validation for
OPENROUTER_API_KEYto fail fast with clear error messages before the server starts. The implementation validates required environment variables immediately after loading the.envfile, exits with code 1 if validation fails, and prints a helpful configuration summary on successful startup with warnings for missing optional variables.Changes:
validateConfig()function that checks for required env vars and returns descriptive errorsmain()after env loading, before server initialization.env.exampleandREADME.mdon validation failurePORT,MODEL,VERIFIER_URL, andCHAIN_IDwhen set[WARN]messages for optional env vars with default values when not setIssues Found:
TestValidateConfig_WithRequiredEnvdoesn't clean up the env var it setsConfidence Score: 4/5
Important Files Changed
validateConfig()function and startup validation with warning messages for optional env vars. Minor indentation inconsistency on line 91.Sequence Diagram
sequenceDiagram participant Main as main() participant Env as Environment participant Validate as validateConfig() participant Server as Gin Server Main->>Env: Load ../.env file alt .env file not found Env-->>Main: Warning logged end Main->>Validate: validateConfig() Validate->>Env: Check OPENROUTER_API_KEY alt Missing required env var Validate-->>Main: Return error Main->>Main: Print error message Main->>Main: os.Exit(1) else All required vars present Validate-->>Main: Return nil Main->>Main: Print "[OK] Configuration validated" Main->>Env: Read optional vars (PORT, MODEL, etc.) Main->>Main: Print configured values Main->>Env: Check optional vars alt Optional var missing Main->>Main: Print [WARN] messages end Main->>Server: Initialize and start Gin server end