RHTAP-6521: Rename WithMCPImage to WithImage and defer validation#57
Conversation
Rename the `App.mcpImage` field to `App.image` and introduce `WithImage()` as the primary functional option, keeping `WithMCPImage()` as a deprecated alias for backward compatibility. Remove the hard validation gate from `setupRootCmd()` so subcommands that don't require a container image (`deploy`, `config`, `topology`, `integration`, `template`, `installer`) work without one. Move the empty-image check into `MCPServer.Complete()`, ensuring the mcp-server subcommand fails clearly at runtime when no image is configured. Update example, documentation (architecture, mcp, getting-started, cli-reference, installer-structure), and add unit tests for the new option, deprecated alias, and `MCPServer.Complete()` validation. Assisted-by: Claude
📝 WalkthroughWalkthroughThis PR unifies the MCP image configuration option from ChangesImage Configuration Unification
Sequence DiagramsequenceDiagram
actor User
participant App as App (Framework)
participant MCPServer as MCPServer<br/>(Subcommand)
participant Complete as Complete()
User->>App: NewAppFromTarball(WithImage("...") or no image)
App->>App: Store image in a.image
Note over App: No validation at setup time
User->>MCPServer: Execute mcp-server command (--image flag optional)
Note over MCPServer: Receives image from app.image<br/>or --image flag override
MCPServer->>Complete: Complete(context)
Complete->>Complete: Check if image is set
alt Image is empty
Complete-->>MCPServer: Error: "WithImage() required"
MCPServer-->>User: Command fails
else Image is set
Complete-->>MCPServer: Success
MCPServer->>User: Proceed with execution
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
framework/app_test.go (1)
23-32: ⚡ Quick win
TestNewApp_WithoutImage_Succeedsdoesn't assert the default image is empty.The test confirms
NewAppsucceeds without an image option, but doesn't assertapp.image == "". This leaves a gap where a future change that defaultedimageto a non-empty value would pass the test silently.✏️ Suggested addition
if app == nil { t.Fatal("expected non-nil App") } + if app.image != "" { + t.Fatalf("expected empty image without WithImage option, got %q", app.image) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/app_test.go` around lines 23 - 32, TestNewApp_WithoutImage_Succeeds currently verifies NewApp returns non-nil but doesn't assert the default image is empty; add an assertion after creation that the App's image field (e.g., app.image or app.Image depending on visibility) equals the empty string and fail the test (using t.Fatalf or t.Fatalf-like) if it's not, so the test explicitly checks the default image behavior of NewApp.internal/subcmd/mcpserver.go (1)
42-49: 💤 Low valueConsider moving the empty-image guard to
Validate()instead ofComplete().Per the documented SubCommand lifecycle (and
cli-reference.mdin this PR),Completeis for loading external data/parsing args, whileValidateis for "required fields are non-empty" checks. The image value is already fully resolved beforePreRunEruns (Cobra applies the--imageflag beforePreRunE), so the guard would be equally correct inValidate()and would better match the lifecycle's stated intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/subcmd/mcpserver.go` around lines 42 - 49, Move the empty-image guard out of Complete() and into Validate(): remove the fmt.Errorf check from MCPServer.Complete and add the same check in MCPServer.Validate (returning the same "container image not configured: use WithImage() option or --image flag" error when m.image == ""), ensuring Complete() only handles loading/parsing and Validate() enforces required-field checks for the MCPServer type.docs/mcp.md (1)
108-108: 💤 Low valueMinor wording ambiguity in the
WithImage()initialization note."WithImage() is no longer required at framework initialization" could be read as "no image is ever needed," since the next sentence mentions
Complete()validation without explicitly stating that some image source is still mandatory. Consider tightening it, e.g.:
WithImage()no longer needs to be provided at framework initialization — image validation is deferred tomcp-server'sComplete(). EitherWithImage()or--imageat runtime must supply a non-empty image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/mcp.md` at line 108, Clarify the wording: update the note about WithImage() so it states that WithImage() is optional at framework initialization because image validation is deferred to mcp-server's Complete(), but that an image source is still required at runtime — either provided via WithImage() earlier or overridden/provided via the --image flag — and that one of those must supply a non-empty image. Mention WithImage(), mcp-server, Complete(), and --image to make the requirement explicit.example/helmet-ex/main.go (1)
29-31: 💤 Low valueMinor naming inconsistency:
buildMCPImage/mcpImagestill carry theMCPprefix.The call site and doc comment were updated to "image" terminology, but the local variable
mcpImageand helperbuildMCPImagestill use the oldMCP-specific prefix. Since this is an example/reference file, a rename would keep it fully consistent with the new naming.✏️ Optional rename
- // 3. Build image reference - mcpImage := buildMCPImage() + // 3. Build image reference + image := buildImage() // 4. Create application with framework options (GitHub URLs via CustomURLProvider; see framework/integrations.go) appIntegrations := framework.StandardIntegrations() appIntegrations = framework.WithURLProvider(appIntegrations, CustomURLProvider{}) app, err := framework.NewAppFromTarball( appCtx, installer.InstallerTarball, cwd, framework.WithIntegrations(appIntegrations...), - framework.WithImage(mcpImage), + framework.WithImage(image), )-// buildMCPImage constructs the container image reference for the installer. +// buildImage constructs the container image reference for the installer. // Uses the commit ID for versioning when available, falls back to 'latest'. -func buildMCPImage() string { +func buildImage() string {Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/helmet-ex/main.go` around lines 29 - 31, Rename the MCP-specific identifiers to the generic "image" naming: change function buildMCPImage to buildImage and all local variable usages of mcpImage to image (also update any related occurrences such as the later variables/uses around buildMCPImage and mcpImage at the other occurrence). Update the function declaration, all calls, and any comments/docstrings that reference buildMCPImage or mcpImage to use buildImage and image so the example file is consistent with the new terminology.framework/options.go (1)
27-30: 💤 Low value
WithMCPImagedoc comment is missing a function-name lead.The Go convention says to add a paragraph beginning with
Deprecated:to the function's existing doc comment. The current comment has only theDeprecated:line with no preceding description starting with the function's name, which is the standard Go doc convention for exported identifiers.✏️ Suggested wording
-// Deprecated: use WithImage instead. +// WithMCPImage sets the container image for the installer. +// +// Deprecated: use WithImage instead. func WithMCPImage(image string) Option {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/options.go` around lines 27 - 30, Update the doc comment for WithMCPImage to follow Go conventions by starting with the function name (e.g., "WithMCPImage returns an Option that sets the image.") and then include the Deprecated paragraph "Deprecated: use WithImage instead." so the comment contains a leading description followed by the deprecation note; ensure the comment remains associated with the WithMCPImage function that currently forwards to WithImage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/mcp.md`:
- Line 108: Clarify the wording: update the note about WithImage() so it states
that WithImage() is optional at framework initialization because image
validation is deferred to mcp-server's Complete(), but that an image source is
still required at runtime — either provided via WithImage() earlier or
overridden/provided via the --image flag — and that one of those must supply a
non-empty image. Mention WithImage(), mcp-server, Complete(), and --image to
make the requirement explicit.
In `@example/helmet-ex/main.go`:
- Around line 29-31: Rename the MCP-specific identifiers to the generic "image"
naming: change function buildMCPImage to buildImage and all local variable
usages of mcpImage to image (also update any related occurrences such as the
later variables/uses around buildMCPImage and mcpImage at the other occurrence).
Update the function declaration, all calls, and any comments/docstrings that
reference buildMCPImage or mcpImage to use buildImage and image so the example
file is consistent with the new terminology.
In `@framework/app_test.go`:
- Around line 23-32: TestNewApp_WithoutImage_Succeeds currently verifies NewApp
returns non-nil but doesn't assert the default image is empty; add an assertion
after creation that the App's image field (e.g., app.image or app.Image
depending on visibility) equals the empty string and fail the test (using
t.Fatalf or t.Fatalf-like) if it's not, so the test explicitly checks the
default image behavior of NewApp.
In `@framework/options.go`:
- Around line 27-30: Update the doc comment for WithMCPImage to follow Go
conventions by starting with the function name (e.g., "WithMCPImage returns an
Option that sets the image.") and then include the Deprecated paragraph
"Deprecated: use WithImage instead." so the comment contains a leading
description followed by the deprecation note; ensure the comment remains
associated with the WithMCPImage function that currently forwards to WithImage.
In `@internal/subcmd/mcpserver.go`:
- Around line 42-49: Move the empty-image guard out of Complete() and into
Validate(): remove the fmt.Errorf check from MCPServer.Complete and add the same
check in MCPServer.Validate (returning the same "container image not configured:
use WithImage() option or --image flag" error when m.image == ""), ensuring
Complete() only handles loading/parsing and Validate() enforces required-field
checks for the MCPServer type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d40d439e-800c-4a9f-8690-d941e2017a43
📒 Files selected for processing (11)
docs/architecture.mddocs/cli-reference.mddocs/getting-started.mddocs/installer-structure.mddocs/mcp.mdexample/helmet-ex/main.goframework/app.goframework/app_test.goframework/options.gointernal/subcmd/mcpserver.gointernal/subcmd/mcpserver_test.go
2 participants
Renames
WithMCPImage()toWithImage()and moves the container image validation from framework initialization toMCPServer.Complete(). Previously,NewApp()andNewAppFromTarball()would fail if no image was provided, blocking all subcommands — even those that don't need one. Now only themcp-serversubcommand requires an image, validated at runtime.WithMCPImage()is retained as a deprecated alias so existing consumers continue to compile without changes.Summary by CodeRabbit
New Features
WithImage()configuration option for specifying container images.Deprecations
WithMCPImage()is deprecated in favor ofWithImage()(backward compatible).Documentation