From 02440cf00bacc1539a57831b1ee187cb2e22bba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Fernandes?= Date: Mon, 4 May 2026 15:24:03 -0300 Subject: [PATCH] feat(framework): rename WithMCPImage to WithImage and defer validation 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 --- docs/architecture.md | 2 +- docs/cli-reference.md | 2 +- docs/getting-started.md | 4 +- docs/installer-structure.md | 2 +- docs/mcp.md | 10 ++--- example/helmet-ex/main.go | 6 +-- framework/app.go | 12 ++---- framework/app_test.go | 64 ++++++++++++++++++++++++++++++ framework/options.go | 11 ++++-- internal/subcmd/mcpserver.go | 4 ++ internal/subcmd/mcpserver_test.go | 65 +++++++++++++++++++++++++++++++ 11 files changed, 157 insertions(+), 25 deletions(-) create mode 100644 framework/app_test.go create mode 100644 internal/subcmd/mcpserver_test.go diff --git a/docs/architecture.md b/docs/architecture.md index d5a13e476..590000548 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -169,7 +169,7 @@ appCtx := api.NewAppContext("helmet-ex", ```go app, _ := framework.NewAppFromTarball(appCtx, installer.InstallerTarball, cwd, framework.WithIntegrations(appIntegrations...), - framework.WithMCPImage(mcpImage), + framework.WithImage(mcpImage), ) ``` diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 6e1fdb113..ad5fa5c97 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -189,7 +189,7 @@ helmet-ex mcp-server [flags] | Flag | Description | |------|-------------| -| `--image` | Container image for installer (overrides default from `WithMCPImage()`) | +| `--image` | Container image for installer (overrides default from `WithImage()`) | **Behavior:** - Reads `instructions.md` from installer filesystem as server instructions diff --git a/docs/getting-started.md b/docs/getting-started.md index 1d3504bef..b4e673984 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -101,7 +101,7 @@ app, err := framework.NewAppFromTarball( installer.InstallerTarball, cwd, framework.WithIntegrations(appIntegrations...), - framework.WithMCPImage(mcpImage), + framework.WithImage(mcpImage), ) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) @@ -118,7 +118,7 @@ if err := app.Run(); err != nil { - `api.NewAppContext()` creates application metadata with functional options - `framework.NewAppFromTarball()` constructs the app from the embedded tarball - The `cwd` parameter enables the [overlay filesystem](installer-structure.md#overlay-filesystem) for development -- `framework.WithMCPImage()` sets the container image for [MCP Job-based deployments](mcp.md#container-image-for-job-based-deployment) +- `framework.WithImage()` sets the container image for [MCP Job-based deployments](mcp.md#container-image-for-job-based-deployment) ## Building diff --git a/docs/installer-structure.md b/docs/installer-structure.md index 01d7a63a7..ae9fb0647 100644 --- a/docs/installer-structure.md +++ b/docs/installer-structure.md @@ -194,7 +194,7 @@ func NewAppFromTarball( | `appCtx` | `*api.AppContext` | Application metadata (name, version, namespace, descriptions) | | `tarball` | `[]byte` | Embedded installer tarball bytes | | `cwd` | `string` | Current working directory for local filesystem overlay | -| `opts` | `...Option` | Functional options (integrations, MCP image, etc.) | +| `opts` | `...Option` | Functional options (integrations, image, etc.) | **Internal flow**: 1. Convert tarball bytes to `fs.FS` via `NewTarFS(tarball)` diff --git a/docs/mcp.md b/docs/mcp.md index 53ebd1060..f124de1b6 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -12,7 +12,7 @@ Configure your MCP client to run your installer binary with the `mcp-server` sub helmet-ex mcp-server ``` -The MCP server requires a [container image](#container-image-for-job-based-deployment) for Job-based deployments, configured via `WithMCPImage()` at framework initialization or overridden with `--image` at runtime. +The MCP server requires a [container image](#container-image-for-job-based-deployment) for Job-based deployments, configured via `WithImage()` at framework initialization or overridden with `--image` at runtime. ### Client Configuration @@ -74,7 +74,7 @@ When `deploy` is invoked, the MCP server — authenticated via the user's kubeco 1. **`ServiceAccount`** named `{appName}` in the installer namespace (via [server-side apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/)) 2. **`ClusterRoleBinding`** named `{appName}`, binding the `ServiceAccount` to the `cluster-admin` `ClusterRole` (via server-side apply) 3. **`Job`** named `{appName}-deploy-job` (via `Create` — only one Job is allowed; use `force: true` to replace an existing one) with: - - Container image: the consumer's installer application image (from `WithMCPImage()` or `--image`) + - Container image: the consumer's installer application image (from `WithImage()` or `--image`) - Args: `["deploy"]` (with optional `--verbose`, `--dry-run`) - Env: `KUBECONFIG=""` (forces [in-cluster authentication](https://kubernetes.io/docs/reference/access-authn-authz/authentication/#service-account-tokens)) - RestartPolicy: `Never`, BackoffLimit: `0` @@ -105,11 +105,11 @@ make image IMAGE_REPOSITORY="quay.io/redhat-appstudio" make image-push ``` -`WithMCPImage()` is **required** at framework initialization — the framework returns an error if no image is configured. This value becomes the default for the optional `--image` flag, which overrides it at runtime. +`WithImage()` is no longer required at framework initialization. The `mcp-server` subcommand validates the image at runtime via `Complete()`. This value becomes the default for the optional `--image` flag, which overrides it at runtime. ```go -// At framework initialization (required) — from helmet-ex/main.go -framework.WithMCPImage("quay.io/redhat-appstudio/helmet-ex:latest") +// At framework initialization — from helmet-ex/main.go +framework.WithImage("quay.io/redhat-appstudio/helmet-ex:latest") ``` ```sh diff --git a/example/helmet-ex/main.go b/example/helmet-ex/main.go index 06f581c84..365e4cc19 100644 --- a/example/helmet-ex/main.go +++ b/example/helmet-ex/main.go @@ -26,7 +26,7 @@ func main() { os.Exit(1) } - // 3. Build MCP image reference + // 3. Build image reference mcpImage := buildMCPImage() // 4. Create application with framework options (GitHub URLs via CustomURLProvider; see framework/integrations.go) @@ -37,7 +37,7 @@ func main() { installer.InstallerTarball, cwd, framework.WithIntegrations(appIntegrations...), - framework.WithMCPImage(mcpImage), + framework.WithImage(mcpImage), ) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) @@ -78,7 +78,7 @@ operators, storage, networking, integrations, and product layers.`), ) } -// buildMCPImage constructs the container image reference for the MCP server. +// buildMCPImage constructs the container image reference for the installer. // Uses the commit ID for versioning when available, falls back to 'latest'. func buildMCPImage() string { mcpImage := "quay.io/redhat-appstudio/helmet-ex" diff --git a/framework/app.go b/framework/app.go index 2c6bcb339..51c1343af 100644 --- a/framework/app.go +++ b/framework/app.go @@ -30,7 +30,7 @@ type App struct { kube *k8s.Kube // kubernetes client mcpToolsBuilder mcptools.MCPToolsBuilder // tools builder - mcpImage string // installer image + image string // installer image installerTarball []byte // embedded installer tarball } @@ -94,18 +94,12 @@ func (a *App) setupRootCmd() error { mcpBuilder = subcmd.StandardMCPToolsBuilder() } - // Validate MCP image is configured. - if a.mcpImage == "" { - return fmt.Errorf( - "MCP server image not configured: use WithMCPImage() option") - } - // Other subcommands via api.Runner. subs := []api.SubCommand{ subcmd.NewConfig(a.AppCtx, runCtx, a.flags), subcmd.NewDeploy(a.AppCtx, runCtx, a.flags, a.integrationManager, a.installerTarball), subcmd.NewInstaller(a.AppCtx, runCtx, a.flags, a.installerTarball), - subcmd.NewMCPServer(a.AppCtx, runCtx, a.flags, a.integrationManager, mcpBuilder, a.mcpImage), + subcmd.NewMCPServer(a.AppCtx, runCtx, a.flags, a.integrationManager, mcpBuilder, a.image), subcmd.NewTemplate(a.AppCtx, runCtx, a.flags, a.installerTarball), subcmd.NewTopology(a.AppCtx, runCtx), } @@ -154,7 +148,7 @@ func NewApp( // - appCtx: Application metadata (name, version, etc.) // - tarball: Embedded installer tarball bytes // - cwd: Current working directory for local filesystem overlay -// - opts: Additional runtime options (integrations, MCP image, etc.) +// - opts: Additional runtime options (integrations, image, etc.) // // The function creates an overlay filesystem combining the embedded tarball // contents with the local filesystem at cwd, then initializes the App. diff --git a/framework/app_test.go b/framework/app_test.go new file mode 100644 index 000000000..f65c4a8bb --- /dev/null +++ b/framework/app_test.go @@ -0,0 +1,64 @@ +package framework + +import ( + "os" + "testing" + + "github.com/redhat-appstudio/helmet/api" + "github.com/redhat-appstudio/helmet/internal/chartfs" +) + +func testAppContext() *api.AppContext { + return api.NewAppContext( + "helmet-test", + api.WithNamespace("test-ns"), + ) +} + +func testChartFS(t *testing.T) *chartfs.ChartFS { + t.Helper() + return chartfs.New(os.DirFS("../test/charts")) +} + +func TestNewApp_WithoutImage_Succeeds(t *testing.T) { + t.Parallel() + app, err := NewApp(testAppContext(), testChartFS(t)) + if err != nil { + t.Fatalf("NewApp without WithImage should succeed, got: %v", err) + } + if app == nil { + t.Fatal("expected non-nil App") + } +} + +func TestWithImage_SetsImage(t *testing.T) { + t.Parallel() + app, err := NewApp(testAppContext(), testChartFS(t), + WithImage("test:latest"), + ) + if err != nil { + t.Fatalf("NewApp with WithImage should succeed, got: %v", err) + } + if app == nil { + t.Fatal("expected non-nil App") + } + if app.image != "test:latest" { + t.Fatalf("expected image %q, got %q", "test:latest", app.image) + } +} + +func TestWithMCPImage_DeprecatedAlias(t *testing.T) { + t.Parallel() + app, err := NewApp(testAppContext(), testChartFS(t), + WithMCPImage("deprecated:latest"), + ) + if err != nil { + t.Fatalf("NewApp with WithMCPImage should succeed, got: %v", err) + } + if app == nil { + t.Fatal("expected non-nil App") + } + if app.image != "deprecated:latest" { + t.Fatalf("expected image %q, got %q", "deprecated:latest", app.image) + } +} diff --git a/framework/options.go b/framework/options.go index 9406e4787..624c8c69e 100644 --- a/framework/options.go +++ b/framework/options.go @@ -17,13 +17,18 @@ func WithIntegrations(modules ...api.IntegrationModule) Option { } } -// WithMCPImage sets the container image for the MCP server. -func WithMCPImage(image string) Option { +// WithImage sets the container image for the installer. +func WithImage(image string) Option { return func(a *App) { - a.mcpImage = image + a.image = image } } +// Deprecated: use WithImage instead. +func WithMCPImage(image string) Option { + return WithImage(image) +} + // WithMCPToolsBuilder sets the MCP tools builder for the application. func WithMCPToolsBuilder(builder mcptools.MCPToolsBuilder) Option { return func(a *App) { diff --git a/internal/subcmd/mcpserver.go b/internal/subcmd/mcpserver.go index 1a084e17c..c9c480b0a 100644 --- a/internal/subcmd/mcpserver.go +++ b/internal/subcmd/mcpserver.go @@ -41,6 +41,10 @@ func (m *MCPServer) Cmd() *cobra.Command { // Complete implements api.SubCommand. func (m *MCPServer) Complete(_ []string) error { + if m.image == "" { + return fmt.Errorf( + "container image not configured: use WithImage() option or --image flag") + } return nil } diff --git a/internal/subcmd/mcpserver_test.go b/internal/subcmd/mcpserver_test.go new file mode 100644 index 000000000..ce499d00f --- /dev/null +++ b/internal/subcmd/mcpserver_test.go @@ -0,0 +1,65 @@ +package subcmd + +import ( + "testing" + + "github.com/onsi/gomega" + "github.com/redhat-appstudio/helmet/internal/flags" + "github.com/redhat-appstudio/helmet/internal/mcptools" +) + +func noopMCPToolsBuilder(_ mcptools.MCPToolsContext) ([]mcptools.Interface, error) { + return nil, nil +} + +func TestMCPServer_Complete_EmptyImage_ReturnsError(t *testing.T) { + t.Parallel() + g := gomega.NewWithT(t) + + appCtx := testAppContext() + runCtx := testRunContext(t) + manager := testManager(t, runCtx) + + m := NewMCPServer(appCtx, runCtx, flags.NewFlags(), manager, noopMCPToolsBuilder, "") + err := m.Complete(nil) + g.Expect(err).To(gomega.HaveOccurred()) + g.Expect(err.Error()).To(gomega.ContainSubstring("WithImage()")) +} + +func TestMCPServer_Complete_WithImage_Succeeds(t *testing.T) { + t.Parallel() + g := gomega.NewWithT(t) + + appCtx := testAppContext() + runCtx := testRunContext(t) + manager := testManager(t, runCtx) + + m := NewMCPServer(appCtx, runCtx, flags.NewFlags(), manager, noopMCPToolsBuilder, "test:latest") + g.Expect(m.Complete(nil)).ToNot(gomega.HaveOccurred()) +} + +func TestMCPServer_Complete_ImageOverriddenByFlag(t *testing.T) { + t.Parallel() + g := gomega.NewWithT(t) + + appCtx := testAppContext() + runCtx := testRunContext(t) + manager := testManager(t, runCtx) + + m := NewMCPServer(appCtx, runCtx, flags.NewFlags(), manager, noopMCPToolsBuilder, "") + err := m.cmd.PersistentFlags().Set("image", "override:latest") + g.Expect(err).ToNot(gomega.HaveOccurred()) + g.Expect(m.Complete(nil)).ToNot(gomega.HaveOccurred()) +} + +func TestMCPServer_Validate_AlwaysSucceeds(t *testing.T) { + t.Parallel() + g := gomega.NewWithT(t) + + appCtx := testAppContext() + runCtx := testRunContext(t) + manager := testManager(t, runCtx) + + m := NewMCPServer(appCtx, runCtx, flags.NewFlags(), manager, noopMCPToolsBuilder, "test:latest") + g.Expect(m.Validate()).ToNot(gomega.HaveOccurred()) +}