feat(cli): add kind and endpointHost to docker model status --json #499
Conversation
- kind: deployment type (Docker Desktop, Docker Engine, Docker Cloud, Docker Engine (Manual Install)) - endpointHost: endpoint URL for host-to-model-runner communication - endpoint: existing field, for container-to-model-runner communication Signed-off-by: Dorin Geman <[email protected]>
Summary of ChangesHello @doringeman, 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 significantly improves the diagnostic capabilities of the Highlights
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 there - I've reviewed your changes - here's some feedback:
- In
jsonStatus, the Cloud branch assumesrunneris non-nil; consider adding a nil check with an explicit error to avoid a potential panic ifjsonStatusis ever called without a runner for this kind. - There are multiple hard-coded
"/v1/"and endpoint constructions across the new logic and tests; consider centralizing these paths (or reusingmakeEndpointwhere possible) to avoid divergence if the API base path changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `jsonStatus`, the Cloud branch assumes `runner` is non-nil; consider adding a nil check with an explicit error to avoid a potential panic if `jsonStatus` is ever called without a runner for this kind.
- There are multiple hard-coded `"/v1/"` and endpoint constructions across the new logic and tests; consider centralizing these paths (or reusing `makeEndpoint` where possible) to avoid divergence if the API base path changes.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/status_test.go:135-141` </location>
<code_context>
}
+
+func TestJsonStatus(t *testing.T) {
+ tests := []struct {
+ name string
+ engineKind types.ModelRunnerEngineKind
+ urlPrefix string
+ expectedKind string
+ expectedEndpoint string
+ expectedHostEnd string
+ }{
+ {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend coverage to validate `Backends` and `Running` fields, not just kind and endpoints
The table-driven tests currently cover `kind`, `endpoint`, `endpointHost`, and `running`, but don’t verify that the JSON `backends` field reflects the input `backendStatus`. Please add assertions on `result["backends"]` (e.g., via `map[string]any` or a small struct) and include at least one case with `status.Running == false` and/or an empty `backendStatus` to confirm those values are serialized correctly.
Suggested implementation:
```golang
func TestJsonStatus(t *testing.T) {
tests := []struct {
name string
engineKind types.ModelRunnerEngineKind
urlPrefix string
expectedKind string
expectedEndpoint string
expectedHostEnd string
expectedBackends map[string]any
expectedRunning bool
}{
```
```golang
{
name: "Docker Desktop",
engineKind: types.ModelRunnerEngineKindDesktop,
urlPrefix: "http://localhost" + inference.ExperimentalEndpointsPrefix,
expectedKind: "Docker Desktop",
expectedEndpoint: "http://model-runner.docker.internal/v1/",
expectedHostEnd: "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/",
// Expect at least one backend entry when running on Desktop.
// The exact keys/values should be aligned with the status implementation.
expectedBackends: map[string]any{
"desktop": map[string]any{
"running": true,
},
},
expectedRunning: true,
},
{
name: "Docker Engine",
engineKind: types.ModelRunnerEngineKindMoby,
```
To fully implement your comment, the following additional changes are needed elsewhere in `TestJsonStatus`, in parts of the file not shown in your snippet:
1. **Update the table-driven test loop to use the new fields**
Wherever you iterate over `tests`, extend the expectations to include `backends` and `running`. Assuming the current code looks roughly like:
```go
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// ... invoke the command, capture JSON output
var result map[string]any
err := json.Unmarshal(out.Bytes(), &result)
require.NoError(t, err)
require.Equal(t, tt.expectedKind, result["kind"])
require.Equal(t, tt.expectedEndpoint, result["endpoint"])
require.Equal(t, tt.expectedHostEnd, result["endpointHost"])
})
}
```
You should extend it along these lines:
```go
require.Equal(t, tt.expectedRunning, result["running"])
backendsRaw, ok := result["backends"]
if tt.expectedBackends == nil {
// When we expect no backends, assert that the field is either
// absent or an empty map/slice (depending on implementation).
if ok {
switch v := backendsRaw.(type) {
case map[string]any:
require.Len(t, v, 0)
case []any:
require.Len(t, v, 0)
default:
require.Failf(t, "unexpected backends type", "%T", v)
}
}
} else {
require.True(t, ok, "backends field should be present")
backends, ok := backendsRaw.(map[string]any)
require.True(t, ok, "backends should be a JSON object")
// You can do a deep equality check if the shape matches exactly:
// require.Equal(t, tt.expectedBackends, backends)
//
// Or, if the real shape is richer, assert selectively:
for k, v := range tt.expectedBackends {
require.Contains(t, backends, k)
require.Equal(t, v, backends[k])
}
}
```
2. **Add a test case with `Running == false` and/or empty backends**
In addition to the updated "Docker Desktop" case, add at least one table entry like:
```go
{
name: "Not running with empty backends",
engineKind: types.ModelRunnerEngineKindMoby, // or whatever triggers not-running
urlPrefix: "http://localhost" + inference.ExperimentalEndpointsPrefix,
expectedKind: "Docker Engine",
expectedEndpoint: "http://localhost/v1/", // align with existing expectations
expectedHostEnd: "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/",
expectedBackends: nil, // or map[string]any{} if JSON always includes an empty object
expectedRunning: false, // ensure JSON "running" is false
}
```
Adjust the specific field values (`expectedKind`, endpoint URLs, the `expectedBackends` structure, and which `engineKind` results in `running == false`) to match the actual behavior of your `status` command and JSON schema.
3. **Align `expectedBackends` structure with actual JSON**
The example above assumes `backends` is a JSON object like:
```json
"backends": {
"desktop": { "running": true }
}
```
If the production code uses different keys or nesting (e.g., `"docker-desktop"` vs `"desktop"`, or includes additional fields), adjust the `expectedBackends` literals and the assertions to match that exact shape.
</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 enhances the docker model status --json command by adding kind and endpointHost fields to the output. This provides more detailed context about the model runner's deployment type and communication endpoints. The implementation is clean, introducing a makeEndpoint helper function to reduce code duplication and adding a comprehensive set of table-driven tests to validate the new functionality across different engine kinds.
My review includes two main points. First, a critical issue regarding a potential nil pointer dereference that could crash the command when handling the Cloud engine kind. Second, a suggestion to refactor a hardcoded URL construction for better code consistency and maintainability. Addressing these points will improve the robustness and quality of the code.
| case types.ModelRunnerEngineKindCloud: | ||
| fallthrough | ||
| case types.ModelRunnerEngineKindMoby: | ||
| if standalone.gatewayIP == "" { | ||
| standalone.gatewayIP = "127.0.0.1" | ||
| if runner.gatewayIP == "" { | ||
| runner.gatewayIP = "127.0.0.1" | ||
| } | ||
|
|
||
| if standalone.gatewayPort == 0 { | ||
| standalone.gatewayPort = 12434 | ||
| if runner.gatewayPort == 0 { | ||
| runner.gatewayPort = standalone.DefaultControllerPortCloud | ||
| } | ||
|
|
||
| endpoint = "http://" + net.JoinHostPort(standalone.gatewayIP, strconv.Itoa(int(standalone.gatewayPort))) + "/engines/v1/" | ||
| endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort)) | ||
| endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud) |
There was a problem hiding this comment.
There's a potential nil pointer dereference here. The runner variable can be nil if ensureStandaloneRunnerAvailable returns a nil runner (e.g., for non-standalone setups). If kind is ModelRunnerEngineKindCloud, the code proceeds to access runner.gatewayIP, which would cause a panic if runner is nil. Adding a nil check for runner will make the code more robust and prevent a crash.
| case types.ModelRunnerEngineKindCloud: | |
| fallthrough | |
| case types.ModelRunnerEngineKindMoby: | |
| if standalone.gatewayIP == "" { | |
| standalone.gatewayIP = "127.0.0.1" | |
| if runner.gatewayIP == "" { | |
| runner.gatewayIP = "127.0.0.1" | |
| } | |
| if standalone.gatewayPort == 0 { | |
| standalone.gatewayPort = 12434 | |
| if runner.gatewayPort == 0 { | |
| runner.gatewayPort = standalone.DefaultControllerPortCloud | |
| } | |
| endpoint = "http://" + net.JoinHostPort(standalone.gatewayIP, strconv.Itoa(int(standalone.gatewayPort))) + "/engines/v1/" | |
| endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort)) | |
| endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud) | |
| case types.ModelRunnerEngineKindCloud: | |
| if runner == nil { | |
| return fmt.Errorf("internal error: standalone runner is required for cloud engine kind but was not provided") | |
| } | |
| if runner.gatewayIP == "" { | |
| runner.gatewayIP = "127.0.0.1" | |
| } | |
| if runner.gatewayPort == 0 { | |
| runner.gatewayPort = standalone.DefaultControllerPortCloud | |
| } | |
| endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort)) | |
| endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud) |
There was a problem hiding this comment.
oh that's true in case of setting MODEL_RUNNER_NO_AUTO_INSTALL
| case types.ModelRunnerEngineKindDesktop: | ||
| endpoint = "http://model-runner.docker.internal/engines/v1/" | ||
| endpoint = "http://model-runner.docker.internal/v1/" | ||
| endpointHost = "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/" |
There was a problem hiding this comment.
For consistency with other cases like ModelRunnerEngineKindMobyManual and to avoid re-implementing the URL construction logic, it's better to use modelRunner.URL("/v1/") to get the host endpoint URL. The modelRunner context is already configured with the correct base URL for host-to-model-runner communication in the Desktop case. This change improves maintainability by centralizing URL construction.
| endpointHost = "http://localhost" + inference.ExperimentalEndpointsPrefix + "/v1/" | |
| endpointHost = modelRunner.URL("/v1/") |
|
|
||
| // NewContextForMockWithKind is a ModelRunnerContext constructor exposed only for the | ||
| // purposes of mock testing with a specific engine kind. | ||
| func NewContextForMockWithKind(client DockerHttpClient, kind types.ModelRunnerEngineKind, rawURLPrefix string) *ModelRunnerContext { |
There was a problem hiding this comment.
nit: we could update NewContextForTest to have the kind parameter instead of creating this new constructor
| case types.ModelRunnerEngineKindCloud: | ||
| fallthrough | ||
| case types.ModelRunnerEngineKindMoby: | ||
| if standalone.gatewayIP == "" { | ||
| standalone.gatewayIP = "127.0.0.1" | ||
| if runner.gatewayIP == "" { | ||
| runner.gatewayIP = "127.0.0.1" | ||
| } | ||
|
|
||
| if standalone.gatewayPort == 0 { | ||
| standalone.gatewayPort = 12434 | ||
| if runner.gatewayPort == 0 { | ||
| runner.gatewayPort = standalone.DefaultControllerPortCloud | ||
| } | ||
|
|
||
| endpoint = "http://" + net.JoinHostPort(standalone.gatewayIP, strconv.Itoa(int(standalone.gatewayPort))) + "/engines/v1/" | ||
| endpoint = makeEndpoint(runner.gatewayIP, int(runner.gatewayPort)) | ||
| endpointHost = makeEndpoint("127.0.0.1", standalone.DefaultControllerPortCloud) |
There was a problem hiding this comment.
oh that's true in case of setting MODEL_RUNNER_NO_AUTO_INSTALL
2 participants
Addresses #446.