Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the CLI/server startup options by consolidating the previously duplicated CLI flag definitions into a shared server.Config type and updating server initialization accordingly.
Changes:
- Replaces
server.Optionswithserver.Config(including CLI flag metadata) and updatesserver.NewAppto accept it. - Embeds
server.Configinto thestartcommand to avoid re-declaring CLI flags incmd. - Moves the (formerly options-based) test HTTP client hook onto
App(but does not currently wire it up).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/server.go | Introduces server.Config, updates App/NewApp signature, and adjusts client creation logic. |
| cmd/cmds.go | Embeds server.Config into StartCmd and passes it directly into server.NewApp. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type Config struct { | ||
| Host string `default:"localhost" help:"Listening address"` | ||
| Port int `default:"8080" help:"Listening port" env:"ONTAP_MCP_PORT"` | ||
| InspectTraffic bool `default:"false" help:"Inspect MCP HTTP traffic"` | ||
| ReadOnly bool `default:"false" help:"Run MCP in read-only mode. This disables all tool calls that modify ONTAP state."` |
There was a problem hiding this comment.
server.Config now carries CLI-specific kong tags (default, help, env). This couples the server package API to the CLI parser and makes it easy to assume defaults are applied when constructing server.Config programmatically (they are only applied by kong). Consider keeping server.Config as a parser-agnostic runtime config and defining a separate CLI flags struct in cmd (or using a type alias/wrapper) to hold kong tags.
| type App struct { | ||
| logger *slog.Logger | ||
| cfg *config.ONTAP | ||
| options Options | ||
| locks *lock.Map | ||
| catalog catalog.APICatalog | ||
| versionCache sync.Map | ||
| logger *slog.Logger | ||
| cfg *config.ONTAP | ||
| options Config | ||
| testHTTPClient *http.Client | ||
| locks *lock.Map |
There was a problem hiding this comment.
App.testHTTPClient is never set anywhere in the codebase after this refactor, which makes the getClient() branch effectively dead and removes the previous ability to inject an HTTP client via options. Either remove this field/branch, or add an explicit way to set it (e.g., a NewApp parameter/functional option or a setter) so tests and callers can actually use it.
| if a.testHTTPClient != nil { | ||
| return rest.NewWithClient(poller, a.testHTTPClient), nil | ||
| } |
There was a problem hiding this comment.
getClient() checks a.testHTTPClient, but there is no code path that assigns this field after removing Options.TestHTTPClient, so this branch is currently dead. Either wire testHTTPClient through NewApp (or another public API) or remove the branch to avoid confusing future readers.
| if a.testHTTPClient != nil { | |
| return rest.NewWithClient(poller, a.testHTTPClient), nil | |
| } |
No description provided.