Repository navigation
fix(cli): kates ctx use, delete, export, import and current exit 1 when they fail - #380
Merged
Merged
Conversation
…en they fail In a step like "kates ctx import --file ci-contexts.yaml && kates ctx use lab && kates test create --type STRESS ...", a file with no lab context made ctx use print "Context 'lab' not found" and exit 0, so the test went to whichever context was current. ctx delete and ctx export --name did the same for a missing context (export > prod.yaml left an empty file), ctx import for a file it could not read, parse or save, and ctx current for an unknown --context or no active context. Only ctx set had been moved to RunE. These commands now use RunE and return cmdErr, which prints the same message and exits 1. TestCtxFailuresExitOne runs each failing case through the real command tree and checks the exit code; TestCtxCommands expects an error from the missing-context rows, and the ctx export tests' helper calls RunE and returns its error to commandOutput. The book no longer lists these commands under the ones that exit 0 on failure, and the MCP eval harness's comment and its fake CLI say ctx export exits 1 for a missing context.
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
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.
Why
Five
kates ctxcommands print an error when they fail and then exit 0. A script cannot tell that they failed.Take a CI step or runbook like
kates ctx import --file ci-contexts.yaml && kates ctx use lab && kates test create --type STRESS .... Suppose the file has nolabcontext, because it was renamed or mistyped.ctx useprintsContext 'lab' not foundand exits 0. The&&chain goes on, and the test runs against whichever context was current. On an engineer's machine that may be prod. The same goes fordisruption runin place oftest create.Checked in
cli/cmd/config.goon main:ctxUseCmd(:151),ctxDeleteCmd(:179),ctxCurrentCmd(:215),ctxExportCmd(:320) andctxImportCmd(:375) useRun. Each failure isoutput.Error(msg); return, andRunhas no error to return. The failures are:ctx useandctx delete: a missing context, or a failed save (:165/:168, :199/:202).ctx current: an unknown--context(:225). With no active context it only warns (:218).ctx export --name: a missing context (:330), or a failed marshal (:357).kates ctx export --name prod > prod.yamlleaves an empty file and exits 0.ctx import: a file it cannot read (:384), invalid YAML (:390), or a failed save (:405).ctx set's comment (:85-87) names this exact bug, but onlyctx sethad been moved toRunE.TestCtxCommands(root_test.go:429-448) called.Run, then returnednilwhatever happened.The plan also found two places the fix as written missed:
ctx_export_test.go:22-34:runCtxCommandLinecallscmd.Run. Once export and import useRunE,Runis nil, so every export and import test there would panic.docs/book/10-cli-reference.md:2490lists these commands under "Commands that exit 0 on failure".Changes
cli/cmd/config.goctx use,ctx delete,ctx current,ctx exportandctx importuseRunE. Eachoutput.Error(msg); returnis nowreturn cmdErr(msg), and each success path ends inreturn nil.cmdErrprints the message withoutput.Errorand returns asilentErr(helpers.go:30-33).Executeturns that into exit 1 and does not print it a second time (root.go:591-598).ctx currentwith no active context returnscmdErrtoo, so its message now prints as an error, not as a warning (see Decisions).ctxUseCmdhas a 3-line comment that names the&&chain case, the wayctx set's comment does.ctx showhas no failure path and staysRun.cli/cmdtestsctx_export_test.go:runCtxCommandLinecallsRunEwhen the command has one. It falls back toRunforctx show, and returns the error. All 12 callers pass that error back throughcommandOutput.root_test.go:TestCtxCommandsand the ports import test callRunE(see Tests).context_test.go: newTestCtxFailuresExitOne.Docs
kates ctx listprints the group's help), which this PR does not change.eval/mcpkates_api.py:read_context's comment saysctx exportexits 1 for a missing context, and that older CLIs exited 0 with nothing on stdout. The check already fails on either, so the code is unchanged.testdata/fake_kates.py: the fake CLI exits 1 for a missing context, as the real one now does.Behaviour change. A script that ran one of these commands and ignored a failure now stops at it. The repo's own callers are safe:
Makefile:737and:741runkates ctx use local … || true.scripts/install-kates.sh:187only prints the command as a hint.scripts/or.github/calls these commands.Tests
Added or changed:
TestCtxFailuresExitOne(cli/cmd/context_test.go). It usesrunKatesCLI, which runs the real command tree in a subprocess and returns the exit code. Each of these exits 1, and none printscontexts:YAML:ctx use nopectx delete nopectx export --name nopectx import --file <missing path>ctx import --file <file with invalid YAML>--context nope ctx currentThe control case,
ctx use lab, exits 0. This is the&&chain from Why.TestCtxCommands(cli/cmd/root_test.go): each row callsRunEand returns its error. A newwantErrfield is set on "use of a missing context changes nothing" and "delete of a missing context changes nothing". The loop checks(err != nil) != tt.wantErrrather than failing on any error.TestCtxImport_ClearsKeySource(root_test.go:393): callsRunEand fails on an error.Run locally, all passing:
git diff --name-only origin/main...HEAD: 1 commit.cli/cmd/{config.go,context_test.go,ctx_export_test.go,root_test.go},docs/book/10-cli-reference.md,eval/mcp/kates_api.pyandeval/mcp/testdata/fake_kates.py.ci.ymlcode,ci-docker,integrationandcodeql(cli/**), andbook.ymlanddocs.yml(docs/book/**).eval/mcp, so its tests were run as an extra check.cd cli && go test ./...: every package ok,cli/cmdin 31.8 s.cd cli && go vet ./...: no output.cd cli && golangci-lint run --new-from-rev=origin/main ./...: 0 issues.cd cli && govulncheck ./...: no vulnerabilities found.scripts/check-cli-style.shandscripts/check-cli-compat.sh: OK.scripts/check-book-style.sh: OK. All 38 pages are within the baseline ratchet.scripts/gen-version-matrix.sh --checkandscripts/gen-chart-table.sh --check: in sync.python3 scripts/check-book-contrast.py: all 170 colour pairs meet WCAG AA.scripts/check-mermaid.sh --quiet: 156 diagrams, 0 invalid.scripts/prepare-book-render.sh && quarto render docs/book --to html, on a scratch copy ofdocs/andscripts/: the render log has noUnable to resolveorWARNline.python3 scripts/check-book-fragments.pyon that render: all 7,458 fragment links in 38 pages land on an id.eval/mcp:python3 -m unittest discover eval/mcp: 240 tests OK. 4 are skipped because they needKATES_BIN.KATES_BINset to akatesbuilt from this branch: 240 tests OK, 0 skipped. That includes the cross-check against the real CLI, which covers the newctx exportexit code.The backend is unchanged. The full backend suite (
./mvnw test) ran on main and passed, and gives the same result here, so it was not run again on this branch.Not run, because nothing they check changed:
helm lint,helm template,scripts/check-versions.shandyamllint: nocharts/**or version pins changed.make check-help: theMakefiledid not change.actionlint:.github/**did not change.shellcheck: no shell script changed.Run only in CI:
*ITintegration tests and the Kind integration tests (integration.yml, triggered bycli/**).ci-docker.yml, triggered bycli/**).codeql.yml, triggered bycli/**/*.go).index.logfont check (book.yml).docs.ymlMarkdown Link Check,lychee --offline --include-fragments. lychee is not installed locally.ci.ymlBackend Build & Test (mvnw verify) on the code filter. It was skipped locally becausekates/is unchanged.kates/**is unchanged.Decisions
ctx currentwith no active context also exits 1. It used to only warn.kubectl config current-contextalso exits 1 when no context is set. The case is rare, because a fresh config has the built-indefaultcontext.ctx importof a file with no contexts stays as it is. It still reportsImported 0 context(s)and exits 0. The finding does not ask for a change there.Deliberately not done
TestCtxExport_MissingNameFails. The plan marked it optional.TestCtxFailuresExitOnealready checks thatctx export --name nopeexits 1 and prints nocontexts:YAML.kates ctx listprints the group's help. It stays in book 10's list. So dokates status,kates portsandkates snapshot create, which belong to other findings.ctxUseCmdand the updated doc comment onrunCtxCommandLine.Landing
Lands after #373 (plan PR 4), #375 (plan PR 6) and #378 (plan PR 9). Each one edits
docs/book/10-cli-reference.md, and so does this PR. No other file is shared.Recipe once those have merged:
mcpAnchoredReforpath:linecomment that cites a file this PR edits. A grep ofcli/cmdandeval/mcpagrees, so the map should leave nothing to re-point.cd cli && go test ./cmd -run TestMCPCaveat.