feat(commands): apply chain stack semantics, upgrade image source, completion, modeline leading comments, set-value warnings, init error wrap#204
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates CLI shell completion, changes ChangesAll changes (single checkpoint)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the talm CLI, including comprehensive shell completion for init, apply, template, and upgrade commands. It improves project root detection for multi-file inputs by anchoring to the first file, adds a helpful warning when --set is used with IP or version literals (recommending --set-string), ensures that talm template -I preserves leading comments in node files, and updates talm upgrade to source the installer image from values.yaml. Extensive testing has been added to validate these new contracts and features. I have no feedback to provide as there were no review comments to assess.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/commands/contract_upgrade_image_source_test.go`:
- Around line 73-85: The test currently only checks that err mentions a missing
field and that hints contain "--image"; update the assertions to explicitly
enforce the contract that the error/hint references values.yaml: add an
assertion that err.Error() contains "values.yaml" (e.g. after the existing
strings.Contains(err.Error(), "image") check) and/or assert that combined (the
joined hints) contains "values.yaml" so the failure message and hint chain
explicitly name the values.yaml file as required by the contract.
- Around line 104-121: The test
TestResolveUpgradeImageFromValues_EmptyImage_ErrorSameAsMissing currently only
checks for a non-nil error; update it to also assert that the operator-facing
error message/hint is identical to the missing-image case by comparing error
strings. Concretely, call resolveUpgradeImageFromValues on a directory that
simulates the "missing image" case (e.g., no values.yaml or values.yaml without
an image) to obtain errMissing, then assert that err.Error() ==
errMissing.Error() (or use the same exported/common error value if one exists)
for the empty-image case; reference resolveUpgradeImageFromValues and
TestResolveUpgradeImageFromValues_EmptyImage_ErrorSameAsMissing when locating
where to add the comparison.
🪄 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: a2b574ac-e0ce-48f9-afd9-5d9fed5402d8
📒 Files selected for processing (22)
README.mdmain.gopkg/commands/apply.gopkg/commands/completion.gopkg/commands/completion_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_inplace_preserve_test.gopkg/commands/contract_template_set_help_test.gopkg/commands/contract_upgrade_image_source_test.gopkg/commands/init.gopkg/commands/init_test.gopkg/commands/root.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/commands/upgrade_handler.gopkg/commands/upgrade_image_source.gopkg/engine/contract_setvalue_warning_test.gopkg/engine/engine.gopkg/engine/setvalue_warn.gopkg/modeline/contract_find_and_parse_test.gopkg/modeline/modeline.go
lexfrei
force-pushed
the
feat/commands-followups
branch
4 times, most recently
from
edf9a73 to
48aefee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/commands/contract_apply_chain_stack_test.go (1)
56-68: ⚡ Quick winCurrent assertions don’t actually prove side-patch precedence/order.
The test validates both patches are present, but not that later patches win on conflicts (the key behavior for chain order).
Suggested patch
side1 := filepath.Join(dir, "side1.yaml") if err := os.WriteFile(side1, []byte( - "machine:\n certSANs:\n - canary.side1.example.com\n", + "machine:\n certSANs:\n - canary.side1.example.com\n network:\n hostname: from-side1\n", ), 0o644); err != nil { t.Fatal(err) } @@ if !strings.Contains(final, "canary.side1.example.com") { t.Errorf("final merged bytes must include side1's certSAN; got:\n%s", final) } if !strings.Contains(final, "canary-side2") { t.Errorf("final merged bytes must include side2's hostname; got:\n%s", final) } + if strings.Contains(final, "from-side1") { + t.Errorf("final merged bytes must apply side-patches in order (later wins on conflict); got:\n%s", final) + }Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/commands/contract_apply_chain_stack_test.go` around lines 56 - 68, The test writes two side-patch files (side1 and side2) but only asserts both keys exist, not that later patches override earlier ones; change the side-patch contents so they conflict on the same key (e.g., set machine.network.hostname in both side1 and side2 to different values) and update the assertion on the merged bytes to assert the final value equals the value from side2 (the later patch), using the existing side1 and side2 variables to locate and verify the precedence behavior.pkg/commands/completion_test.go (1)
242-247: ⚡ Quick winPin the completion directive in this regression test too.
Line 242 currently ignores the returned directive, so a fallback-to-file completion regression could slip through undetected.
Suggested patch
- got, _ := completeNodeFiles(nil, nil, "") + got, directive := completeNodeFiles(nil, nil, "") want := filepath.Join("nodes", "cp01.yaml") if !slices.Contains(got, want) { t.Errorf("file with leading operator comments must appear in completion; got %v", got) } + if directive != cobra.ShellCompDirectiveNoFileComp { + t.Errorf("directive = %v, want ShellCompDirectiveNoFileComp", directive) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/commands/completion_test.go` around lines 242 - 247, The test currently ignores the completion directive returned by completeNodeFiles, allowing a fallback-to-file completion regression to pass; change the call to capture the directive (e.g., got, dir := completeNodeFiles(nil, nil, "")) and add an assertion that dir equals the no-file completion directive (e.g., cobra.ShellCompDirectiveNoFileComp) so the test fails if completeNodeFiles falls back to file completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/commands/apply.go`:
- Around line 250-258: fileHasTalmModeline currently masks all errors from
modeline.FindAndParseModeline and treats unreadable/corrupt files the same as
files without a modeline; change fileHasTalmModeline to return (bool, error) (or
equivalent) so it returns true/false plus any non-modeline I/O/parsing error
from modeline.FindAndParseModeline, and then update its caller (the side-patch
detection branch that checks for a modeline) to surface or propagate file
I/O/parsing errors instead of treating them as "no modeline"—use the returned
error to distinguish absent modeline (nil err with indication of absence) from
real I/O/parsing failures coming from modeline.FindAndParseModeline.
---
Nitpick comments:
In `@pkg/commands/completion_test.go`:
- Around line 242-247: The test currently ignores the completion directive
returned by completeNodeFiles, allowing a fallback-to-file completion regression
to pass; change the call to capture the directive (e.g., got, dir :=
completeNodeFiles(nil, nil, "")) and add an assertion that dir equals the
no-file completion directive (e.g., cobra.ShellCompDirectiveNoFileComp) so the
test fails if completeNodeFiles falls back to file completion.
In `@pkg/commands/contract_apply_chain_stack_test.go`:
- Around line 56-68: The test writes two side-patch files (side1 and side2) but
only asserts both keys exist, not that later patches override earlier ones;
change the side-patch contents so they conflict on the same key (e.g., set
machine.network.hostname in both side1 and side2 to different values) and update
the assertion on the merged bytes to assert the final value equals the value
from side2 (the later patch), using the existing side1 and side2 variables to
locate and verify the precedence behavior.
🪄 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: 1150a413-3d4f-4b67-81f5-00e6b2345b76
📒 Files selected for processing (26)
README.mdmain.gopkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/completion.gopkg/commands/completion_test.gopkg/commands/contract_apply_chain_stack_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_inplace_preserve_test.gopkg/commands/contract_template_set_help_test.gopkg/commands/contract_upgrade_image_source_test.gopkg/commands/init.gopkg/commands/init_test.gopkg/commands/root.gopkg/commands/root_test.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/commands/upgrade_handler.gopkg/commands/upgrade_image_source.gopkg/engine/contract_setvalue_warning_test.gopkg/engine/engine.gopkg/engine/setvalue_warn.gopkg/modeline/contract_find_and_parse_test.gopkg/modeline/contract_test.gopkg/modeline/modeline.go
💤 Files with no reviewable changes (1)
- pkg/modeline/contract_test.go
✅ Files skipped from review due to trivial changes (2)
- README.md
- main.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/commands/upgrade_image_source.go
- pkg/commands/contract_upgrade_image_source_test.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/commands/init_test.go
- pkg/commands/talosctl_wrapper.go
- pkg/commands/contract_template_set_help_test.go
- pkg/engine/engine.go
- pkg/commands/init.go
- pkg/commands/upgrade_handler.go
- pkg/commands/contract_template_inplace_preserve_test.go
lexfrei
force-pushed
the
feat/commands-followups
branch
3 times, most recently
from
8e1ca6d to
979d6d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/commands/contract_root_detection_test.go (1)
320-325: ⚡ Quick winStrengthen hint assertion to avoid false negatives and better pin the contract.
Line 323 currently uses a case-sensitive
"first"check and doesn’t verify reorder guidance. Use normalized text and assert both concepts explicitly.Proposed test assertion hardening
- hints := strings.Join(errors.GetAllHints(err), "\n") - full := err.Error() + "\n" + hints - if !strings.Contains(full, "first") { - t.Errorf("error / hint must explain that the FIRST file anchors the root; got:\n%s", full) - } + hints := strings.Join(errors.GetAllHints(err), "\n") + full := strings.ToLower(err.Error() + "\n" + hints) + if !strings.Contains(full, "first") || !strings.Contains(full, "reorder") { + t.Errorf("error / hint must explain first-file anchoring and reordering; got:\n%s", full) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/commands/contract_root_detection_test.go` around lines 320 - 325, Normalize the combined error+hint text (the variable full built from err.Error() and hints obtained via errors.GetAllHints) to lowercase and assert it contains both the concept that the FIRST file anchors the root and explicit reorder guidance; e.g., set low := strings.ToLower(full) and assert strings.Contains(low, "first") and (strings.Contains(low, "reorder") || strings.Contains(low, "re-order") || strings.Contains(low, "swap")) so the test in contract_root_detection_test.go more robustly detects case variations and explicit instructions to reorder files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/commands/apply.go`:
- Around line 328-332: The progress output prints []string variables (nodes,
sidePatches, targetNodes, GlobalArgs.Endpoints) using the %s verb which yields
artifacts; update the fmt.Printf calls in pkg/commands/apply.go (the lines that
print "- talm: ..." and the other progress lines referencing
nodes/sidePatches/targetNodes/GlobalArgs.Endpoints) to use a slice-compatible
formatter (e.g., %v) or better, join the slices into a readable comma-separated
string with strings.Join(...) before printing so the output shows clean lists;
ensure you import strings if you use Join and update all occurrences (the two
shown fmt.Printf lines and the later print at line ~457) consistently.
---
Nitpick comments:
In `@pkg/commands/contract_root_detection_test.go`:
- Around line 320-325: Normalize the combined error+hint text (the variable full
built from err.Error() and hints obtained via errors.GetAllHints) to lowercase
and assert it contains both the concept that the FIRST file anchors the root and
explicit reorder guidance; e.g., set low := strings.ToLower(full) and assert
strings.Contains(low, "first") and (strings.Contains(low, "reorder") ||
strings.Contains(low, "re-order") || strings.Contains(low, "swap")) so the test
in contract_root_detection_test.go more robustly detects case variations and
explicit instructions to reorder files.
🪄 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: 94373904-3b3a-41d5-aa47-9f34b14e2ad7
📒 Files selected for processing (26)
README.mdmain.gopkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/completion.gopkg/commands/completion_test.gopkg/commands/contract_apply_chain_stack_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_inplace_preserve_test.gopkg/commands/contract_template_set_help_test.gopkg/commands/contract_upgrade_image_source_test.gopkg/commands/init.gopkg/commands/init_test.gopkg/commands/root.gopkg/commands/root_test.gopkg/commands/talosctl_wrapper.gopkg/commands/template.gopkg/commands/upgrade_handler.gopkg/commands/upgrade_image_source.gopkg/engine/contract_setvalue_warning_test.gopkg/engine/engine.gopkg/engine/setvalue_warn.gopkg/modeline/contract_find_and_parse_test.gopkg/modeline/contract_test.gopkg/modeline/modeline.go
💤 Files with no reviewable changes (1)
- pkg/modeline/contract_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/commands/talosctl_wrapper.go
🚧 Files skipped from review as they are similar to previous changes (17)
- main.go
- pkg/commands/root_test.go
- pkg/commands/contract_template_set_help_test.go
- pkg/engine/engine.go
- pkg/commands/root.go
- pkg/commands/upgrade_image_source.go
- pkg/modeline/modeline.go
- pkg/commands/completion_test.go
- pkg/commands/init_test.go
- pkg/commands/contract_upgrade_image_source_test.go
- pkg/modeline/contract_find_and_parse_test.go
- pkg/engine/setvalue_warn.go
- pkg/commands/apply_test.go
- pkg/commands/template.go
- pkg/commands/completion.go
- pkg/commands/contract_template_inplace_preserve_test.go
- pkg/commands/upgrade_handler.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/commands/upgrade_handler.go (1)
83-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle positional node files in the same path as
--file.Lines 83-88 advertise positional node-file completion, but Lines 102-109 only ingest the
--fileflag. That meanstalm upgrade nodes/controlplane.yamlwill skip root detection, modeline population, andvalues.yamlimage resolution, so it behaves differently from the completed-fflow.Suggested direction
- // Get config files from --file flag - var filesToProcess []string + // Get config files from positional args and --file. + filesToProcess := append([]string(nil), args...) if fileFlag := cmd.Flags().Lookup("file"); fileFlag != nil { if fileFlagValue, err := cmd.Flags().GetStringSlice("file"); err == nil { - filesToProcess = fileFlagValue + filesToProcess = append(filesToProcess, fileFlagValue...) } }If mixed positional/flag forms are intentionally unsupported, reject that combination explicitly instead of silently ignoring the positional path here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/commands/upgrade_handler.go` around lines 83 - 109, The handler currently only reads --file into filesToProcess and ignores positional args; update wrappedCmd.RunE so positional node files are handled consistently: after populating filesToProcess from cmd.Flags().Lookup("file") check the incoming args slice and if both filesToProcess and args are non-empty return an explicit error (reject mixed flag+positional), otherwise if filesToProcess is empty and args is non-empty set filesToProcess = args so positional invocation (the args variable in RunE) goes through the same root detection/modeline/values.yaml flow; keep validatePostUpgradeReconcileWindow(upgradeCmdFlags.postUpgradeReconcileWindow) and the variable name filesToProcess unchanged so subsequent code paths remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 17: Replace the unclear word "modelined" in the README sentence "Per-node
configuration sits in a single modelined `nodes/<name>.yaml`" with clearer
operator-facing wording such as "modeline-annotated" or "with a modeline" so the
sentence reads e.g. "Per-node configuration sits in a single modeline-annotated
`nodes/<name>.yaml`" (update the README entry that includes this sentence).
---
Outside diff comments:
In `@pkg/commands/upgrade_handler.go`:
- Around line 83-109: The handler currently only reads --file into
filesToProcess and ignores positional args; update wrappedCmd.RunE so positional
node files are handled consistently: after populating filesToProcess from
cmd.Flags().Lookup("file") check the incoming args slice and if both
filesToProcess and args are non-empty return an explicit error (reject mixed
flag+positional), otherwise if filesToProcess is empty and args is non-empty set
filesToProcess = args so positional invocation (the args variable in RunE) goes
through the same root detection/modeline/values.yaml flow; keep
validatePostUpgradeReconcileWindow(upgradeCmdFlags.postUpgradeReconcileWindow)
and the variable name filesToProcess unchanged so subsequent code paths remain
valid.
🪄 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: 4c3b19bc-be64-4a88-a7b7-5f25b8c2f7c3
📒 Files selected for processing (26)
README.mdmain.gopkg/applycheck/diff.gopkg/commands/apply.gopkg/commands/completion.gopkg/commands/completion_test.gopkg/commands/contract_apply_chain_stack_test.gopkg/commands/contract_init_ux_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_inplace_preserve_test.gopkg/commands/contract_upgrade_image_source_test.gopkg/commands/init.gopkg/commands/preflight_apply_safety_redact.gopkg/commands/preflight_test.gopkg/commands/preflight_upgrade_verify.gopkg/commands/reset_handler_test.gopkg/commands/root.gopkg/commands/root_test.gopkg/commands/talosctl_wrapper_test.gopkg/commands/template.gopkg/commands/upgrade_handler.gopkg/commands/upgrade_handler_test.gopkg/commands/upgrade_image_source.gopkg/engine/contract_network_multidoc_test.gopkg/modeline/modeline.go
✅ Files skipped from review due to trivial changes (9)
- pkg/applycheck/diff.go
- pkg/commands/contract_init_ux_test.go
- pkg/commands/preflight_apply_safety_redact.go
- pkg/commands/preflight_test.go
- pkg/commands/preflight_upgrade_verify.go
- pkg/commands/reset_handler_test.go
- pkg/engine/contract_network_multidoc_test.go
- pkg/commands/talosctl_wrapper_test.go
- pkg/commands/upgrade_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/commands/init.go
- main.go
- pkg/commands/root_test.go
- pkg/modeline/modeline.go
- pkg/commands/upgrade_image_source.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/commands/contract_template_inplace_preserve_test.go
- pkg/commands/root.go
- pkg/commands/contract_upgrade_image_source_test.go
- pkg/commands/completion_test.go
- pkg/commands/apply.go
- pkg/commands/template.go
- pkg/commands/contract_apply_chain_stack_test.go
- pkg/commands/contract_root_detection_test.go
- pkg/commands/completion.go
lexfrei
force-pushed
the
feat/commands-followups
branch
2 times, most recently
from
c143e4b to
28d960e
Compare
…date
readChartYamlPreset returns errors.WithHint('preset not found in
Chart.yaml dependencies', 'add a preset chart…'); the call site in
updateTalmLibraryChart wrapped that again with errors.Wrap(err,
'preset is required: use --preset flag or ensure Chart.yaml has a
preset dependency'). The rendered error became 'preset is required:
…: preset not found in Chart.yaml dependencies' — the same fact
twice. Return the inner error directly; its hint already covers
both recovery paths.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
…-string
Helm's strvals.ParseInto interprets dots in the RHS as YAML key
nesting, so 'talm template --set endpoint=10.0.0.1' silently
produces {endpoint: {10: {0: {0: 1}}}} — corrupt config the
operator notices only when the rendered manifest is wrong. Add a
pre-parse screen that emits a one-line stderr warning when the
value matches an IPv4 / IPv4 CIDR / IPv6 / semver shape, pointing
at --set-string as the fix. The screen is non-fatal: existing
automation continues to render (with the broken nested map).
Help text on --set and --set-string now surfaces the trade-off so
operators reading --help find the hint before hitting the trap.
README gains a short paragraph with worked examples.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
feat/commands-followups
branch
from
28d960e to
3295889
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/commands/apply.go`:
- Around line 243-249: The early error when len(GlobalArgs.Nodes) == 0
incorrectly aborts runs where nodes will be populated from the talosconfig
context or direct-patch flow; instead, remove or guard this early return and
only error if nodes remain empty after the talosconfig/direct-patch resolution
step. Concretely: stop returning the WithHint error in the GlobalArgs.Nodes
check shown (using GlobalArgs.Nodes and configFile), and move this validation to
run after the code path that resolves targets from talosconfig/direct-patch (the
function that populates GlobalArgs.Nodes), or wrap the check so it only triggers
when talosconfig/direct-patch resolution is not enabled. Ensure the final
behavior is: attempt talosconfig/direct-patch resolution first, then if
GlobalArgs.Nodes is still empty, return the existing error message.
🪄 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: bab5b48d-3af9-43d7-b4dd-f84dc30f2808
📒 Files selected for processing (38)
README.mdmain.gopkg/applycheck/diff.gopkg/commands/apply.gopkg/commands/apply_test.gopkg/commands/completion.gopkg/commands/completion_test.gopkg/commands/contract_apply_chain_stack_test.gopkg/commands/contract_init_ux_test.gopkg/commands/contract_root_detection_test.gopkg/commands/contract_root_dispatch_test.gopkg/commands/contract_template_inplace_preserve_test.gopkg/commands/contract_template_set_help_test.gopkg/commands/contract_upgrade_image_source_test.gopkg/commands/init.gopkg/commands/init_test.gopkg/commands/preflight_apply_safety_redact.gopkg/commands/preflight_test.gopkg/commands/preflight_upgrade_verify.gopkg/commands/reset_handler_test.gopkg/commands/root.gopkg/commands/root_test.gopkg/commands/talosctl_wrapper.gopkg/commands/talosctl_wrapper_test.gopkg/commands/template.gopkg/commands/upgrade_handler.gopkg/commands/upgrade_handler_test.gopkg/commands/upgrade_image_source.gopkg/engine/contract_network_multidoc_test.gopkg/engine/contract_setvalue_warning_test.gopkg/engine/engine.gopkg/engine/engine_test.gopkg/engine/helm/funcs_test.gopkg/engine/render_test.gopkg/engine/setvalue_warn.gopkg/modeline/contract_find_and_parse_test.gopkg/modeline/contract_test.gopkg/modeline/modeline.go
💤 Files with no reviewable changes (1)
- pkg/modeline/contract_test.go
✅ Files skipped from review due to trivial changes (8)
- pkg/engine/helm/funcs_test.go
- pkg/applycheck/diff.go
- pkg/engine/engine_test.go
- pkg/commands/preflight_apply_safety_redact.go
- pkg/commands/reset_handler_test.go
- pkg/engine/render_test.go
- pkg/commands/preflight_test.go
- pkg/commands/talosctl_wrapper_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- pkg/engine/engine.go
- pkg/engine/contract_network_multidoc_test.go
- pkg/commands/talosctl_wrapper.go
- pkg/commands/preflight_upgrade_verify.go
- pkg/commands/contract_init_ux_test.go
- main.go
- pkg/commands/contract_template_set_help_test.go
- pkg/commands/upgrade_image_source.go
- pkg/commands/init.go
- pkg/engine/setvalue_warn.go
- pkg/commands/root.go
- pkg/commands/init_test.go
- pkg/commands/contract_apply_chain_stack_test.go
- pkg/commands/root_test.go
- pkg/commands/completion.go
- pkg/commands/contract_upgrade_image_source_test.go
- pkg/commands/apply_test.go
- pkg/commands/upgrade_handler.go
- pkg/commands/contract_root_detection_test.go
- pkg/modeline/modeline.go
- pkg/engine/contract_setvalue_warning_test.go
- pkg/commands/contract_root_dispatch_test.go
- pkg/commands/contract_template_inplace_preserve_test.go
- pkg/modeline/contract_find_and_parse_test.go
lexfrei
force-pushed
the
feat/commands-followups
branch
5 times, most recently
from
edc7b80 to
2fbc9e9
Compare
lexfrei
changed the title
…rewrite `talm template -I` previously parsed the node file via a strict first-line-only modeline reader. Operator-authored comments above the modeline (project documentation, deployment notes, context hints) were either rejected at parse time or lost on rewrite. The intended `-I` behavior is regeneration that preserves the operator's leading block. Adds `modeline.FindAndParseModeline` that scans the file allowing `#`-prefixed comment lines and blank lines as a leading prefix, returns them verbatim alongside the parsed modeline, and rejects any non-comment line that precedes the modeline. `talm template -I` prepends the captured leading block back to the regenerated file so operator documentation survives. The strict first-line-only `ReadAndParseModeline` is removed — `FindAndParseModeline` is now the single entry point for every node-file consumer (apply, upgrade, completion, wrapped talosctl commands, template). The file-shape contract is uniform across the talm surface. The classifier distinguishes three file shapes via `ErrModelineNotFound`: modeline-on-top (parsed and returned), orphan (no `# talm:` line anywhere — surfaces ErrModelineNotFound so callers route to the side-patch / direct-patch path), and misplaced (a `# talm:` line sits below YAML content — rejected with a specific hint). "Modeline below YAML" detection is column-0-strict: indented `# talm:` text inside the YAML body is treated as a regular comment, not a misplaced modeline, so operator-authored documentation comments referencing the modeline don't falsely trip the gate. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
talm upgrade previously called engine.FullConfigProcess on the node body file and extracted machine.install.image from the result — that is, the image already baked into the LAST 'talm template' render. Operators bumping values.yaml::image to upgrade their cluster saw the old image silently reused until they re-rendered every node file. values.yaml is the source of truth for cluster-wide knobs; the upgrade target now reads from there directly. The new resolveUpgradeImageFromValues helper parses values.yaml under Config.RootDir, returns the 'image' field, and surfaces operator-facing errors with hints when the field is missing or the file is unreachable. Per-node override remains available via --image, which short-circuits the resolution. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
feat/commands-followups
branch
from
2fbc9e9 to
93aa0de
Compare
ValidateAndDetectRootsForFiles previously iterated EVERY -f file and rejected the chain if any file lacked a Chart.yaml + secrets ancestor OR resolved to a divergent project root. That gate blocked the natural side-patch UX: 'talm apply -f nodes/n.yaml -f /tmp/extra.yaml' should layer /tmp/extra.yaml on top of the project anchored by nodes/n.yaml, but the gate refused the chain because /tmp/extra.yaml has no project root. Switch to first-file-anchors semantics: only filePaths[0] is required to resolve a project root, and that root is returned as the anchor. Subsequent files skip detection entirely and are loaded as patches downstream. Reversing the chain (orphan first, rooted second) is rejected with a hint pointing at the FIRST file and instructing the operator to reorder, not to move the file. Single-file orphans continue to error out identically to pre- change behaviour. Existing tests that pinned the strict-consistency rule (_DifferentRootsError, _OrphanFileError, _FilesInDifferentRootsError) are rewritten as positive contracts under the new semantics; the new tests cover the side-patch acceptance, the reorder-hint rejection, and the no-regression single-orphan case. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…talosconfig
Cobra's __complete subcommand was wired (via cobra defaults) but
talm registered no per-flag or per-positional completion targets.
Operators tab-completing flag values got the generic file
completion fallback, even for fixed enums (--mode) or enumerable
sets (--preset).
This change adds a per-target completion module
(pkg/commands/completion.go) with:
- --preset (talm init): enumerate generated.AvailablePresets()
- --mode (talm apply): fixed enum {auto, no-reboot, reboot,
staged, try} matching upstream helpers.AddModeFlags
- --file, --values, --template, --with-secrets: filter file
completion to *.yaml / *.yml
- positional file args on apply / template / upgrade: scan
the project's nodes/ directory, return only YAML files with a
talm modeline
- --nodes, --endpoints (root persistent): load the in-scope
talosconfig (--talosconfig > $TALOSCONFIG > project-local
talosconfig) and return the union of the requested field
across every context
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Per the project's "describe content, not the workflow that produced it" rule, code comments, help text, README, and test docstrings must not embed bug/ticket numbers — a reader of git log six months from now, with no access to GitHub, must understand what the code does without grepping issue trackers. The same rule applies to external-tracker references and Telegram message IDs. Sweeps every #NNN and similar reference out of: - Operator-facing surface: applyCmd Long, --file Usage, --skip-post-apply-verify and --skip-post-upgrade-verify Usage, templateCmd Long, README copy. These are visible in --help and the project README. - Source comments and test docstrings across pkg/commands/, pkg/modeline/, pkg/applycheck/, pkg/engine/, main.go. Each reference is rewritten to describe the contract, the behaviour, or the failure mode it captures, not the ticket that prompted the work. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
feat/commands-followups
branch
from
93aa0de to
8f17918
Compare
This was referenced May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes the entire
area/commandsfollow-up queue in one cycle. Six independent issues bundled because they all touch the talm CLI surface (init / template / apply / upgrade / completion) and share an init() wiring style. One review cycle, six squashed commits, one ping.Six logical commits (smallest → largest scope):
fix(init): drop redundant error wrap when --preset is missing on --update— 1-line fix, single hint surfaces;readChartYamlPresetalready wraps +WithHint.feat(engine,template): warn on IP-shaped --set values; document --set-string— pre-parse heuristic warns when--set <k>=<v>RHS matches a valid IPv4 / IPv4 CIDR / three-component semver; colon-separated literals (IPv6, MAC, timestamps) and bare two-component decimals (cpu=1.5) are deliberately not flagged. Help text + README cover the trade-off.fix(template): preserve operator comments above modeline on in-place rewrite— newmodeline.FindAndParseModelineaccepts leading#-prefixed and blank lines;talm template -Iprepends them back. Every consumer (apply, upgrade, completion, talosctl wrappers) uses the same parser so file shape is uniform across the CLI surface.fix(upgrade): take target image from values.yaml, not rendered node body— bumpingvalues.yaml::imagenow upgrades the cluster without re-runningtalm template.--imageflag override unchanged.feat(apply): first -f file anchors project root; later files are patches—talm apply -f nodes/n.yaml -f /tmp/side.yamlnow succeeds; reverse order errors with reorder hint. Single-orphan still rejected.feat(commands): rich shell autocompletion for presets, modes, files, talosconfig— per-flag + positional completions:--presetenumeratesgenerated.AvailablePresets,--modereturns the upstream enum, file flags filter*.yaml/*.yml, positional file args list modelined files undernodes/,--nodes/--endpointsdraw from in-scope talosconfig contexts.Why
Six small operator-facing pains, each minor on its own but visible day-to-day. Bundled so the review cycle costs one cadence, not six. No public API additions — every change tightens existing operator UX or fixes a documented bug.
Verification
go build ./...clean.go test ./... -count=1 -raceall packages green.golangci-lint run ./...0 issues.GOOS=windows go vet ./...clean.Real-environment smokes (dev17, 3-node OCI Talos):
values.yaml::image,talm upgradereads the new value (vs. previous behaviour: stuck on the last templated image).talm template -Ire-rendered with the comment block preserved verbatim; the SAME file then accepted bytalm apply --dry-runend-to-end (the cross-parser handoff is the contract test that pinned the missing piece).talm template --set endpoint=10.0.0.1 …emits the--set-stringwarning on stderr;--set-stringform does not.talm apply --dry-run -f nodes/n.yaml -f /tmp/side.yaml(side patch outside project) succeeded; reverse order failed with reorder hint.talm init --preset <TAB>listscozystackandgeneric;talm apply --mode <TAB>lists the five upstream modes;talm upgrade <TAB>lists modelined files undernodes/.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests