Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 63 additions & 42 deletions pkg/commands/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,33 @@ func apply(args []string) error {
// Resolve secrets.yaml path relative to project root if not absolute
withSecretsPath := ResolveSecretsPath(applyCmdFlags.withSecrets)

var result []byte
if len(modelineTemplates) > 0 {
// Template rendering path: render templates via Helm engine to produce full config
// Template rendering path: connect to the node first, render templates
// online (so lookup() functions resolve real discovery data), then apply.
opts := buildApplyRenderOptions(modelineTemplates, withSecretsPath)
result, err = engine.Render(ctx, nil, opts)
if err != nil {
return fmt.Errorf("template rendering error: %w", err)
}

err = withApplyClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the code around lines 121 and 157 in pkg/commands/apply.go
head -160 pkg/commands/apply.go | tail -50

Repository: cozystack/talm

Length of output: 2030


🏁 Script executed:

# Also check what GlobalArgs is and its type definition
rg "GlobalArgs\s*=" pkg/commands/ -A 2 -B 2 | head -30

Repository: cozystack/talm

Length of output: 393


🏁 Script executed:

# Search for GlobalArgs struct/variable definition
rg "GlobalArgs" pkg/ -t go | grep -E "(var|const|type).*GlobalArgs" | head -20

Repository: cozystack/talm

Length of output: 105


🏁 Script executed:

# Find the definition of global.Args struct
fd "args.go" pkg/ --type f

Repository: cozystack/talm

Length of output: 40


🏁 Script executed:

# Search for Args struct in global package
rg "type Args struct" pkg/ -A 20

Repository: cozystack/talm

Length of output: 40


🏁 Script executed:

# Find all Go files to locate the global package
find pkg -name "*.go" | xargs grep -l "package global"

Repository: cozystack/talm

Length of output: 42


🏁 Script executed:

# Search for Nodes and Endpoints field declarations
rg "Nodes|Endpoints" pkg/ -t go -B 2 -A 2 | head -50

Repository: cozystack/talm

Length of output: 2941


🏁 Script executed:

# List all Go files in pkg and look for global-related files
ls -la pkg/

Repository: cozystack/talm

Length of output: 503


🏁 Script executed:

# Search more broadly for Args struct definition in entire codebase
rg "Nodes.*\[\]string|Endpoints.*\[\]string" pkg/ -t go -B 5 -A 5 | head -60

Repository: cozystack/talm

Length of output: 3336


🏁 Script executed:

# Verify the exact context around GlobalArgs type assignment
grep -n "GlobalArgs" pkg/commands/root.go -A 5 -B 5

Repository: cozystack/talm

Length of output: 3573


🏁 Script executed:

# Check if global.Args is imported from talos or defined locally
rg "import.*global|from.*global" pkg/commands/ -t go

Repository: cozystack/talm

Length of output: 155


Use %v instead of %s for []string values in format strings.

GlobalArgs.Nodes and GlobalArgs.Endpoints are []string types. The %s format specifier is for single strings and will print Go's error-like formatting (e.g., %!s([]string=[...])) instead of readable values. Use %v to properly display slices.

🩹 Proposed fix
-				fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
+				fmt.Printf("- talm: file=%s, nodes=%v, endpoints=%v\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)

Also applies to: 157-157

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/commands/apply.go` at line 121, The fmt.Printf call that prints slice
values uses %s which misformats []string; update the format specifier to use %v
for GlobalArgs.Nodes and GlobalArgs.Endpoints in the Printf on the line with
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile,
GlobalArgs.Nodes, GlobalArgs.Endpoints) and likewise update the other occurrence
(around the second reported location) so both slices are formatted with %v
instead of %s.


result, err := engine.Render(ctx, c, opts)
if err != nil {
return fmt.Errorf("template rendering error: %w", err)
}

resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
})
if err != nil {
return fmt.Errorf("error applying new configuration: %s", err)
}
Comment on lines +134 to +136
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use %w instead of %s to wrap the error. This is a best practice in Go as it allows the caller to use errors.Is or errors.As to inspect the underlying error.

Suggested change
if err != nil {
return fmt.Errorf("error applying new configuration: %s", err)
}
if err != nil {
return fmt.Errorf("error applying new configuration: %w", err)
}


helpers.PrintApplyResults(resp)

return nil
})
} else {
// Direct patch path: apply config file as patch against empty bundle
opts := buildApplyPatchOptions(withSecretsPath)
Expand All @@ -129,46 +148,29 @@ func apply(args []string) error {
return fmt.Errorf("full config processing error: %w", err)
}

result, err = engine.SerializeConfiguration(configBundle, machineType)
result, err := engine.SerializeConfiguration(configBundle, machineType)
if err != nil {
return fmt.Errorf("error serializing configuration: %w", err)
}
}

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)
}

wrappedF := wrapWithNodeContext(f)

if GlobalArgs.SkipVerify {
return WithClientSkipVerify(wrappedF)
}
err = withApplyClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)

return WithClientNoNodes(wrappedF)
}
resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
})
if err != nil {
return fmt.Errorf("error applying new configuration: %s", err)
}

err = withClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
helpers.PrintApplyResults(resp)

resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
return nil
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The err variable is shadowed in this else block (due to the use of := at lines 146 and 151), which means the assignment at line 156 does not update the err variable checked at line 174. Consequently, any error returned by withApplyClient in the direct patch path will be silently ignored, and the loop will incorrectly proceed to the next file. It is safer to check the error immediately within the block. Additionally, using %w for error wrapping is preferred over %s to maintain error context.

Suggested change
err = withApplyClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
return WithClientNoNodes(wrappedF)
}
resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
})
if err != nil {
return fmt.Errorf("error applying new configuration: %s", err)
}
err = withClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
helpers.PrintApplyResults(resp)
resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
return nil
})
if err := withApplyClient(func(ctx context.Context, c *client.Client) error {
fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\\n", configFile, GlobalArgs.Nodes, GlobalArgs.Endpoints)
resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{
Data: result,
Mode: applyCmdFlags.Mode.Mode,
DryRun: applyCmdFlags.dryRun,
TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout),
})
if err != nil {
return fmt.Errorf("error applying new configuration: %w", err)
}
helpers.PrintApplyResults(resp)
return nil
}); err != nil {
return err
}

if err != nil {
return fmt.Errorf("error applying new configuration: %s", err)
}

helpers.PrintApplyResults(resp)

return nil
})
}
if err != nil {
return err
}
Expand All @@ -184,19 +186,38 @@ func apply(args []string) error {
return nil
}

// withApplyClient creates a Talos client appropriate for the current apply mode
// and invokes the given action with it.
func withApplyClient(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)
}

wrappedF := wrapWithNodeContext(f)

if GlobalArgs.SkipVerify {
return WithClientSkipVerify(wrappedF)
}

return WithClientNoNodes(wrappedF)
}

// 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.
// Offline is false because templates need a live Talos client for lookup() functions
// (e.g., discovering interface names, addresses, routes). The caller creates the
// client and passes it to engine.Render together with these options.
func buildApplyRenderOptions(modelineTemplates []string, withSecretsPath string) engine.Options {
resolvedTemplates := resolveTemplatePaths(modelineTemplates, Config.RootDir)
return engine.Options{
Insecure: applyCmdFlags.insecure,
TalosVersion: applyCmdFlags.talosVersion,
WithSecrets: withSecretsPath,
KubernetesVersion: applyCmdFlags.kubernetesVersion,
Debug: applyCmdFlags.debug,
Full: true,
Offline: true,
Root: Config.RootDir,
TemplateFiles: resolvedTemplates,
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ func TestBuildApplyRenderOptions(t *testing.T) {
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.Offline {
t.Error("expected Offline=false for online template rendering path")
}
if opts.Root != "/project" {
t.Errorf("expected Root=/project, got %s", opts.Root)
Expand Down
Loading