Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
printAppConfig, the host port is only printed whenca.defaultHostPort > 0, which means an explicitly overridden--portfor apps with no defaultHostPort (e.g. future apps likellmfit) would be hidden; consider checkinghostPort > 0instead so overrides are reflected in the config output. - In
launchContainerApp, a non-zeroportOverrideis silently ignored whenca.containerPort == 0(e.g. forllmfit); consider either validating and erroring on--portusage in that case or logging a message so users know their port setting is not applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `printAppConfig`, the host port is only printed when `ca.defaultHostPort > 0`, which means an explicitly overridden `--port` for apps with no defaultHostPort (e.g. future apps like `llmfit`) would be hidden; consider checking `hostPort > 0` instead so overrides are reflected in the config output.
- In `launchContainerApp`, a non-zero `portOverride` is silently ignored when `ca.containerPort == 0` (e.g. for `llmfit`); consider either validating and erroring on `--port` usage in that case or logging a message so users know their port setting is not applied.
## Individual Comments
### Comment 1
<location path="cmd/cli/commands/launch.go" line_range="223-224" />
<code_context>
+ if ca.containerPort > 0 {
+ cmd.Printf(" Container port: %d\n", ca.containerPort)
+ }
+ if ca.defaultHostPort > 0 {
+ cmd.Printf(" Host port: %d\n", hostPort)
+ }
if ca.envFn != nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** Host port visibility should depend on the effective hostPort value, not defaultHostPort.
Using `ca.defaultHostPort > 0` here can suppress valid user overrides. For example, if `defaultHostPort == 0` but the user passes `--port`, `hostPort` will be non-zero yet the host port line won’t print. This should check `hostPort > 0` instead so the output matches the actual binding.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request adds support for the llmfit container application and updates the launch command to support container apps without port mappings. The changes include conditional logic for port display and Docker flags, along with corresponding tests. Review feedback identifies a logic error in how host ports are displayed when no default is provided and notes a formatting inconsistency in the containerApps configuration map.
|
More work needed I think, this doesn't behave as expected, I tested on macOS got his: $ ./cmd/cli/model-cli launch llmfit |
|
Hi @ericcurtin, can you detail what is expected, like the behavior? If you want the TUI version, we need to make more changes to implement it. Are you expecting that behavior? |
|
Yes, the TUI version |
Changes
fixes #747