fix(config): persist API URL override#44
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI configuration model so an installer-selected API endpoint can be persisted in ~/.volcano/config.json, while still allowing process-level overrides via VOLCANO_API_URL and runtime overrides.
Changes:
- Added a persisted
api_urlfield (ConfiguredAPIURL) to the on-disk config model. - Updated
Config.APIURL()to resolve API URL with precedence: env → runtime override → persisted config → compiled default. - Updated/added tests to verify persistence and precedence behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/config/config.go | Adds a persisted API URL field and updates URL resolution precedence. |
| internal/config/config_test.go | Updates persistence tests and adds precedence coverage for the new field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // APIBaseURL overrides the configured API URL for synthetic command configs. | ||
| // It is intentionally not persisted to the user's cloud config file. | ||
| APIBaseURL string `json:"-"` | ||
| UserToken string `json:"user_token,omitempty"` | ||
| UserID string `json:"user_id,omitempty"` | ||
| AnonKey string `json:"-"` | ||
| ServiceKey string `json:"-"` | ||
| CurrentProject *ProjectConfig `json:"current_project,omitempty"` | ||
| APIBaseURL string `json:"-"` |
| if c.APIBaseURL != "" { | ||
| return c.APIBaseURL | ||
| } |
| require.NoError(t, err) | ||
| assert.NotContains(t, string(data), "api_url") | ||
| assert.NotContains(t, string(data), "http://localhost:8000") | ||
| assert.Contains(t, string(data), `"api_url": "http://configured.example.test"`) |
swkeever
left a comment
swkeever
left a comment
There was a problem hiding this comment.
Small, well-tested fix — precedence logic and round-trip behavior are covered by the new tests, gofmt/go vet/go test ./internal/config/... all pass. One non-blocking question inline.
| CurrentProject *ProjectConfig `json:"current_project,omitempty"` | ||
| APIBaseURL string `json:"-"` | ||
| // ConfiguredAPIURL is the durable API endpoint selected by the installer. | ||
| // VOLCANO_API_URL remains the higher-precedence, per-process override. |
There was a problem hiding this comment.
Nothing in this repo currently writes ConfiguredAPIURL — cfg.Save() call sites (auth.go, function.go, project.go) never set it, so today this only round-trips a value that was placed into config.json by an external process (per the comment, "selected by the installer"). Worth confirming that installer path actually exists/is landing separately, since right now there's no way for a user to get this field populated (short of hand-editing the file) and exercise the new precedence.
|
Closing as unnecessary scope expansion. End users use the CLI compiled production API URL and do not configure API endpoints. |
Summary
api_urlin~/.volcano/config.jsonVOLCANO_API_URL→ runtime override → persisted config → compiled defaultValidation
go test ./...