Commit ca9f5d4
committed
fix(commands+docs): branch-review pass 2 — Phase 2C insecure/staged skip + linksDisksReader three-valued + md reflow
Blockers from branch-review pass 2 (#173):
1. Phase 2C ran WithClient even on --insecure upgrade. The
maintenance / pre-auth connection cannot reach the auth-only
COSI path; pre-fix the gate either silently surrendered with a
'version unreadable' line or, worse, connected to an unrelated
node from the talosconfig context and verified its state.
Extract shouldRunPostUpgradeVerify(insecure, staged, skip)
pure predicate; capture --insecure flag BEFORE original RunE
(alongside --image and --stage). Mirrors
cosiMachineConfigReader's insecure-path branch in apply.
2. Phase 2C ran after --stage upgrades — guaranteed false positive
blocker since the new partition isn't activated until the next
reboot and runtime.Version still reports the OLD version. Same
predicate now skips on --stage, mirroring
shouldRunPostApplyVerify's STAGED case for apply.
3. docs/manual-test-plan.md K2-pre block claimed Phase 2C 'until
it lands', contradicting the new K1-pre section right above and
the README addition. Reworked K2-pre as the manual fallback for
--skip-post-upgrade-verify / --insecure flows that the gate
skips by design. K1-pre's expected output is synced with the
actual emit (two-hypothesis hint, waiting line, skip list).
4. cosiLinksDisksReader had a two-valued (snapshot, ok) signature
that conflated transient COSI errors with auth-disallowed.
Mirror machineConfigReader's three-valued
(snapshot, ok, err) shape:
- err != nil -> transient (apid timeout, network blip);
surface underlying cause, the operator's config isn't
wrong.
- !ok && err==nil -> auth-disallowed / resource unreachable
by design; suggest --skip-resource-validation.
- ok -> success.
preflightValidateResources splits the two branches into
distinct error messages (no more misleading 'config is wrong'
on a 2s COSI timeout). New regression test
TestPreflightValidateResources_TransientErr_WrapsUnderlying
pins the contract.
5. Phase 2C silent no-op when both --nodes and talosconfig
context's Nodes are empty. Now prints an explanatory line so
the operator sees the gate they opted into didn't actually
run.
Recommendations addressed:
- Phase 2C 90s wait now prints a 'waiting...' line up front so
the operator's terminal isn't a mystery hang.
- targetImage == '' path prints a skip notice instead of silent
no-op.
- transportNVMe const split into transportNVMe (COSI Disk.Transport
value) + selectorTypeNVMe (v1alpha1 InstallDiskSelector.type
enum). They're the same string today; documenting the contract
separation prevents a silent future regression.
- Phase 2C error message reworded to lead with both hypotheses
('auto-rolled back OR still booting'), matching the two-
hypothesis hint constant.
Documentation reflow (separate, no behaviour change): user pushed
back on hardwrapped prose in human-rendered .md and PR/issue
bodies. Reflowed docs/manual-test-plan.md,
docs/apply-safety-gates-test-plan.md, nit.md, GitHub issue #178,
and PR #173 body to one-line-per-paragraph per CLAUDE.md's prose
rule. Tables, code blocks, and bullet structure preserved
verbatim.
Recommendation NOT addressed in this pass: the
parentDir-shadowing-the-package-constant nit. Pre-existing across
~6 sites; folding it into this PR adds noise. Worth a follow-up
issue (kind/cleanup, area/commands, good first issue).
Refs: #172, #175
Signed-off-by: Aleksei Sviridkin <f@lex.la>1 parent 3525abf commit ca9f5d4
6 files changed
Lines changed: 286 additions & 252 deletions
File tree
- docs
- pkg
- applycheck
- commands
0 commit comments