Add pkg/envconfig package and consolidate environment variable access#711
Add pkg/envconfig package and consolidate environment variable access#711ericcurtin merged 1 commit intomainfrom
Conversation
Summary of ChangesHello @ericcurtin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a central Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- ModelsPath now panics on os.UserHomeDir() error, whereas previously main.go logged and exited with a clear message; consider preserving the explicit error handling instead of panicking in a library-style package.
- AllowedOrigins now always appends localhost/127.0.0.1/0.0.0.0 origins even when DMR_ORIGINS is unset, which widens CORS compared to the previous nil/"no origins allowed" behavior; if this is intentional, it might still be worth confirming it won’t expose endpoints unexpectedly.
- AsMap’s entry for DMR_ORIGINS uses AllowedOrigins(), but the description says "extra" origins while the value includes the localhost defaults as well; consider either returning only the configured extras or updating the description to match the actual value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ModelsPath now panics on os.UserHomeDir() error, whereas previously main.go logged and exited with a clear message; consider preserving the explicit error handling instead of panicking in a library-style package.
- AllowedOrigins now always appends localhost/127.0.0.1/0.0.0.0 origins even when DMR_ORIGINS is unset, which widens CORS compared to the previous nil/"no origins allowed" behavior; if this is intentional, it might still be worth confirming it won’t expose endpoints unexpectedly.
- AsMap’s entry for DMR_ORIGINS uses AllowedOrigins(), but the description says "extra" origins while the value includes the localhost defaults as well; consider either returning only the configured extras or updating the description to match the actual value.
## Individual Comments
### Comment 1
<location path="pkg/envconfig/envconfig.go" line_range="91-99" />
<code_context>
+
+// ModelsPath returns the directory where models are stored.
+// Configured via MODELS_PATH; defaults to ~/.docker/models.
+func ModelsPath() string {
+ if s := Var("MODELS_PATH"); s != "" {
+ return s
+ }
+ home, err := os.UserHomeDir()
+ if err != nil {
+ panic(err)
+ }
+ return filepath.Join(home, ".docker", "models")
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** ModelsPath now panics on UserHomeDir error, changing failure mode vs previous logged exit
This changes the failure from a logged, explicit exit to an unstructured panic, bypassing existing logging. Please either return an error from `ModelsPath` and let `main` handle/log it, or log the error with the existing logger before panicking so operators still see a clear, structured message.
</issue_to_address>
### Comment 2
<location path="pkg/envconfig/envconfig.go" line_range="219" />
<code_context>
+ "VLLM_METAL_SERVER_PATH": {"VLLM_METAL_SERVER_PATH", VLLMMetalServerPath(), "Path to vLLM Metal server binary"},
+ "DISABLE_METRICS": {"DISABLE_METRICS", DisableMetrics(), "Disable Prometheus metrics endpoint (any truthy value, e.g. 1)"},
+ "LOG_LEVEL": {"LOG_LEVEL", LogLevel(), "Log verbosity: debug, info, warn, error (default: info)"},
+ "DMR_ORIGINS": {"DMR_ORIGINS", AllowedOrigins(), "Comma-separated extra CORS allowed origins"},
+ "MODEL_RUNNER_TLS_ENABLED": {"MODEL_RUNNER_TLS_ENABLED", TLSEnabled(), "Enable TLS listener"},
+ "MODEL_RUNNER_TLS_PORT": {"MODEL_RUNNER_TLS_PORT", TLSPort(), "TLS listener port (default: 12444)"},
</code_context>
<issue_to_address>
**suggestion:** AsMap’s DMR_ORIGINS description doesn’t match the value returned (includes defaults, not just extras)
`AsMap` describes `DMR_ORIGINS` as “extra” origins, but the value comes from `AllowedOrigins()`, which already includes the localhost defaults. This can mislead users/tooling that expect only env-provided origins. Please either update the description to reflect “env + defaults” or change the value to return only env-configured origins (e.g., via a `RawAllowedOriginsFromEnv` helper).
```suggestion
"DMR_ORIGINS": {"DMR_ORIGINS", AllowedOrigins(), "Comma-separated CORS allowed origins (defaults plus any env-provided origins)"},
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new pkg/envconfig package to centralize and standardize access to environment variables, replacing numerous scattered os.Getenv/os.LookupEnv calls. This is a significant improvement for maintainability and clarity, as it provides typed and documented accessors for all server-side environment variables. The changes also include updating main.go and pkg/middleware/cors.go to utilize this new package, streamlining environment variable handling across the application. Overall, the refactoring is well-executed and enhances the robustness of configuration management.
I'm just gonna try and catch the bot comments before merge |
Introduces a central envconfig package, replacing ~20 scattered os.Getenv/os.LookupEnv calls in main.go and pkg/middleware/cors.go with typed, documented accessors. - Add pkg/envconfig/envconfig.go: Var, String, Bool, BoolWithDefault factory helpers; typed accessors for all server-side env vars; AllowedOrigins() with localhost/127.0.0.1/0.0.0.0 defaults; AsMap() for introspection - Update main.go: use envconfig.* throughout; remove DefaultTLSPort const; add AllowedOrigins to ServiceConfig - Update pkg/middleware/cors.go: remove private getAllowedOrigins(); fall back to envconfig.AllowedOrigins() which now includes sensible localhost defaults Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
force-pushed
the
envconfig
branch
from
5db9945 to
2f1da40
Compare
2 participants
Introduces a central envconfig package, replacing ~20 scattered os.Getenv/os.LookupEnv calls in main.go and pkg/middleware/cors.go with typed, documented accessors.