fix(commands): apply command ignores modeline templates and --skip-verify bypasses node context#113
fix(commands): apply command ignores modeline templates and --skip-verify bypasses node context#113
Conversation
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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <file> | 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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
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/<basename> 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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- 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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- 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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- 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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
📝 WalkthroughWalkthroughThe apply command now branches into two config flows: template-based rendering when modelines provide templates, or the existing patch flow otherwise. Changes
Sequence DiagramsequenceDiagram
participant User
participant ApplyCmd as Apply Command
participant Modeline as processModelineAndUpdateGlobals
participant Engine as Engine
participant Client as Client/GRPC
User->>ApplyCmd: run apply with config
ApplyCmd->>Modeline: parse modeline & update globals
Modeline-->>ApplyCmd: return templates[], error
alt templates non-empty (render flow)
ApplyCmd->>ApplyCmd: buildApplyRenderOptions
ApplyCmd->>ApplyCmd: resolveTemplatePaths
ApplyCmd->>Engine: Render(config, options with TemplateFiles)
Engine-->>ApplyCmd: rendered configuration
else templates empty (patch flow)
ApplyCmd->>Engine: FullConfigProcess(config) / SerializeConfiguration
Engine-->>ApplyCmd: processed/serialized output
end
ApplyCmd->>Client: invoke client call (wrapped with node context)
Client-->>ApplyCmd: response
ApplyCmd-->>User: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a template rendering path to the apply command, allowing Helm templates to be processed directly. It includes updates to processModelineAndUpdateGlobals to return templates, new helper functions for engine options and node context resolution, and comprehensive tests. The review feedback highlights several areas for improvement: replacing %s with %w in error wrapping for better error chain preservation, addressing a potential regression where hardcoding Offline: true prevents the use of the lookup function in Helm templates, and simplifying the node resolution logic in wrapWithNodeContext as it may be redundant given existing checks in processModelineAndUpdateGlobals.
pkg/commands/apply.go
Outdated
| opts := buildApplyRenderOptions(modelineTemplates, withSecretsPath) | ||
| result, err = engine.Render(ctx, nil, opts) | ||
| if err != nil { | ||
| return fmt.Errorf("template rendering error: %s", err) |
pkg/commands/apply.go
Outdated
| patches := []string{"@" + configFile} | ||
| configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) | ||
| if err != nil { | ||
| return fmt.Errorf("full config processing error: %s", err) |
pkg/commands/apply.go
Outdated
| 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) |
| KubernetesVersion: applyCmdFlags.kubernetesVersion, | ||
| Debug: applyCmdFlags.debug, | ||
| Full: true, | ||
| Offline: true, |
There was a problem hiding this comment.
Hardcoding Offline: true in the template rendering path prevents the use of the lookup function in Helm templates during talm apply. This is a regression in functionality compared to the talm template | talm apply workaround mentioned in the PR description, where lookup works by default. Consider making this configurable or initializing a client earlier to support online rendering if the templates require it.
There was a problem hiding this comment.
Good catch. The Talos client is not available at rendering time — it is created later in withClient, after the config is already built. Supporting online rendering would require restructuring the flow to establish the client connection before template rendering.
Added a documenting comment in b393aa5 and filed #114 to track this as a follow-up.
| if len(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") | ||
| } | ||
| nodes = configContext.Nodes | ||
| } |
There was a problem hiding this comment.
The logic to resolve nodes from the client's config context appears to be unreachable in the apply command. processModelineAndUpdateGlobals is called at the start of the loop (line 108) and it already enforces that GlobalArgs.Nodes is non-empty (returning an error otherwise at lines 250-252 in root.go). If this wrapper is intended only for apply, this block can be simplified to remove the unreachable resolution logic.
There was a problem hiding this comment.
Intentionally kept as defensive code. wrapWithNodeContext is a standalone helper — while processModelineAndUpdateGlobals guarantees non-empty nodes in the current apply call site, the function could be reused by other callers that do not enforce this precondition. Removing the fallback would make the helper fragile on reuse.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/commands/apply.go (1)
81-84:⚠️ Potential issue | 🟡 MinorMinor typo: "clinet" → "client".
✏️ Proposed fix
- // Set dummy endpoint to avoid errors on building clinet + // Set dummy endpoint to avoid errors on building client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/commands/apply.go` around lines 81 - 84, Fix the typo in the inline comment above the endpoint fallback: change "clinet" to "client" in the comment that precedes the GlobalArgs.Endpoints fallback (the comment right above the conditional that appends "127.0.0.1" to GlobalArgs.Endpoints).
🧹 Nitpick comments (1)
pkg/commands/root_test.go (1)
9-72: Good test coverage for template extraction.The tests properly validate the new return signature behavior with appropriate state isolation via
defer. Consider adding tests for error scenarios:
- Invalid modeline syntax (parsing failure)
- Empty nodes resulting in error (Line 251 in root.go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/commands/root_test.go` around lines 9 - 72, Add two new tests for processModelineAndUpdateGlobals: one that writes a config file containing a malformed modeline (to trigger the modeline parsing failure) and asserts the returned error is non-nil, and another that writes a config with a valid modeline that yields no nodes (or explicitly sets GlobalArgs.Nodes=[] before calling) and asserts the function returns the specific error path for empty nodes (the error branch referenced around Line 251 in root.go). Use the same pattern of saving/restoring GlobalArgs (origNodes/origEndpoints with defer), create temp files via t.TempDir() and os.WriteFile, call processModelineAndUpdateGlobals(configFile, false, false, true), and assert the expected non-nil error for the parse case and the expected empty-nodes error for the empty nodes case to cover those failure branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/commands/apply.go`:
- Around line 81-84: Fix the typo in the inline comment above the endpoint
fallback: change "clinet" to "client" in the comment that precedes the
GlobalArgs.Endpoints fallback (the comment right above the conditional that
appends "127.0.0.1" to GlobalArgs.Endpoints).
---
Nitpick comments:
In `@pkg/commands/root_test.go`:
- Around line 9-72: Add two new tests for processModelineAndUpdateGlobals: one
that writes a config file containing a malformed modeline (to trigger the
modeline parsing failure) and asserts the returned error is non-nil, and another
that writes a config with a valid modeline that yields no nodes (or explicitly
sets GlobalArgs.Nodes=[] before calling) and asserts the function returns the
specific error path for empty nodes (the error branch referenced around Line 251
in root.go). Use the same pattern of saving/restoring GlobalArgs
(origNodes/origEndpoints with defer), create temp files via t.TempDir() and
os.WriteFile, call processModelineAndUpdateGlobals(configFile, false, false,
true), and assert the expected non-nil error for the parse case and the expected
empty-nodes error for the empty nodes case to cover those failure branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9573cdb8-1d2b-4b4c-b76f-b5e561a22b9f
📒 Files selected for processing (6)
pkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/root.gopkg/commands/root_test.gopkg/commands/talosctl_wrapper.gopkg/commands/upgrade_handler.go
- 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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add tests for invalid modeline syntax (parsing failure) and empty nodes (validation error), covering both error branches. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Addressing bot review feedback:
|
|
Thank you for the update, @lexfrei. The fixes look solid, and the added unit tests provide excellent coverage for the new template rendering path and the node context injection logic. I have no further concerns regarding these changes. |
|
Tip For best results, initiate chat on the files or code changes.
Appreciate the thorough handling of all the feedback — the defensive copy behavior and the follow-up issue If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/commands/apply.go (1)
191-212: Factor the sharedengine.Optionsdefaults out of both builders.Lines 194-197 and 208-211 need to stay in lockstep. A tiny base helper would reduce the chance of future flag changes updating one path but not the other.
Refactor sketch
+func buildApplyBaseOptions(withSecretsPath string) engine.Options { + return engine.Options{ + TalosVersion: applyCmdFlags.talosVersion, + WithSecrets: withSecretsPath, + KubernetesVersion: applyCmdFlags.kubernetesVersion, + Debug: applyCmdFlags.debug, + } +} + // 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{ - TalosVersion: applyCmdFlags.talosVersion, - WithSecrets: withSecretsPath, - KubernetesVersion: applyCmdFlags.kubernetesVersion, - Debug: applyCmdFlags.debug, - Full: true, - Offline: true, - Root: Config.RootDir, - TemplateFiles: resolvedTemplates, - } + opts := buildApplyBaseOptions(withSecretsPath) + opts.Full = true + opts.Offline = true + opts.Root = Config.RootDir + opts.TemplateFiles = resolvedTemplates + return opts } // 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, - } + return buildApplyBaseOptions(withSecretsPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/commands/apply.go` around lines 191 - 212, Extract the common engine.Options defaults into a small helper (e.g., buildBaseApplyOptions or baseApplyOptions) that returns an engine.Options populated with TalosVersion, WithSecrets, KubernetesVersion, Debug and any other shared defaults (Root, Full, Offline or TemplateFiles left unset as appropriate), then have buildApplyRenderOptions call that helper and set/override TemplateFiles, Root, Full, Offline and buildApplyPatchOptions call the same helper and only keep the patch-specific fields; update uses of applyCmdFlags/talosVersion and withSecretsPath to populate the base helper so shared fields in buildApplyRenderOptions and buildApplyPatchOptions remain in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/commands/apply.go`:
- Around line 116-136: When modelineTemplates is non-empty the code renders only
the templates and never merges the original configFile, causing node-specific
YAML in configFile to be dropped; after rendering via
buildApplyRenderOptions/engine.Render (the modelineTemplates branch) include the
original configFile in the merge/patch step the same way the else branch does
(e.g., pass "@" + configFile into engine.FullConfigProcess or otherwise merge
configFile into the rendered result) so that processModelineAndUpdateGlobals and
engine.FullConfigProcess receive both the rendered templates and the original
file's overrides before calling engine.SerializeConfiguration.
---
Nitpick comments:
In `@pkg/commands/apply.go`:
- Around line 191-212: Extract the common engine.Options defaults into a small
helper (e.g., buildBaseApplyOptions or baseApplyOptions) that returns an
engine.Options populated with TalosVersion, WithSecrets, KubernetesVersion,
Debug and any other shared defaults (Root, Full, Offline or TemplateFiles left
unset as appropriate), then have buildApplyRenderOptions call that helper and
set/override TemplateFiles, Root, Full, Offline and buildApplyPatchOptions call
the same helper and only keep the patch-specific fields; update uses of
applyCmdFlags/talosVersion and withSecretsPath to populate the base helper so
shared fields in buildApplyRenderOptions and buildApplyPatchOptions remain in
one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f10d3257-1d1c-4c65-9551-de70e9cbc70c
📒 Files selected for processing (2)
pkg/commands/apply.gopkg/commands/root_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/commands/root_test.go
| if len(modelineTemplates) > 0 { | ||
| // Template rendering path: render templates via Helm engine to produce full config | ||
| opts := buildApplyRenderOptions(modelineTemplates, withSecretsPath) | ||
| result, err = engine.Render(ctx, nil, opts) | ||
| if err != nil { | ||
| return fmt.Errorf("template rendering error: %w", err) | ||
| } | ||
| } else { | ||
| // Direct patch path: apply config file as patch against empty bundle | ||
| opts := buildApplyPatchOptions(withSecretsPath) | ||
| patches := []string{"@" + configFile} | ||
| configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) | ||
| if err != nil { | ||
| 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) | ||
| result, err = engine.SerializeConfiguration(configBundle, machineType) | ||
| if err != nil { | ||
| return fmt.Errorf("error serializing configuration: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't turn modeline-bearing -f files into metadata-only inputs.
Lines 116-123 render only modelineTemplates; configFile is never merged in this branch. Since processModelineAndUpdateGlobals only extracts the modeline metadata, any YAML overrides that remain in the same node file are dropped, so talm apply -f nodes/node0.yaml applies the shared templates but loses the node-specific patch from the file the user passed in. Please keep configFile in the merge step after rendering the template set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/commands/apply.go` around lines 116 - 136, When modelineTemplates is
non-empty the code renders only the templates and never merges the original
configFile, causing node-specific YAML in configFile to be dropped; after
rendering via buildApplyRenderOptions/engine.Render (the modelineTemplates
branch) include the original configFile in the merge/patch step the same way the
else branch does (e.g., pass "@" + configFile into engine.FullConfigProcess or
otherwise merge configFile into the rendered result) so that
processModelineAndUpdateGlobals and engine.FullConfigProcess receive both the
rendered templates and the original file's overrides before calling
engine.SerializeConfiguration.
There was a problem hiding this comment.
This is intentional and matches the existing behavior of talm template -f.
The templateWithFiles function in template.go (lines 127-218) also does NOT pass configFile as a patch to engine.Render. The node file serves as a modeline container (nodes, endpoints, templates metadata) and as the output target for --in-place rendering. The actual cluster configuration is produced entirely by the Helm templates + values.yaml.
The two-step workaround talm template -f node.yaml | talm apply -f - works the same way: template renders full config from templates, apply sends it to the node. This PR makes talm apply -f do both steps internally.
There was a problem hiding this comment.
@lexfrei, thank you for the clear explanation! That makes sense — the node file is purely a modeline carrier (nodes, endpoints, template references) and the full configuration is produced entirely by the Helm templates and values.yaml, consistent with how templateWithFiles works. I've noted this design pattern for future reviews.
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/talm PR: 113
File: pkg/commands/apply.go:116-136
Timestamp: 2026-04-02T08:59:38.723Z
Learning: In the talm project (pkg/commands/apply.go and pkg/commands/template.go), when a node file (e.g. nodes/node0.yaml) is passed via `-f`, it serves only as a modeline container providing metadata (nodes, endpoints, template paths). The actual cluster configuration is produced entirely by Helm templates + values.yaml. The configFile is intentionally NOT passed as a patch to engine.Render. This matches templateWithFiles behavior in template.go and is equivalent to the two-step `talm template -f node.yaml | talm apply -f -` pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Re: nitpick about extracting a shared base options helper ( The two builders share 4 fields and differ in 4 fields. A shared base helper for this would be overengineering — the duplication is minimal and the two paths are semantically distinct (template rendering vs direct patching). Keeping them separate makes each path self-contained and easy to read. |
Summary
Fix two compounding bugs in
talm apply:Modeline templates ignored:
talm apply -f nodes/node0.yamlread the modeline but silentlydiscarded the
templatesfield. The node file was used as a raw patch against an empty configbundle, resulting in missing cluster configuration (
controlPlane.endpoint,clusterName, etc.)and Talos validation failures.
--skip-verifybroken: The--skip-verifypath calledWithClientSkipVerify(f)directly,bypassing node context injection. The gRPC request had no target nodes, so TLS verification
was never actually skipped.
The workaround was
talm template -f nodes/node0.yaml | talm apply --insecure -f -. Both bugsare now fixed, making
talm apply -f nodes/node0.yamlwork as expected.Changes
Bug 1: Modeline template support in apply
processModelineAndUpdateGlobalsnow returns([]string, error)instead oferror,extracting the
Templatesfield from the parsed modelineapplyrenders them viaengine.Render(same path astalm template -f) withFull: trueandOffline: trueFullConfigProcessdirect-patch flow is preservedBug 2: --skip-verify node context
wrapWithNodeContexthelper that resolves nodes fromGlobalArgsor client configcontext and injects them via
client.WithNodes--skip-verifyand normal paths now usewrapWithNodeContext; only the underlyingconnection method differs (
WithClientSkipVerifyvsWithClientNoNodes)Additional improvements
owerwrite→overwriteinprocessModelineAndUpdateGlobalsparameter namewrapWithNodeContextmakes a defensive copy ofGlobalArgs.Nodesto prevent mutationresolveTemplatePathsresolves against project rootDir (not CWD) and does not performsilent basename fallback for paths outside the root
Testing
10 new unit tests covering:
processModelineAndUpdateGlobals)buildApplyRenderOptions,buildApplyPatchOptions)Summary by CodeRabbit
New Features
Tests