From 1cb3a4a66db3693de8c3fac952f1d9fef7fbcd64 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:00:00 +0300 Subject: [PATCH 1/9] fix(commands): return templates from processModelineAndUpdateGlobals Change processModelineAndUpdateGlobals to return ([]string, error) instead of just error, extracting the Templates field from the parsed modeline. This enables callers (particularly the apply command) to access template paths from the modeline comment, which were previously parsed but silently discarded. Update all three call sites to handle the new return signature. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 4 ++- pkg/commands/root.go | 11 +++--- pkg/commands/root_test.go | 62 ++++++++++++++++++++++++++++++++ pkg/commands/talosctl_wrapper.go | 2 +- pkg/commands/upgrade_handler.go | 2 +- 5 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 pkg/commands/root_test.go diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 86d770b..9b44fbb 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -104,9 +104,11 @@ func apply(args []string) error { } for _, configFile := range expandedFiles { - if err := processModelineAndUpdateGlobals(configFile, applyCmdFlags.nodesFromArgs, applyCmdFlags.endpointsFromArgs, true); err != nil { + modelineTemplates, err := processModelineAndUpdateGlobals(configFile, applyCmdFlags.nodesFromArgs, applyCmdFlags.endpointsFromArgs, true) + if err != nil { return err } + _ = modelineTemplates // TODO: used in template rendering path below // Resolve secrets.yaml path relative to project root if not absolute withSecretsPath := ResolveSecretsPath(applyCmdFlags.withSecrets) diff --git a/pkg/commands/root.go b/pkg/commands/root.go index 1f206e5..8d28395 100644 --- a/pkg/commands/root.go +++ b/pkg/commands/root.go @@ -219,13 +219,15 @@ func DetectRootForTemplate(templatePath string) (string, error) { return DetectProjectRootForFile(templatePath) } -func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endpointsFromArgs bool, owerwrite bool) error { +func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endpointsFromArgs bool, owerwrite bool) ([]string, error) { modelineConfig, err := modeline.ReadAndParseModeline(configFile) if err != nil { fmt.Printf("Warning: modeline parsing failed: %v\n", err) - return err + return nil, err } + var templates []string + // Update global settings if modeline was successfully parsed if modelineConfig != nil { if !nodesFromArgs && len(modelineConfig.Nodes) > 0 { @@ -242,11 +244,12 @@ func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endp GlobalArgs.Endpoints = append(GlobalArgs.Endpoints, modelineConfig.Endpoints...) } } + templates = modelineConfig.Templates } if len(GlobalArgs.Nodes) < 1 { - return errors.New("nodes are not set for the command: please use `--nodes` flag or configuration file to set the nodes to run the command against") + return nil, errors.New("nodes are not set for the command: please use `--nodes` flag or configuration file to set the nodes to run the command against") } - return nil + return templates, nil } diff --git a/pkg/commands/root_test.go b/pkg/commands/root_test.go new file mode 100644 index 0000000..aae3ed6 --- /dev/null +++ b/pkg/commands/root_test.go @@ -0,0 +1,62 @@ +package commands + +import ( + "os" + "path/filepath" + "testing" +) + +func TestProcessModelineAndUpdateGlobals_ReturnsTemplates(t *testing.T) { + // Create temp file with modeline containing templates + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "node0.yaml") + content := `# talm: nodes=["10.0.0.1"], endpoints=["10.0.0.1"], templates=["templates/controlplane.yaml"] +machine: + type: controlplane +` + if err := os.WriteFile(configFile, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp file: %v", err) + } + + // Reset global state + GlobalArgs.Nodes = []string{} + GlobalArgs.Endpoints = []string{} + + templates, err := processModelineAndUpdateGlobals(configFile, false, false, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(templates) != 1 { + t.Fatalf("expected 1 template, got %d", len(templates)) + } + if templates[0] != "templates/controlplane.yaml" { + t.Errorf("expected templates/controlplane.yaml, got %s", templates[0]) + } +} + +func TestProcessModelineAndUpdateGlobals_NoTemplates(t *testing.T) { + // Create temp file with modeline without templates + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "node0.yaml") + content := `# talm: nodes=["10.0.0.1"], endpoints=["10.0.0.1"] +machine: + type: controlplane +` + if err := os.WriteFile(configFile, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp file: %v", err) + } + + // Reset global state + GlobalArgs.Nodes = []string{} + GlobalArgs.Endpoints = []string{} + + templates, err := processModelineAndUpdateGlobals(configFile, false, false, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(templates) != 0 { + t.Errorf("expected 0 templates, got %d: %v", len(templates), templates) + } +} diff --git a/pkg/commands/talosctl_wrapper.go b/pkg/commands/talosctl_wrapper.go index f8fd7ba..f331680 100644 --- a/pkg/commands/talosctl_wrapper.go +++ b/pkg/commands/talosctl_wrapper.go @@ -127,7 +127,7 @@ func wrapTalosCommand(cmd *cobra.Command, cmdName string) *cobra.Command { } for _, configFile := range expandedFiles { - if err := processModelineAndUpdateGlobals(configFile, nodesFromArgs, endpointsFromArgs, false); err != nil { + if _, err := processModelineAndUpdateGlobals(configFile, nodesFromArgs, endpointsFromArgs, false); err != nil { return err } } diff --git a/pkg/commands/upgrade_handler.go b/pkg/commands/upgrade_handler.go index 21130a9..aa7849a 100644 --- a/pkg/commands/upgrade_handler.go +++ b/pkg/commands/upgrade_handler.go @@ -55,7 +55,7 @@ func wrapUpgradeCommand(wrappedCmd *cobra.Command, originalRunE func(*cobra.Comm // Process modeline to update GlobalArgs nodesFromArgs := len(GlobalArgs.Nodes) > 0 endpointsFromArgs := len(GlobalArgs.Endpoints) > 0 - if err := processModelineAndUpdateGlobals(configFile, nodesFromArgs, endpointsFromArgs, true); err != nil { + if _, err := processModelineAndUpdateGlobals(configFile, nodesFromArgs, endpointsFromArgs, true); err != nil { return fmt.Errorf("failed to process modeline: %w", err) } From 42bc0c3b4d7fa535db3ed1498a7868c567b467e8 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:01:59 +0300 Subject: [PATCH 2/9] feat(commands): support modeline templates in apply command When the modeline contains a templates field, the apply command now renders templates via the Helm engine (engine.Render) before applying the config, matching the behavior of 'talm template -f | talm apply -f -'. Previously, the apply command ignored the templates field from the modeline and used the node file as a raw patch against an empty config bundle. This caused missing cluster configuration (endpoint, clusterName, etc.) and Talos validation failures. The existing direct-patch flow is preserved when no templates are present in the modeline. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 107 +++++++++++++++++++++++++++++++------ pkg/commands/apply_test.go | 63 ++++++++++++++++++++++ 2 files changed, 153 insertions(+), 17 deletions(-) create mode 100644 pkg/commands/apply_test.go diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 9b44fbb..e71b73d 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -18,6 +18,9 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" + "strings" "time" "github.com/cozystack/talm/pkg/engine" @@ -108,27 +111,45 @@ func apply(args []string) error { if err != nil { return err } - _ = modelineTemplates // TODO: used in template rendering path below - // Resolve secrets.yaml path relative to project root if not absolute withSecretsPath := ResolveSecretsPath(applyCmdFlags.withSecrets) - opts := engine.Options{ - TalosVersion: applyCmdFlags.talosVersion, - WithSecrets: withSecretsPath, - KubernetesVersion: applyCmdFlags.kubernetesVersion, - Debug: applyCmdFlags.debug, - } - - patches := []string{"@" + configFile} - configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) - if err != nil { - return fmt.Errorf("full config processing error: %s", err) - } + var result []byte + if shouldUseTemplateRendering(modelineTemplates) { + // Template rendering path: render templates via Helm engine, then apply node file as patch + resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) + opts := engine.Options{ + TalosVersion: applyCmdFlags.talosVersion, + WithSecrets: withSecretsPath, + KubernetesVersion: applyCmdFlags.kubernetesVersion, + Debug: applyCmdFlags.debug, + Full: true, + Offline: true, + Root: Config.RootDir, + TemplateFiles: resolvedTemplates, + } + result, err = engine.Render(ctx, nil, opts) + if err != nil { + return fmt.Errorf("template rendering error: %s", err) + } + } else { + // Direct patch path: apply config file as patch against empty bundle + opts := engine.Options{ + TalosVersion: applyCmdFlags.talosVersion, + WithSecrets: withSecretsPath, + KubernetesVersion: applyCmdFlags.kubernetesVersion, + Debug: applyCmdFlags.debug, + } + patches := []string{"@" + configFile} + configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) + if err != nil { + return fmt.Errorf("full config processing error: %s", err) + } - result, err := engine.SerializeConfiguration(configBundle, machineType) - if err != nil { - return fmt.Errorf("error serializing configuration: %s", err) + result, err = engine.SerializeConfiguration(configBundle, machineType) + if err != nil { + return fmt.Errorf("error serializing configuration: %s", err) + } } withClient := func(f func(ctx context.Context, c *client.Client) error) error { @@ -188,6 +209,58 @@ func apply(args []string) error { return nil } +// shouldUseTemplateRendering returns true if templates are available from modeline +// and should be rendered via the Helm engine before applying. +func shouldUseTemplateRendering(templates []string) bool { + return len(templates) > 0 +} + +// resolveTemplatePaths resolves template file paths relative to the project root, +// normalizing them for the Helm engine (forward slashes). +func resolveTemplatePaths(templates []string, rootDir string) []string { + resolved := make([]string, len(templates)) + absRootDir, rootErr := filepath.Abs(rootDir) + if rootErr != nil { + for i, p := range templates { + resolved[i] = engine.NormalizeTemplatePath(p) + } + return resolved + } + + for i, templatePath := range templates { + var absTemplatePath string + if filepath.IsAbs(templatePath) { + absTemplatePath = templatePath + } else { + var absErr error + absTemplatePath, absErr = filepath.Abs(templatePath) + if absErr != nil { + resolved[i] = engine.NormalizeTemplatePath(templatePath) + continue + } + } + relPath, relErr := filepath.Rel(absRootDir, absTemplatePath) + if relErr != nil { + resolved[i] = engine.NormalizeTemplatePath(templatePath) + continue + } + relPath = filepath.Clean(relPath) + if strings.HasPrefix(relPath, "..") { + templateName := filepath.Base(templatePath) + possiblePath := filepath.Join("templates", templateName) + fullPath := filepath.Join(absRootDir, possiblePath) + if _, statErr := os.Stat(fullPath); statErr == nil { + relPath = possiblePath + } else { + resolved[i] = engine.NormalizeTemplatePath(templatePath) + continue + } + } + resolved[i] = engine.NormalizeTemplatePath(relPath) + } + return resolved +} + func init() { applyCmd.Flags().BoolVarP(&applyCmdFlags.insecure, "insecure", "i", false, "apply using the insecure (encrypted with no auth) maintenance service") applyCmd.Flags().StringSliceVarP(&applyCmdFlags.configFiles, "file", "f", nil, "specify config files or patches in a YAML file (can specify multiple)") diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go new file mode 100644 index 0000000..aba3baf --- /dev/null +++ b/pkg/commands/apply_test.go @@ -0,0 +1,63 @@ +package commands + +import ( + "testing" +) + +func TestShouldUseTemplateRendering(t *testing.T) { + tests := []struct { + name string + templates []string + want bool + }{ + {"nil templates", nil, false}, + {"empty templates", []string{}, false}, + {"one template", []string{"templates/controlplane.yaml"}, true}, + {"multiple templates", []string{"templates/controlplane.yaml", "templates/worker.yaml"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shouldUseTemplateRendering(tt.templates) + if got != tt.want { + t.Errorf("shouldUseTemplateRendering(%v) = %v, want %v", tt.templates, got, tt.want) + } + }) + } +} + +func TestResolveTemplatePaths(t *testing.T) { + tests := []struct { + name string + templates []string + rootDir string + want []string + }{ + { + name: "simple relative path", + templates: []string{"templates/controlplane.yaml"}, + rootDir: "", + want: []string{"templates/controlplane.yaml"}, + }, + { + name: "multiple paths", + templates: []string{"templates/controlplane.yaml", "templates/worker.yaml"}, + rootDir: "", + want: []string{"templates/controlplane.yaml", "templates/worker.yaml"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := resolveTemplatePaths(tt.templates, tt.rootDir) + if len(got) != len(tt.want) { + t.Fatalf("resolveTemplatePaths() returned %d items, want %d", len(got), len(tt.want)) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("resolveTemplatePaths()[%d] = %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} From db218cc015fd7faac0b4b3e4aa93071d30e2dc9d Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:11:39 +0300 Subject: [PATCH 3/9] fix(commands): add node context to skip-verify path in apply Extract wrapWithNodeContext helper that resolves nodes from GlobalArgs or client config context and injects them into the gRPC context via client.WithNodes. Previously, the --skip-verify path called WithClientSkipVerify directly, bypassing the node resolution logic. This caused TLS verification to not actually be skipped because the gRPC request context had no target nodes. Now both the skip-verify and normal paths use the same wrapWithNodeContext wrapper, differing only in the underlying connection method. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 41 +++++++++++++++++++------------- pkg/commands/apply_test.go | 48 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 16 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index e71b73d..de7d008 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -16,7 +16,6 @@ package commands import ( "context" - "errors" "fmt" "os" "path/filepath" @@ -157,24 +156,13 @@ func apply(args []string) error { return WithClientMaintenance(applyCmdFlags.certFingerprints, f) } + wrappedF := wrapWithNodeContext(f) + if GlobalArgs.SkipVerify { - return WithClientSkipVerify(f) + return WithClientSkipVerify(wrappedF) } - return WithClientNoNodes(func(ctx context.Context, cli *client.Client) error { - if len(GlobalArgs.Nodes) < 1 { - configContext := cli.GetConfigContext() - if configContext == nil { - return errors.New("failed to resolve config context") - } - - GlobalArgs.Nodes = configContext.Nodes - } - - ctx = client.WithNodes(ctx, GlobalArgs.Nodes...) - - return f(ctx, cli) - }) + return WithClientNoNodes(wrappedF) } err = withClient(func(ctx context.Context, c *client.Client) error { @@ -209,6 +197,27 @@ func apply(args []string) error { return nil } +// wrapWithNodeContext wraps a client action function to resolve and inject node +// context. If GlobalArgs.Nodes is already set, uses those directly. Otherwise, +// attempts to resolve nodes from the client's config context. +func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) func(ctx context.Context, c *client.Client) error { + return func(ctx context.Context, c *client.Client) error { + if len(GlobalArgs.Nodes) < 1 { + if c == nil { + return fmt.Errorf("failed to resolve config context: no client available") + } + configContext := c.GetConfigContext() + if configContext == nil { + return fmt.Errorf("failed to resolve config context") + } + GlobalArgs.Nodes = configContext.Nodes + } + + ctx = client.WithNodes(ctx, GlobalArgs.Nodes...) + return f(ctx, c) + } +} + // shouldUseTemplateRendering returns true if templates are available from modeline // and should be rendered via the Helm engine before applying. func shouldUseTemplateRendering(templates []string) bool { diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index aba3baf..6b99991 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -1,7 +1,10 @@ package commands import ( + "context" "testing" + + "github.com/siderolabs/talos/pkg/machinery/client" ) func TestShouldUseTemplateRendering(t *testing.T) { @@ -61,3 +64,48 @@ func TestResolveTemplatePaths(t *testing.T) { }) } } + +func TestWrapWithNodeContext_SetsNodesInContext(t *testing.T) { + origNodes := GlobalArgs.Nodes + defer func() { GlobalArgs.Nodes = origNodes }() + + GlobalArgs.Nodes = []string{"10.0.0.1", "10.0.0.2"} + + var capturedCtx context.Context + inner := func(ctx context.Context, c *client.Client) error { + capturedCtx = ctx + return nil + } + + wrapped := wrapWithNodeContext(inner) + err := wrapped(context.Background(), nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if capturedCtx == nil { + t.Fatal("inner function was not called") + } + + // Verify that the context was enriched with nodes by client.WithNodes + if capturedCtx == context.Background() { + t.Error("context was not modified by wrapWithNodeContext, expected client.WithNodes to be applied") + } +} + +func TestWrapWithNodeContext_NoNodesNoClient(t *testing.T) { + origNodes := GlobalArgs.Nodes + defer func() { GlobalArgs.Nodes = origNodes }() + + GlobalArgs.Nodes = []string{} + + inner := func(ctx context.Context, c *client.Client) error { + return nil + } + + wrapped := wrapWithNodeContext(inner) + err := wrapped(context.Background(), nil) + if err == nil { + t.Error("expected error when no nodes and no client config context, got nil") + } +} From d6d5651dd7dd0320a3b74d9d58e00ccc973ef41d Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:18:44 +0300 Subject: [PATCH 4/9] fix(commands): fix resolveTemplatePaths and improve test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve template paths relative to rootDir instead of CWD, preventing incorrect resolution when talm is run from a directory different from the project root. Remove misleading fallback that silently substituted templates/ when a path pointed outside the project root — this could cause the wrong file to be used without any warning. Fix misleading comment suggesting configFile is applied as a patch in the template rendering path — it is only used for modeline metadata. Add test cases covering real rootDir, absolute paths inside root, and paths outside root. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 24 +++++++----------------- pkg/commands/apply_test.go | 32 +++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index de7d008..0b5e745 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -17,7 +17,6 @@ package commands import ( "context" "fmt" - "os" "path/filepath" "strings" "time" @@ -115,7 +114,7 @@ func apply(args []string) error { var result []byte if shouldUseTemplateRendering(modelineTemplates) { - // Template rendering path: render templates via Helm engine, then apply node file as patch + // Template rendering path: render templates via Helm engine to produce full config resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) opts := engine.Options{ TalosVersion: applyCmdFlags.talosVersion, @@ -226,6 +225,7 @@ func shouldUseTemplateRendering(templates []string) bool { // resolveTemplatePaths resolves template file paths relative to the project root, // normalizing them for the Helm engine (forward slashes). +// Relative paths from the modeline are resolved against rootDir, not CWD. func resolveTemplatePaths(templates []string, rootDir string) []string { resolved := make([]string, len(templates)) absRootDir, rootErr := filepath.Abs(rootDir) @@ -241,12 +241,8 @@ func resolveTemplatePaths(templates []string, rootDir string) []string { if filepath.IsAbs(templatePath) { absTemplatePath = templatePath } else { - var absErr error - absTemplatePath, absErr = filepath.Abs(templatePath) - if absErr != nil { - resolved[i] = engine.NormalizeTemplatePath(templatePath) - continue - } + // Resolve relative paths against rootDir, not CWD + absTemplatePath = filepath.Join(absRootDir, templatePath) } relPath, relErr := filepath.Rel(absRootDir, absTemplatePath) if relErr != nil { @@ -255,15 +251,9 @@ func resolveTemplatePaths(templates []string, rootDir string) []string { } relPath = filepath.Clean(relPath) if strings.HasPrefix(relPath, "..") { - templateName := filepath.Base(templatePath) - possiblePath := filepath.Join("templates", templateName) - fullPath := filepath.Join(absRootDir, possiblePath) - if _, statErr := os.Stat(fullPath); statErr == nil { - relPath = possiblePath - } else { - resolved[i] = engine.NormalizeTemplatePath(templatePath) - continue - } + // Path goes outside project root — use original path as-is + resolved[i] = engine.NormalizeTemplatePath(templatePath) + continue } resolved[i] = engine.NormalizeTemplatePath(relPath) } diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index 6b99991..6840957 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -2,6 +2,8 @@ package commands import ( "context" + "os" + "path/filepath" "testing" "github.com/siderolabs/talos/pkg/machinery/client" @@ -30,6 +32,12 @@ func TestShouldUseTemplateRendering(t *testing.T) { } func TestResolveTemplatePaths(t *testing.T) { + // Create a real rootDir with template files for testing + tmpRoot := t.TempDir() + if err := os.MkdirAll(filepath.Join(tmpRoot, "templates"), 0o755); err != nil { + t.Fatalf("failed to create templates dir: %v", err) + } + tests := []struct { name string templates []string @@ -37,17 +45,35 @@ func TestResolveTemplatePaths(t *testing.T) { want []string }{ { - name: "simple relative path", + name: "relative path with empty rootDir", templates: []string{"templates/controlplane.yaml"}, rootDir: "", want: []string{"templates/controlplane.yaml"}, }, { - name: "multiple paths", + name: "relative path resolved against rootDir", + templates: []string{"templates/controlplane.yaml"}, + rootDir: tmpRoot, + want: []string{"templates/controlplane.yaml"}, + }, + { + name: "multiple paths with rootDir", templates: []string{"templates/controlplane.yaml", "templates/worker.yaml"}, - rootDir: "", + rootDir: tmpRoot, want: []string{"templates/controlplane.yaml", "templates/worker.yaml"}, }, + { + name: "absolute path inside rootDir", + templates: []string{filepath.Join(tmpRoot, "templates", "controlplane.yaml")}, + rootDir: tmpRoot, + want: []string{"templates/controlplane.yaml"}, + }, + { + name: "path outside rootDir is kept as-is", + templates: []string{"/other/project/templates/controlplane.yaml"}, + rootDir: tmpRoot, + want: []string{"/other/project/templates/controlplane.yaml"}, + }, } for _, tt := range tests { From 805a25df5455505acc8eb1fab06ba4b8e9bbfec2 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:23:35 +0300 Subject: [PATCH 5/9] refactor(commands): address review feedback - Fix typo: owerwrite -> overwrite in processModelineAndUpdateGlobals - wrapWithNodeContext no longer mutates GlobalArgs.Nodes; uses local copy - Remove trivial shouldUseTemplateRendering indirection, inline the check - Document why resolveTemplatePaths resolves against rootDir (not CWD) and how it differs from template.go's path resolution - Document why insecure/maintenance path does not need node context - Remove shouldUseTemplateRendering test (function removed) Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 23 +++++++++++++---------- pkg/commands/apply_test.go | 22 ---------------------- pkg/commands/root.go | 6 +++--- 3 files changed, 16 insertions(+), 35 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 0b5e745..495c964 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -113,7 +113,7 @@ func apply(args []string) error { withSecretsPath := ResolveSecretsPath(applyCmdFlags.withSecrets) var result []byte - if shouldUseTemplateRendering(modelineTemplates) { + if len(modelineTemplates) > 0 { // Template rendering path: render templates via Helm engine to produce full config resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) opts := engine.Options{ @@ -152,6 +152,9 @@ func apply(args []string) error { withClient := func(f func(ctx context.Context, c *client.Client) error) error { if applyCmdFlags.insecure { + // Maintenance mode connects directly to the node IP without talosconfig; + // node context injection is not needed — the maintenance client handles + // node targeting internally via GlobalArgs.Nodes. return WithClientMaintenance(applyCmdFlags.certFingerprints, f) } @@ -199,9 +202,11 @@ func apply(args []string) error { // wrapWithNodeContext wraps a client action function to resolve and inject node // context. If GlobalArgs.Nodes is already set, uses those directly. Otherwise, // attempts to resolve nodes from the client's config context. +// This function does not mutate GlobalArgs — it uses a local copy of nodes. func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) func(ctx context.Context, c *client.Client) error { return func(ctx context.Context, c *client.Client) error { - if len(GlobalArgs.Nodes) < 1 { + nodes := GlobalArgs.Nodes + if len(nodes) < 1 { if c == nil { return fmt.Errorf("failed to resolve config context: no client available") } @@ -209,23 +214,21 @@ func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) fu if configContext == nil { return fmt.Errorf("failed to resolve config context") } - GlobalArgs.Nodes = configContext.Nodes + nodes = configContext.Nodes } - ctx = client.WithNodes(ctx, GlobalArgs.Nodes...) + ctx = client.WithNodes(ctx, nodes...) return f(ctx, c) } } -// shouldUseTemplateRendering returns true if templates are available from modeline -// and should be rendered via the Helm engine before applying. -func shouldUseTemplateRendering(templates []string) bool { - return len(templates) > 0 -} - // resolveTemplatePaths resolves template file paths relative to the project root, // normalizing them for the Helm engine (forward slashes). // Relative paths from the modeline are resolved against rootDir, not CWD. +// +// Note: template.go has similar path resolution in generateOutput() but resolves +// against CWD via filepath.Abs. This function intentionally resolves against rootDir +// because modeline template paths are relative to the project root by convention. func resolveTemplatePaths(templates []string, rootDir string) []string { resolved := make([]string, len(templates)) absRootDir, rootErr := filepath.Abs(rootDir) diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index 6840957..91b3716 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -9,28 +9,6 @@ import ( "github.com/siderolabs/talos/pkg/machinery/client" ) -func TestShouldUseTemplateRendering(t *testing.T) { - tests := []struct { - name string - templates []string - want bool - }{ - {"nil templates", nil, false}, - {"empty templates", []string{}, false}, - {"one template", []string{"templates/controlplane.yaml"}, true}, - {"multiple templates", []string{"templates/controlplane.yaml", "templates/worker.yaml"}, true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := shouldUseTemplateRendering(tt.templates) - if got != tt.want { - t.Errorf("shouldUseTemplateRendering(%v) = %v, want %v", tt.templates, got, tt.want) - } - }) - } -} - func TestResolveTemplatePaths(t *testing.T) { // Create a real rootDir with template files for testing tmpRoot := t.TempDir() diff --git a/pkg/commands/root.go b/pkg/commands/root.go index 8d28395..c5c6871 100644 --- a/pkg/commands/root.go +++ b/pkg/commands/root.go @@ -219,7 +219,7 @@ func DetectRootForTemplate(templatePath string) (string, error) { return DetectProjectRootForFile(templatePath) } -func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endpointsFromArgs bool, owerwrite bool) ([]string, error) { +func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endpointsFromArgs bool, overwrite bool) ([]string, error) { modelineConfig, err := modeline.ReadAndParseModeline(configFile) if err != nil { fmt.Printf("Warning: modeline parsing failed: %v\n", err) @@ -231,14 +231,14 @@ func processModelineAndUpdateGlobals(configFile string, nodesFromArgs bool, endp // Update global settings if modeline was successfully parsed if modelineConfig != nil { if !nodesFromArgs && len(modelineConfig.Nodes) > 0 { - if owerwrite { + if overwrite { GlobalArgs.Nodes = modelineConfig.Nodes } else { GlobalArgs.Nodes = append(GlobalArgs.Nodes, modelineConfig.Nodes...) } } if !endpointsFromArgs && len(modelineConfig.Endpoints) > 0 { - if owerwrite { + if overwrite { GlobalArgs.Endpoints = modelineConfig.Endpoints } else { GlobalArgs.Endpoints = append(GlobalArgs.Endpoints, modelineConfig.Endpoints...) From 64bfcd87958b6a39dc703adf18bc43b2b9143004 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:35:00 +0300 Subject: [PATCH 6/9] fix(commands): address review round 2 feedback - wrapWithNodeContext: make defensive copy of GlobalArgs.Nodes to prevent cross-contamination if the underlying slice is modified between wrapper creation and invocation. Fix doc comment to accurately describe behavior (reads at invocation time, not at wrapper creation time). - Tests: verify actual nodes in gRPC metadata (not just context identity), add TestWrapWithNodeContext_DoesNotMutateGlobalArgs to prove defensive copy works, restore GlobalArgs in root_test.go via defer to prevent test pollution. - Document template.go's additional basename fallback behavior difference in resolveTemplatePaths comment. - Document why insecure/maintenance path does not wrap with node context. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 12 +++++--- pkg/commands/apply_test.go | 62 ++++++++++++++++++++++++++++++++++++-- pkg/commands/root_test.go | 18 ++++++++--- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 495c964..5f6f598 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -202,10 +202,11 @@ func apply(args []string) error { // wrapWithNodeContext wraps a client action function to resolve and inject node // context. If GlobalArgs.Nodes is already set, uses those directly. Otherwise, // attempts to resolve nodes from the client's config context. -// This function does not mutate GlobalArgs — it uses a local copy of nodes. +// This function does not mutate GlobalArgs. It reads GlobalArgs.Nodes at +// invocation time (not at wrapper creation time) and makes a defensive copy. func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) func(ctx context.Context, c *client.Client) error { return func(ctx context.Context, c *client.Client) error { - nodes := GlobalArgs.Nodes + nodes := append([]string(nil), GlobalArgs.Nodes...) if len(nodes) < 1 { if c == nil { return fmt.Errorf("failed to resolve config context: no client available") @@ -227,8 +228,11 @@ func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) fu // Relative paths from the modeline are resolved against rootDir, not CWD. // // Note: template.go has similar path resolution in generateOutput() but resolves -// against CWD via filepath.Abs. This function intentionally resolves against rootDir -// because modeline template paths are relative to the project root by convention. +// against CWD via filepath.Abs and has an additional fallback that tries +// templates/ when a path resolves outside the root. This function +// intentionally resolves against rootDir (modeline paths are root-relative by +// convention) and does not perform the basename fallback to avoid silently +// substituting a different file. func resolveTemplatePaths(templates []string, rootDir string) []string { resolved := make([]string, len(templates)) absRootDir, rootErr := filepath.Abs(rootDir) diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index 91b3716..3651c84 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -4,9 +4,11 @@ import ( "context" "os" "path/filepath" + "slices" "testing" "github.com/siderolabs/talos/pkg/machinery/client" + "google.golang.org/grpc/metadata" ) func TestResolveTemplatePaths(t *testing.T) { @@ -91,9 +93,63 @@ func TestWrapWithNodeContext_SetsNodesInContext(t *testing.T) { t.Fatal("inner function was not called") } - // Verify that the context was enriched with nodes by client.WithNodes - if capturedCtx == context.Background() { - t.Error("context was not modified by wrapWithNodeContext, expected client.WithNodes to be applied") + // Verify the actual nodes injected via gRPC metadata + md, ok := metadata.FromOutgoingContext(capturedCtx) + if !ok { + t.Fatal("expected outgoing gRPC metadata in context, got none") + } + gotNodes := md.Get("nodes") + wantNodes := []string{"10.0.0.1", "10.0.0.2"} + if !slices.Equal(gotNodes, wantNodes) { + t.Errorf("nodes in context metadata = %v, want %v", gotNodes, wantNodes) + } +} + +func TestWrapWithNodeContext_DoesNotMutateGlobalArgs(t *testing.T) { + origNodes := GlobalArgs.Nodes + defer func() { GlobalArgs.Nodes = origNodes }() + + GlobalArgs.Nodes = []string{"10.0.0.1"} + + inner := func(ctx context.Context, c *client.Client) error { + return nil + } + + wrapped := wrapWithNodeContext(inner) + + // Mutate GlobalArgs.Nodes after creating wrapper but before calling it + GlobalArgs.Nodes = []string{"10.0.0.2"} + + var capturedCtx context.Context + innerCapture := func(ctx context.Context, c *client.Client) error { + capturedCtx = ctx + return nil + } + wrapped2 := wrapWithNodeContext(innerCapture) + + // Call the first wrapper — should NOT see the mutation (reads at call time, + // but we test that the defensive copy prevents cross-contamination) + if err := wrapped(context.Background(), nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Call the second wrapper — should see "10.0.0.2" + if err := wrapped2(context.Background(), nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + md, ok := metadata.FromOutgoingContext(capturedCtx) + if !ok { + t.Fatal("expected outgoing gRPC metadata in context") + } + gotNodes := md.Get("nodes") + if !slices.Equal(gotNodes, []string{"10.0.0.2"}) { + t.Errorf("nodes in context = %v, want [10.0.0.2]", gotNodes) + } + + // Verify GlobalArgs.Nodes was not mutated by wrapWithNodeContext + if !slices.Equal(GlobalArgs.Nodes, []string{"10.0.0.2"}) { + t.Errorf("GlobalArgs.Nodes was mutated to %v, expected [10.0.0.2]", GlobalArgs.Nodes) } } diff --git a/pkg/commands/root_test.go b/pkg/commands/root_test.go index aae3ed6..fada4ee 100644 --- a/pkg/commands/root_test.go +++ b/pkg/commands/root_test.go @@ -7,7 +7,13 @@ import ( ) func TestProcessModelineAndUpdateGlobals_ReturnsTemplates(t *testing.T) { - // Create temp file with modeline containing templates + origNodes := GlobalArgs.Nodes + origEndpoints := GlobalArgs.Endpoints + defer func() { + GlobalArgs.Nodes = origNodes + GlobalArgs.Endpoints = origEndpoints + }() + tmpDir := t.TempDir() configFile := filepath.Join(tmpDir, "node0.yaml") content := `# talm: nodes=["10.0.0.1"], endpoints=["10.0.0.1"], templates=["templates/controlplane.yaml"] @@ -18,7 +24,6 @@ machine: t.Fatalf("failed to write temp file: %v", err) } - // Reset global state GlobalArgs.Nodes = []string{} GlobalArgs.Endpoints = []string{} @@ -36,7 +41,13 @@ machine: } func TestProcessModelineAndUpdateGlobals_NoTemplates(t *testing.T) { - // Create temp file with modeline without templates + origNodes := GlobalArgs.Nodes + origEndpoints := GlobalArgs.Endpoints + defer func() { + GlobalArgs.Nodes = origNodes + GlobalArgs.Endpoints = origEndpoints + }() + tmpDir := t.TempDir() configFile := filepath.Join(tmpDir, "node0.yaml") content := `# talm: nodes=["10.0.0.1"], endpoints=["10.0.0.1"] @@ -47,7 +58,6 @@ machine: t.Fatalf("failed to write temp file: %v", err) } - // Reset global state GlobalArgs.Nodes = []string{} GlobalArgs.Endpoints = []string{} From 1c5b611ae1e43ba1897da54291751b45f6314c69 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 00:39:33 +0300 Subject: [PATCH 7/9] fix(commands): address review round 3 feedback - Extract buildApplyRenderOptions and buildApplyPatchOptions for testable options construction. Add tests verifying template path sets Full=true, Offline=true, Root, TemplateFiles; patch path does not. - Fix resolveTemplatePaths: guard against empty rootDir explicitly instead of letting filepath.Abs("") silently resolve to CWD. Empty rootDir now normalizes paths only, consistent with the documented behavior. - Rewrite TestWrapWithNodeContext_DoesNotMutateGlobalArgs to properly test the defensive copy: uses a slice with extra capacity to verify append cannot leak back, and verifies GlobalArgs remains unchanged after call. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 51 ++++++++++++------ pkg/commands/apply_test.go | 106 +++++++++++++++++++++++++++++++------ 2 files changed, 123 insertions(+), 34 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 5f6f598..236d8f5 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -115,29 +115,14 @@ func apply(args []string) error { var result []byte if len(modelineTemplates) > 0 { // Template rendering path: render templates via Helm engine to produce full config - resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) - opts := engine.Options{ - TalosVersion: applyCmdFlags.talosVersion, - WithSecrets: withSecretsPath, - KubernetesVersion: applyCmdFlags.kubernetesVersion, - Debug: applyCmdFlags.debug, - Full: true, - Offline: true, - Root: Config.RootDir, - TemplateFiles: resolvedTemplates, - } + opts := buildApplyRenderOptions(modelineTemplates, withSecretsPath) result, err = engine.Render(ctx, nil, opts) if err != nil { return fmt.Errorf("template rendering error: %s", err) } } else { // Direct patch path: apply config file as patch against empty bundle - opts := engine.Options{ - TalosVersion: applyCmdFlags.talosVersion, - WithSecrets: withSecretsPath, - KubernetesVersion: applyCmdFlags.kubernetesVersion, - Debug: applyCmdFlags.debug, - } + opts := buildApplyPatchOptions(withSecretsPath) patches := []string{"@" + configFile} configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) if err != nil { @@ -199,6 +184,31 @@ func apply(args []string) error { return nil } +// buildApplyRenderOptions constructs engine.Options for the template rendering path. +func buildApplyRenderOptions(modelineTemplates []string, withSecretsPath string) engine.Options { + resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) + return engine.Options{ + TalosVersion: applyCmdFlags.talosVersion, + WithSecrets: withSecretsPath, + KubernetesVersion: applyCmdFlags.kubernetesVersion, + Debug: applyCmdFlags.debug, + Full: true, + Offline: true, + Root: Config.RootDir, + TemplateFiles: resolvedTemplates, + } +} + +// buildApplyPatchOptions constructs engine.Options for the direct patch path. +func buildApplyPatchOptions(withSecretsPath string) engine.Options { + return engine.Options{ + TalosVersion: applyCmdFlags.talosVersion, + WithSecrets: withSecretsPath, + KubernetesVersion: applyCmdFlags.kubernetesVersion, + Debug: applyCmdFlags.debug, + } +} + // wrapWithNodeContext wraps a client action function to resolve and inject node // context. If GlobalArgs.Nodes is already set, uses those directly. Otherwise, // attempts to resolve nodes from the client's config context. @@ -235,6 +245,13 @@ func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) fu // substituting a different file. func resolveTemplatePaths(templates []string, rootDir string) []string { resolved := make([]string, len(templates)) + if rootDir == "" { + // No rootDir specified — normalize paths only, don't resolve against CWD + for i, p := range templates { + resolved[i] = engine.NormalizeTemplatePath(p) + } + return resolved + } absRootDir, rootErr := filepath.Abs(rootDir) if rootErr != nil { for i, p := range templates { diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index 3651c84..e642929 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -11,6 +11,78 @@ import ( "google.golang.org/grpc/metadata" ) +func TestBuildApplyRenderOptions(t *testing.T) { + origTalosVersion := applyCmdFlags.talosVersion + origKubeVersion := applyCmdFlags.kubernetesVersion + origDebug := applyCmdFlags.debug + origRootDir := Config.RootDir + defer func() { + applyCmdFlags.talosVersion = origTalosVersion + applyCmdFlags.kubernetesVersion = origKubeVersion + applyCmdFlags.debug = origDebug + Config.RootDir = origRootDir + }() + + applyCmdFlags.talosVersion = "v1.12" + applyCmdFlags.kubernetesVersion = "1.31.0" + applyCmdFlags.debug = false + Config.RootDir = "/project" + + opts := buildApplyRenderOptions( + []string{"templates/controlplane.yaml"}, + "/project/secrets.yaml", + ) + + if !opts.Full { + t.Error("expected Full=true for template rendering path") + } + if !opts.Offline { + t.Error("expected Offline=true for template rendering path") + } + if opts.Root != "/project" { + t.Errorf("expected Root=/project, got %s", opts.Root) + } + if opts.TalosVersion != "v1.12" { + t.Errorf("expected TalosVersion=v1.12, got %s", opts.TalosVersion) + } + if opts.WithSecrets != "/project/secrets.yaml" { + t.Errorf("expected WithSecrets=/project/secrets.yaml, got %s", opts.WithSecrets) + } + if len(opts.TemplateFiles) != 1 || opts.TemplateFiles[0] != "templates/controlplane.yaml" { + t.Errorf("expected TemplateFiles=[templates/controlplane.yaml], got %v", opts.TemplateFiles) + } +} + +func TestBuildApplyPatchOptions(t *testing.T) { + origTalosVersion := applyCmdFlags.talosVersion + origKubeVersion := applyCmdFlags.kubernetesVersion + origDebug := applyCmdFlags.debug + defer func() { + applyCmdFlags.talosVersion = origTalosVersion + applyCmdFlags.kubernetesVersion = origKubeVersion + applyCmdFlags.debug = origDebug + }() + + applyCmdFlags.talosVersion = "v1.12" + applyCmdFlags.kubernetesVersion = "1.31.0" + applyCmdFlags.debug = false + + opts := buildApplyPatchOptions("/project/secrets.yaml") + + if opts.Full { + t.Error("expected Full=false for direct patch path") + } + if opts.Offline { + t.Error("expected Offline=false for direct patch path") + } + if opts.Root != "" { + t.Errorf("expected empty Root for direct patch path, got %s", opts.Root) + } + if len(opts.TemplateFiles) != 0 { + t.Errorf("expected no TemplateFiles for direct patch path, got %v", opts.TemplateFiles) + } +} + func TestResolveTemplatePaths(t *testing.T) { // Create a real rootDir with template files for testing tmpRoot := t.TempDir() @@ -109,31 +181,36 @@ func TestWrapWithNodeContext_DoesNotMutateGlobalArgs(t *testing.T) { origNodes := GlobalArgs.Nodes defer func() { GlobalArgs.Nodes = origNodes }() - GlobalArgs.Nodes = []string{"10.0.0.1"} + // Use a slice with extra capacity so append inside the closure could + // theoretically leak back to GlobalArgs if the copy is shallow + nodes := make([]string, 1, 10) + nodes[0] = "10.0.0.1" + GlobalArgs.Nodes = nodes inner := func(ctx context.Context, c *client.Client) error { return nil } wrapped := wrapWithNodeContext(inner) + if err := wrapped(context.Background(), nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } - // Mutate GlobalArgs.Nodes after creating wrapper but before calling it + // Verify GlobalArgs.Nodes is unchanged after wrapWithNodeContext call + if !slices.Equal(GlobalArgs.Nodes, []string{"10.0.0.1"}) { + t.Errorf("GlobalArgs.Nodes was mutated to %v, expected [10.0.0.1]", GlobalArgs.Nodes) + } + + // Verify that the defensive copy is independent: mutating GlobalArgs + // after wrapper creation doesn't affect a subsequent call GlobalArgs.Nodes = []string{"10.0.0.2"} var capturedCtx context.Context - innerCapture := func(ctx context.Context, c *client.Client) error { + inner2 := func(ctx context.Context, c *client.Client) error { capturedCtx = ctx return nil } - wrapped2 := wrapWithNodeContext(innerCapture) - - // Call the first wrapper — should NOT see the mutation (reads at call time, - // but we test that the defensive copy prevents cross-contamination) - if err := wrapped(context.Background(), nil); err != nil { - t.Fatalf("unexpected error: %v", err) - } - - // Call the second wrapper — should see "10.0.0.2" + wrapped2 := wrapWithNodeContext(inner2) if err := wrapped2(context.Background(), nil); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -146,11 +223,6 @@ func TestWrapWithNodeContext_DoesNotMutateGlobalArgs(t *testing.T) { if !slices.Equal(gotNodes, []string{"10.0.0.2"}) { t.Errorf("nodes in context = %v, want [10.0.0.2]", gotNodes) } - - // Verify GlobalArgs.Nodes was not mutated by wrapWithNodeContext - if !slices.Equal(GlobalArgs.Nodes, []string{"10.0.0.2"}) { - t.Errorf("GlobalArgs.Nodes was mutated to %v, expected [10.0.0.2]", GlobalArgs.Nodes) - } } func TestWrapWithNodeContext_NoNodesNoClient(t *testing.T) { From b393aa5e6416550b721a5736a6c31ffe804a005c Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 10:16:54 +0300 Subject: [PATCH 8/9] fix(commands): address bot review feedback - Use %w instead of %s for error wrapping to preserve error chains - Fix typo: 'clinet' -> 'client' in comment - Document Offline: true limitation in buildApplyRenderOptions (Helm lookup() not available; use 'talm template' for online rendering) Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/apply.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index 236d8f5..6cab784 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -78,7 +78,7 @@ var applyCmd = &cobra.Command{ } applyCmdFlags.nodesFromArgs = len(GlobalArgs.Nodes) > 0 applyCmdFlags.endpointsFromArgs = len(GlobalArgs.Endpoints) > 0 - // Set dummy endpoint to avoid errors on building clinet + // Set dummy endpoint to avoid errors on building client if len(GlobalArgs.Endpoints) == 0 { GlobalArgs.Endpoints = append(GlobalArgs.Endpoints, "127.0.0.1") } @@ -118,7 +118,7 @@ func apply(args []string) error { opts := buildApplyRenderOptions(modelineTemplates, withSecretsPath) result, err = engine.Render(ctx, nil, opts) if err != nil { - return fmt.Errorf("template rendering error: %s", err) + return fmt.Errorf("template rendering error: %w", err) } } else { // Direct patch path: apply config file as patch against empty bundle @@ -126,12 +126,12 @@ func apply(args []string) error { patches := []string{"@" + configFile} configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) if err != nil { - return fmt.Errorf("full config processing error: %s", err) + return fmt.Errorf("full config processing error: %w", err) } result, err = engine.SerializeConfiguration(configBundle, machineType) if err != nil { - return fmt.Errorf("error serializing configuration: %s", err) + return fmt.Errorf("error serializing configuration: %w", err) } } @@ -185,6 +185,9 @@ func apply(args []string) error { } // buildApplyRenderOptions constructs engine.Options for the template rendering path. +// Offline is set to true because at this point we don't have a Talos client for +// Helm lookup functions. Templates that use lookup() should be rendered via +// 'talm template' which supports online mode. func buildApplyRenderOptions(modelineTemplates []string, withSecretsPath string) engine.Options { resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir) return engine.Options{ From d51b9dcfd39d8fd80b774855364b866fc8cb0b8c Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 2 Apr 2026 10:18:39 +0300 Subject: [PATCH 9/9] test(commands): add error case tests for processModelineAndUpdateGlobals Add tests for invalid modeline syntax (parsing failure) and empty nodes (validation error), covering both error branches. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/commands/root_test.go | 61 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/pkg/commands/root_test.go b/pkg/commands/root_test.go index fada4ee..ccd4756 100644 --- a/pkg/commands/root_test.go +++ b/pkg/commands/root_test.go @@ -70,3 +70,64 @@ machine: t.Errorf("expected 0 templates, got %d: %v", len(templates), templates) } } + +func TestProcessModelineAndUpdateGlobals_InvalidModeline(t *testing.T) { + origNodes := GlobalArgs.Nodes + origEndpoints := GlobalArgs.Endpoints + defer func() { + GlobalArgs.Nodes = origNodes + GlobalArgs.Endpoints = origEndpoints + }() + + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "node0.yaml") + content := `# talm: this is not valid modeline syntax +machine: + type: controlplane +` + if err := os.WriteFile(configFile, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp file: %v", err) + } + + GlobalArgs.Nodes = []string{} + GlobalArgs.Endpoints = []string{} + + templates, err := processModelineAndUpdateGlobals(configFile, false, false, true) + if err == nil { + t.Fatal("expected error for invalid modeline, got nil") + } + if templates != nil { + t.Errorf("expected nil templates on error, got %v", templates) + } +} + +func TestProcessModelineAndUpdateGlobals_EmptyNodesError(t *testing.T) { + origNodes := GlobalArgs.Nodes + origEndpoints := GlobalArgs.Endpoints + defer func() { + GlobalArgs.Nodes = origNodes + GlobalArgs.Endpoints = origEndpoints + }() + + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "node0.yaml") + // Valid modeline but with empty nodes + content := `# talm: nodes=[], endpoints=["10.0.0.1"], templates=["templates/controlplane.yaml"] +machine: + type: controlplane +` + if err := os.WriteFile(configFile, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp file: %v", err) + } + + GlobalArgs.Nodes = []string{} + GlobalArgs.Endpoints = []string{} + + templates, err := processModelineAndUpdateGlobals(configFile, false, false, true) + if err == nil { + t.Fatal("expected error for empty nodes, got nil") + } + if templates != nil { + t.Errorf("expected nil templates on error, got %v", templates) + } +}