Conversation
- Allow `docker model launch` with no args to list supported apps with descriptions and install status - Add `--config` flag to print configuration without launching - Properly handle `--` separator for passing extra args to integrations - Reject unexpected extra args without `--` separator with helpful error - Add app descriptions for all supported apps - Update documentation and tests Signed-off-by: Eric Curtin <[email protected]>
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 significantly enhances the 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. Changelog
Activity
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
|
![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 significantly improves the docker model launch command by adding the ability to list supported applications, view their configurations, and providing a more robust argument parsing for passing arguments to the launched applications. The changes are well-implemented and include comprehensive tests and documentation updates. I have a couple of minor suggestions to improve the consistency of a user-facing usage string.
![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 3 issues, and left some high level feedback:
- In
newLaunchCmd'sRunE, when usingArgsLenAtDash,appArgsshould probably beargs[dashIdx+1:]instead ofargs[dashIdx:]so that the literal--separator is not forwarded to the app alongside its arguments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `newLaunchCmd`'s `RunE`, when using `ArgsLenAtDash`, `appArgs` should probably be `args[dashIdx+1:]` instead of `args[dashIdx:]` so that the literal `--` separator is not forwarded to the app alongside its arguments.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/launch.go:123` </location>
<code_context>
app := strings.ToLower(args[0])
- appArgs := args[1:]
+
+ // Extract passthrough args using -- separator
+ var appArgs []string
+ dashIdx := cmd.ArgsLenAtDash()
</code_context>
<issue_to_address>
**issue (bug_risk):** Extra positional args before `--` are silently ignored instead of being rejected.
When `ArgsLenAtDash()` > 1, any args between the app name and `--` (e.g., `docker model launch claude foo -- bar`) are dropped, so only `bar` reaches the app and `foo` is silently ignored. This differs from the behavior without `--`, where extra args cause an error. Consider requiring `dashIdx == 1` and erroring otherwise, or clearly documenting that only args after `--` are forwarded and earlier ones are ignored.
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/launch.go:191-200` </location>
<code_context>
+func printAppConfig(cmd *cobra.Command, app string, ep engineEndpoints) error {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `--config` output does not reflect user overrides like `--image` or `--port`.
`printAppConfig` always shows `ca.defaultImage` and `ca.defaultHostPort`, even when `--image` or `--port` overrides are provided with `--config`. This can cause the printed config to differ from what `launchContainerApp` will actually use. Please pass the resolved image/port (including overrides) into `printAppConfig` so its output reflects the real launch configuration.
Suggested implementation:
```golang
// printAppConfig prints the configuration that would be used for the given app.
func printAppConfig(cmd *cobra.Command, app string, ep engineEndpoints, image string, hostPort int) error {
if ca, ok := containerApps[app]; ok {
cmd.Printf("Configuration for %s (container app):\n", app)
cmd.Printf(" Image: %s\n", image)
cmd.Printf(" Container port: %d\n", ca.containerPort)
cmd.Printf(" Host port: %d\n", hostPort)
if ca.envFn != nil {
cmd.Printf(" Environment:\n")
for _, e := range ca.envFn(ep.container) {
cmd.Printf(" %s\n", e)
}
```
To fully implement the behavior you described, you will also need to:
1. Update every call to `printAppConfig` to use the new signature:
- Compute the final image string (respecting `--image` flag overrides and any defaults used by `launchContainerApp`).
- Compute the final host port (respecting `--port` flag overrides and any defaults).
- Pass those values as the `image` and `hostPort` arguments when calling `printAppConfig`.
For example, a call that previously looked like:
```go
if err := printAppConfig(cmd, app, ep); err != nil { ... }
```
should become something like:
```go
resolvedImage := imageFlagValueOrDefault // same value used by launchContainerApp
resolvedHostPort := hostPortFlagValueOrDefault
if err := printAppConfig(cmd, app, ep, resolvedImage, resolvedHostPort); err != nil { ... }
```
2. Ensure that the logic computing `resolvedImage` and `resolvedHostPort` is exactly the same as that used by `launchContainerApp`, so the printed configuration always matches the actual launch configuration.
</issue_to_address>
### Comment 3
<location> `cmd/cli/commands/launch.go:186` </location>
<code_context>
+ cmd.Printf(" %-15s %s%s\n", name, desc, status)
+ }
+ cmd.Println()
+ cmd.Println("Usage: docker model launch APP [-- APP_ARGS...]")
+ return nil
+}
</code_context>
<issue_to_address>
**nitpick:** Usage line in `listSupportedApps` is inconsistent with the new `[APP]` optional syntax.
This usage line implies `APP` is required, unlike the command’s `Use`/docs, which show `[APP]`. Please align this string with the canonical usage (e.g., by reusing `Use` or changing `APP` to `[APP]`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| app := strings.ToLower(args[0]) | ||
| appArgs := args[1:] | ||
|
|
||
| // Extract passthrough args using -- separator |
There was a problem hiding this comment.
issue (bug_risk): Extra positional args before -- are silently ignored instead of being rejected.
When ArgsLenAtDash() > 1, any args between the app name and -- (e.g., docker model launch claude foo -- bar) are dropped, so only bar reaches the app and foo is silently ignored. This differs from the behavior without --, where extra args cause an error. Consider requiring dashIdx == 1 and erroring otherwise, or clearly documenting that only args after -- are forwarded and earlier ones are ignored.
| func printAppConfig(cmd *cobra.Command, app string, ep engineEndpoints) error { | ||
| if ca, ok := containerApps[app]; ok { | ||
| cmd.Printf("Configuration for %s (container app):\n", app) | ||
| cmd.Printf(" Image: %s\n", ca.defaultImage) | ||
| cmd.Printf(" Container port: %d\n", ca.containerPort) | ||
| cmd.Printf(" Host port: %d\n", ca.defaultHostPort) | ||
| if ca.envFn != nil { | ||
| cmd.Printf(" Environment:\n") | ||
| for _, e := range ca.envFn(ep.container) { | ||
| cmd.Printf(" %s\n", e) |
There was a problem hiding this comment.
suggestion (bug_risk): --config output does not reflect user overrides like --image or --port.
printAppConfig always shows ca.defaultImage and ca.defaultHostPort, even when --image or --port overrides are provided with --config. This can cause the printed config to differ from what launchContainerApp will actually use. Please pass the resolved image/port (including overrides) into printAppConfig so its output reflects the real launch configuration.
Suggested implementation:
// printAppConfig prints the configuration that would be used for the given app.
func printAppConfig(cmd *cobra.Command, app string, ep engineEndpoints, image string, hostPort int) error {
if ca, ok := containerApps[app]; ok {
cmd.Printf("Configuration for %s (container app):\n", app)
cmd.Printf(" Image: %s\n", image)
cmd.Printf(" Container port: %d\n", ca.containerPort)
cmd.Printf(" Host port: %d\n", hostPort)
if ca.envFn != nil {
cmd.Printf(" Environment:\n")
for _, e := range ca.envFn(ep.container) {
cmd.Printf(" %s\n", e)
}To fully implement the behavior you described, you will also need to:
-
Update every call to
printAppConfigto use the new signature:- Compute the final image string (respecting
--imageflag overrides and any defaults used bylaunchContainerApp). - Compute the final host port (respecting
--portflag overrides and any defaults). - Pass those values as the
imageandhostPortarguments when callingprintAppConfig.
For example, a call that previously looked like:
if err := printAppConfig(cmd, app, ep); err != nil { ... }
should become something like:
resolvedImage := imageFlagValueOrDefault // same value used by launchContainerApp resolvedHostPort := hostPortFlagValueOrDefault if err := printAppConfig(cmd, app, ep, resolvedImage, resolvedHostPort); err != nil { ... }
- Compute the final image string (respecting
-
Ensure that the logic computing
resolvedImageandresolvedHostPortis exactly the same as that used bylaunchContainerApp, so the printed configuration always matches the actual launch configuration.
|
I think its worth to add the Sourcery suggestion:
|
3 participants
docker model launchwith no args to list supported apps with descriptions and install status--configflag to print configuration without launching--separator for passing extra args to integrations--separator with helpful error