Repository navigation
feat : add consistent exit codes - #3130
vikashkumar2020 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@vikashkumar2020 thanks for the PR! |
added into description |
divyansh42
left a comment
There was a problem hiding this comment.
The PR wraps errors in pkg/actions (get, list, delete, patch) but many commands produce errors through other paths: direct client calls in trigger/bundle commands, flag validation errors in RunE, etc. The issue asks for consistent exit codes "across all commands."
This is acceptable as a first pass, but please file a follow-up issue tracking the remaining commands so this doesn't fall through the cracks.
| if err == nil { | ||
| return nil | ||
| } | ||
| switch { |
There was a problem hiding this comment.
The InvalidInput constant is declared but nothing in this PR ever emits exit code 3. FromAPIError doesn't map k8serrors.IsInvalid() or k8serrors.IsBadRequest() to it, and Cobra flag-parsing errors also bypass this classification entirely.
We can probably do
case k8serrors.IsInvalid(err), k8serrors.IsBadRequest(err):
return &Error{Code: InvalidInput, Message: err.Error()}| Message string | ||
| } | ||
|
|
||
| func (e *Error) Error() string { |
There was a problem hiding this comment.
FromAPIError creates a new *exitcode.Error with only the string message, discarding the original Kubernetes *StatusError. Any downstream code that calls k8serrors.IsNotFound(err) or errors.As(err, &statusErr) after a pkg/actions call will now get false because the original error is no longer in the chain.
Can we store the original and implement Unwrap():
type Error struct {
Code int
Message string
Err error
}
func (e *Error) Error() string { return e.Message }
func (e *Error) Unwrap() error { return e.Err }And update FromAPIError to preserve the original:
return &Error{Code: NotFound, Message: err.Error(), Err: err}This preserves both the exit code semantics and the original error chain
| // "json", the error is serialised as {"error":"<message>","code":<n>}. | ||
| // Otherwise the standard "Error: <message>\n" format is used. | ||
| func PrintError(cmd *cobra.Command, err error, errW *os.File) { | ||
| outputFlag, _ := cmd.Flags().GetString("output") |
There was a problem hiding this comment.
PrintError calls cmd.Flags().GetString("output") where cmd is the root tkn command (passed from cmd/tkn/main.go). However, the --output flag is defined on individual subcommands, not on the root. GetString("output") on the root will return "" (and a non-nil error which is silently discarded), so the JSON error path will never trigger.
Consider resolving the executed subcommand's flags instead, or propagating the output format via a persistent pre-run hook that stores it somewhere accessible at error-handling time.
| return &Error{Code: NotFound, Message: err.Error()} | ||
| case k8serrors.IsUnauthorized(err), k8serrors.IsForbidden(err): | ||
| return &Error{Code: Unauthorized, Message: err.Error()} | ||
| case k8serrors.IsTimeout(err): |
There was a problem hiding this comment.
k8serrors.IsTimeout() only matches HTTP 504 from the API server. Context deadline exceeded errors (context.DeadlineExceeded) and client-side request timeouts won't produce exit code 4.
Can we add this before the default case
case errors.Is(err, context.DeadlineExceeded):
return &Error{Code: Timeout, Message: err.Error(), Err: err}| // PipelineRun has no conditions yet; treat as a general failure | ||
| // so the caller can distinguish from a successful run (0) without | ||
| // conflicting with the "resource not found" exit code (2). | ||
| return 1 |
There was a problem hiding this comment.
Changing the "no conditions yet" exit code from 2 to 1 is semantically correct under the new scheme, but it's a breaking change for scripts relying on the previous behavior. The release note should explicitly mention this: "The exit code for PipelineRuns with no conditions has changed from 2 to 1."
|
|
||
| | Code | Constant | Meaning | | ||
| |------|----------------|--------------------------------------------| | ||
| | `0` | `Success` | The command completed successfully. | |
There was a problem hiding this comment.
In Bash, exit code 2 conventionally means "incorrect usage / invalid arguments." This PR uses 2 for "not found" instead. This isn't necessarily wrong (kubectl doesn't differentiate at all, and many CLIs define their own schemes), but it's worth documenting this divergence explicitly in docs/exit-codes.md so users don't confuse it with the shell convention.
There was a problem hiding this comment.
Pull request overview
Implements a shared exit-code scheme for tkn by introducing a typed error that carries an intended exit code, classifying certain Kubernetes API failures at the pkg/actions boundary, and wiring the top-level CLI entrypoint to exit using the resolved code (with documentation for consumers).
Changes:
- Added
pkg/exitcodewith standard codes (0–5), a typedexitcode.Error, and Kubernetes API error classification helpers. - Updated the top-level command execution path to silence Cobra’s default error printing and to drive
os.Exit(...)fromexitcode.CodeFrom(err), plus structured JSON error output support. - Updated selected actions helpers to wrap API errors via
exitcode.FromAPIError(...)and alignedpipelinerun logs --exit-with-errorbehavior with the new scheme.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/exitcode/exitcode.go | Defines exit codes, typed error, and K8s API error classification. |
| pkg/exitcode/exitcode_test.go | Unit tests for CodeFrom and FromAPIError. |
| pkg/cmd/root.go | Silences Cobra errors and adds PrintError to emit JSON error payloads for --output json. |
| cmd/tkn/main.go | Switches to centralized error printing + exit code resolution. |
| pkg/actions/get.go | Wraps dynamic client Get errors via exitcode.FromAPIError. |
| pkg/actions/list.go | Wraps dynamic client List errors via exitcode.FromAPIError. |
| pkg/actions/delete.go | Wraps dynamic client Delete errors via exitcode.FromAPIError. |
| pkg/actions/patch.go | Wraps dynamic client Patch errors via exitcode.FromAPIError. |
| pkg/cmd/pipelinerun/logs.go | Adjusts “no conditions” exit code to avoid conflicting with NotFound. |
| pkg/cmd/pipelinerun/logs_test.go | Updates the corresponding unit test expectation. |
| docs/exit-codes.md | Documents the exit code table, examples, and structured JSON stderr errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| unstructuredObj, err := clients.Dynamic.Resource(*gvr).Namespace(ns).Patch(context.Background(), objName, types.JSONPatchType, data, opt) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to patch object from %s namespace \n", ns) | ||
| return err | ||
| return exitcode.FromAPIError(err) |
| switch { | ||
| case k8serrors.IsNotFound(err): | ||
| return &Error{Code: NotFound, Message: err.Error()} | ||
| case k8serrors.IsUnauthorized(err), k8serrors.IsForbidden(err): | ||
| return &Error{Code: Unauthorized, Message: err.Error()} | ||
| case k8serrors.IsTimeout(err): | ||
| return &Error{Code: Timeout, Message: err.Error()} | ||
| default: | ||
| return err | ||
| } |
| allRes, err := dynamic.Resource(*gvr).Namespace(ns).List(context.Background(), op) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, exitcode.FromAPIError(err) | ||
| } |
| // PrintError writes err to errW. When the resolved --output flag on cmd is | ||
| // "json", the error is serialised as {"error":"<message>","code":<n>}. | ||
| // Otherwise the standard "Error: <message>\n" format is used. | ||
| func PrintError(cmd *cobra.Command, err error, errW *os.File) { | ||
| outputFlag, _ := cmd.Flags().GetString("output") |
|
One more minor comment: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
cmd/tkn/main.go:53
PrintErroris given the root command, but--outputis a local flag on the executed leaf command (PrintFlags.AddFlagsusescmd.Flags()). The lookup therefore fails and is ignored, so eventkn pipelinerun describe missing --output jsonemits the plain-text error instead of JSON. UseExecuteCand pass its returned command; an integration test with a leaf--output jsonflag would catch this.
if err := tkn.Execute(); err != nil {
tknCmd.PrintError(tkn, err, os.Stderr)
pkg/actions/list.go:61
- The API-boundary audit is incomplete.
pkg/actions/create.go:34andpkg/actions/watch.go:35still return raw Kubernetes API errors, so commands usingactions.Createoractions.Watchreturn code 1 for forbidden or timeout responses instead of codes 5 or 4. Apply the same classification to every API operation in this package.
allRes, err := dynamic.Resource(*gvr).Namespace(ns).List(context.Background(), op)
if err != nil {
return nil, exitcode.FromAPIError(err)
| case k8serrors.IsTimeout(err): | ||
| return &Error{Code: Timeout, Message: err.Error()} |
| // InvalidInput is the exit code for invalid flags, parameters, or input. | ||
| InvalidInput = 3 |
| // FromAPIError converts a Kubernetes API error into an Error with the | ||
| // appropriate exit code. If err is not a k8s status error it is returned | ||
| // unchanged. |
|
@vikashkumar2020 is there any update on this? |
1 similar comment
|
@vikashkumar2020 is there any update on this? |
Hi, due to some complications around the contribution to open source in my org at IBM. I have to hold on making any further contribution |
Thanks, @vikashkumar2020 for informing us. I hope you come back to contribute to this project. Thank you for your contributions. 🙂 |
Introduce a pkg/exitcode package that defines the standard tkn exit
codes (0 success, 1 general error, 2 not found, 3 invalid input,
4 timeout, 5 unauthorized) and a typed error so that errors can carry
their intended exit code through the call stack.
Classify Kubernetes API errors at the boundary in the shared actions
layer (get, list, delete, patch) via exitcode.FromAPIError, and emit
the resolved code from main using exitcode.CodeFrom. When --output
json is set, errors are written to stderr as a structured
{"error":"...","code":N} payload. Document the full scheme in
docs/exit-codes.md.
Closes tektoncd#2855
Signed-off-by: Vikash Kumar <kvvik2020@gmail.com>
034007f to
b94d1b2
Compare
|
/retest |
Two fixes on top of the exit-code support:
- FromAPIError now keeps the original Kubernetes error as the wrapped
error and exposes it via Unwrap, so k8serrors helpers such as
IsNotFound/IsForbidden still match after classification. Replacing
the typed error broke callers like customrun delete that branch on
IsNotFound.
- main passes the command returned by ExecuteC (the one that actually
ran) to PrintError, and PrintError takes an io.Writer. --output is a
subcommand-local flag, so reading it from the root command meant the
structured {"error":...,"code":N} JSON output never triggered.
Signed-off-by: divyansh42 <diagrawa@redhat.com>
Assisted-by: Claude Opus 4.8 (via GitHub Copilot CLI)
|
/retest |
b94d1b2 to
c095e71
Compare
|
/retest |
|
@copilot review |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved error classification and propagation gaps produce incorrect exit codes, while direct stderr diagnostics and inconsistent output-mode handling break the JSON error contract.
14 open issues
Preserve typed errors through description, list, cancel, and delete paths · New Classify Cobra parsing and validation failures as InvalidInput · New Normalize output format before selecting structured error rendering · New Resolve output mode even when flag parsing fails · New Map client and server timeouts to the Timeout exit code · New Map Kubernetes Invalid and BadRequest errors to InvalidInput · New Classify untyped Kubernetes API errors before the GeneralError fallback · New Classifying at this boundary is not sufficient to make the code reachmain: several existing… Exit code 3 is currently unreachable in production: no command or validator creates an…k8serrors.IsTimeoutdoes not match KubernetesStatusReasonServerTimeout; that case has the… The PR description states that all Kubernetes API calls inpkg/actionsclassify errors via…FromAPIErrordoesn’t currently map Kubernetes validation/client errors (e.g. HTTP 400/422) to… This function prints a plain-text message directly to stderr before returning the error. With the… New behavior inPrintError(structured JSON to stderr when--output json) isn’t covered by unit…
1 resolved since last review
Review effort: Max
| executedCmd, err := tkn.ExecuteC() | ||
| if err != nil { | ||
| tknCmd.PrintError(executedCmd, err, os.Stderr) | ||
| os.Exit(exitcode.CodeFrom(err)) |
| Short: "CLI for tekton pipelines", | ||
| Long: ``, | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, |
| // root command, so that the locally defined --output flag resolves correctly. | ||
| func PrintError(cmd *cobra.Command, err error, errW io.Writer) { | ||
| outputFlag, _ := cmd.Flags().GetString("output") | ||
| if outputFlag == "json" { |
| outputFlag, _ := cmd.Flags().GetString("output") | ||
| if outputFlag == "json" { |
| // FromAPIError converts a Kubernetes API error into an Error with the | ||
| // appropriate exit code. If err is not a k8s status error it is returned | ||
| // unchanged. The original error is kept as the wrapped error so that | ||
| // k8serrors helpers (IsNotFound, IsForbidden, ...) still match it. | ||
| func FromAPIError(err error) error { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| switch { | ||
| case k8serrors.IsNotFound(err): | ||
| return &Error{Code: NotFound, Message: err.Error(), Err: err} | ||
| case k8serrors.IsUnauthorized(err), k8serrors.IsForbidden(err): | ||
| return &Error{Code: Unauthorized, Message: err.Error(), Err: err} | ||
| case k8serrors.IsTimeout(err): |
| return &Error{Code: Unauthorized, Message: err.Error(), Err: err} | ||
| case k8serrors.IsTimeout(err): | ||
| return &Error{Code: Timeout, Message: err.Error(), Err: err} | ||
| default: |
| // CodeFrom returns the exit code carried by err, or GeneralError if err | ||
| // carries no code. | ||
| func CodeFrom(err error) int { | ||
| if err == nil { | ||
| return Success | ||
| } | ||
| var e *Error | ||
| if errors.As(err, &e) { | ||
| return e.Code | ||
| } | ||
| return GeneralError |


Changes
Fixes : #2855
Implemented consistent exit codes across tkn. Added a new pkg/exitcode package defining codes 0–5 (success, general error, not found, invalid input, timeout, unauthorized) with a typed error that propagates through the call stack. All Kubernetes API calls in pkg/actions now classify errors at the boundary via exitcode.FromAPIError(). The root command wires up exitcode.CodeFrom() to drive os.Exit, and adds structured {"error":"…","code":N} JSON output to stderr when --output json is set. A reference doc at docs/exit-codes.md covers the full scheme with examples.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make checkmake generatedSee the contribution guide
for more details.
Release Notes