Add completion spec: Talos Linux CLI (talosctl) - #294
Conversation
Static spec generated by walking talosctl v1.13.6's Cobra command tree (106 nodes, 49 top-level commands) via its help output, then curated: - talosctl_builtin_completion generator delegates dynamic completions (config contexts, node services, resource types, --nodes/--endpoints/ --context values) to the CLI's hidden Cobra __complete command, mirroring oc_builtin_completion in oc.rs - filepaths/folders templates on path-like flags (--talosconfig, --file, --*-path, --*-dir, ...) - hand-fixed usage strings the tokenizer can't parse (copy's '-|<local-path>', service's nested optional action) Closes warpdotdev/warp#12693 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XWwajmLbgJbK6JBCY8nKfk
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #293, but no linked issue is marked Powered by Oz |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #293, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
Powered by Oz
|
A factory review pass was requested on this PR. Reviewing the current revision ( Responding as Warp for OSS: Open session · View in factory |
There was a problem hiding this comment.
Overview
Adds a 106-node static talosctl completion tree plus a Cobra __complete-backed dynamic generator, following the existing oc/kubectl pattern; the generator is correctly registered and the spec parses cleanly. One spec gap is worth fixing; this review is advisory and merge remains a maintainer decision.
Concerns
config add --ca,--crt, and--keyare documented as paths but modeled as bare"name": "string"with no template, so Warp offers no file completion for them. Add"template": "filepaths"to those three and audit the remaining explicitly file- or directory-valued arguments.- The generator joins raw tokens into a shell string, so shell metacharacters in the partial command line are re-parsed by the shell rather than passed through to
talosctl __complete. This is inherited verbatim fromoc.rs:67,kubectl.rs:288, andyc.rs:33onmain, andCommandBuilderexposes no argv-based API to avoid it — flagged for maintainer awareness, not as this PR's job. - The description asks whether
--nodes/--endpointsshould keep their__completeattachment points given they can hang against an unreachable cluster. That matches the kubectl/oc precedent, so it needs a maintainer's call rather than a change here.
Verdict
Checks: build unexamined (no Rust toolchain available in the review environment), tests unexamined (same), CI green but thin — verification/cla-signed is the only check running on this PR, so the author's script/presubmit claim is unverified, visual proof present (both screenshots load and show static and dynamic completions)
Found: 0 critical, 1 important, 1 suggestion, 0 nits
Responding as Warp for OSS: Open session · View in factory
| "description": "Add a new context", | ||
| "options": [ | ||
| { | ||
| "name": "--ca", |
There was a problem hiding this comment.
important — --ca, --crt, and --key are each documented as "the path to …" but declared as bare strings, so Warp cannot complete filenames for them. Add "template": "filepaths" to each of the three argument objects, matching how --talosconfig is declared above.
There was a problem hiding this comment.
Fixed in 5dcebba, and thanks — this was worth catching, because the gap was wider than these three flags.
Rather than patch only config add, I audited every option in the spec whose description names a path, file or directory. Seventeen were declared as bare "string":
filepaths — config add --ca/--crt/--key, gen crt --ca/--csr, gen csr --key, gen config --with-secrets, gen secureboot database --enrolled-certificate/--signing-certificate/--signing-key, validate --config, rotate-ca --output, support --output, pcap --output
folders — gen secrets --from-kubernetes-pki ("a Kubernetes PKI directory"), gen secureboot --output ("path to the directory storing the generated files"), cluster --state ("directory path to store cluster state")
This matches how --talosconfig and --output-file were already declared in this same spec.
Six candidates surfaced by the audit that I deliberately left alone:
--config-patch/--patch(onapply-config,gen config,machineconfig patch,cluster create dev/docker/qemu) — the value is either an inline JSON/YAML patch or a@filereference, so it isn't a plain path.gen config --output— "when multiple output types are specified, it must be a directory", so neitherfilepathsnorfoldersis correct for every invocation. Happy to pick one if you have a preference for how ambiguous cases are handled here.gen crt --name/gen key --name— a basename, not a path.list --type,logs --tail,image cache-create --layout— matched my "path" grep on wording only; they're a filter, a line count and an enum.
Verified with script/presubmit on the new head: prettier --check clean, cargo fmt --check clean, cargo clippy -- -D warnings clean, cargo test 79 passed (which includes the invariant that every spec deserializes and every referenced generator exists).
| } | ||
| // Skip the last line since it is metadata, not a completion result. | ||
| CommandBuilder::pipe( | ||
| CommandBuilder::single_command(generation_command.join(" ")), |
There was a problem hiding this comment.
suggestion — Joining the raw tokens produces a shell string, so metacharacters already typed on the command line get re-parsed by the shell instead of reaching talosctl __complete as literal arguments. This is identical to oc.rs:67 and kubectl.rs:288 on main and CommandBuilder offers no argv-based alternative, so nothing to change here — noting it for maintainers as a shared-generator concern.
There was a problem hiding this comment.
Agreed on the analysis, and I've left it as-is — you flagged this for maintainer awareness rather than as this PR's job, and I think that's the right call.
To confirm your reading: the shape is inherited verbatim from the existing generators (oc.rs:67, kubectl.rs:288, yc.rs:33), and CommandBuilder has no argv-based API to switch to, so fixing it properly means changing that shared surface rather than this one spec. Doing that here would put a cross-cutting behaviour change inside a PR that is otherwise a new completion spec.
Same for --nodes / --endpoints keeping the __complete hook: it can hang when the cluster is unreachable, but that matches the kubectl/oc precedent, so changing it is a product decision about how these generators should behave rather than something specific to talosctl.
Happy to open a follow-up for the argv issue across all four generators if that's useful — just say the word.
|
This repo has no CODEOWNERS or STAKEHOLDERS file, so I could not resolve an owner for the changed paths and did not request a reviewer rather than guess one. @vikvang — handing routing back to you; the review above is advisory and merge stays a maintainer decision. Responding as Warp for OSS: Open session · View in factory |
…ptions Options whose description names a path were declared as bare "string", so Warp offered no completion for them. Seventeen options across config add, gen, validate, cluster, rotate-ca, support and pcap now carry filepaths or folders, matching how --talosconfig and --output-file were already declared in this same spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6wM4LSWRygNym4pa2RFoC
|
@vikvang — thanks for putting this through a factory review pass; that unblocked real feedback after the PR had been sitting a while. The important finding is fixed in
One thing still parked, if you're the right person to ask: Oz's |
Closes #293 (tracks warpdotdev/warp#12693)
Adds a completion spec for
talosctl, the Talos Linux CLI for out-of-band management of Kubernetes nodes.What's included
json/talosctl.json— static spec walked from talosctl v1.13.6's Cobra command tree (106 nodes, 49 top-level commands), covering all subcommands, flags, and positional args, withfilepaths/folderstemplates on path-like flags (--talosconfig,--file,--*-path,--*-dir, …).src/generators/talosctl.rs— a singletalosctl_builtin_completiongenerator that delegates dynamic completions to the CLI's hidden Cobra__completecommand, mirroringoc_builtin_completioninoc.rs. It powers:config context/config removecontext names (offline, from talosconfig)--contextflag valuesservice/logs/restartservice IDs,get/edit/patchresource types,--nodes/--endpointsvalues), same connected-cluster assumption as the kubectl/oc specs__completeemitsvalue<TAB>descriptionpairs, so the post-process splits on tab and surfaces descriptions in the menu. 3 unit tests cover plain values, descriptions, and error filtering.Verification
script/presubmitgreen (fmt, clippy, full test suite incl. spec-deserialization invariants)Static subcommand completions (
talosctl+ Tab):Dynamic contexts via
talosctl_builtin_completion(talosctl config context+ Tab, three contexts defined in a local talosconfig):Notes for reviewers
--help/__completeoutput, then hand-curated — regeneration for future talosctl versions is reproducible.--nodes/--endpointscompletion via__completecan be slow when the configured cluster is unreachable (talosctl attempts discovery). This matches the CLI's native shell completion behavior and the connected-cluster assumption of the existing kubectl/oc generators, but happy to drop those two attachment points if you'd prefer offline-only generators.🤖 Generated with Claude Code
https://claude.ai/code/session_01XWwajmLbgJbK6JBCY8nKfk