fix: preserve Failed status in determineStepCompletion#5941
Merged
EronWright merged 3 commits intomainfrom Mar 18, 2026
Merged
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5941 +/- ##
==========================================
+ Coverage 56.38% 56.39% +0.01%
==========================================
Files 457 457
Lines 38236 38245 +9
==========================================
+ Hits 21558 21567 +9
Misses 15401 15401
Partials 1277 1277 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jessesuen
approved these changes
Mar 17, 2026
When a step signals Failed via a TerminalError, the promotion phase and step status were overridden to Errored with a noisy "an unrecoverable error occurred: ..." wrapper. This commit fixes two related issues in determineStepCompletion: 1. Failed + IsTerminal: split into its own case so the Failed status is preserved and the message is the raw error text, without the boilerplate prefix reserved for technical failures. 2. Error threshold: preserve meta.Status (Failed or Errored) instead of unconditionally setting Errored. Tests are updated to assert the correct statuses and messages, and new cases cover the Failed-preservation paths. Signed-off-by: Eron Wright <eron.wright@akuity.io>
Avoid wrapping intentional failure messages (e.g. from the fail step) with "error running step": prefix only Errored (technical) results. Signed-off-by: Eron Wright <eron.wright@akuity.io>
For Failed status, use a shorter "step %q:" prefix instead of "error running step %q:" to identify which step produced the error without implying a technical failure. Signed-off-by: Eron Wright <eron.wright@akuity.io>
Contributor
Author
Updated test results (with
|
| Scenario | Phase | Message |
|---|---|---|
| Config error (before) | Errored |
an unrecoverable error occurred: error running step "bad-config": invalid git-push config: (root): Additional property bogus is not allowed |
| Config error (after) | Failed |
step "bad-config": invalid git-push config: (root): Additional property bogus is not allowed |
| Intentional fail (before) | Errored |
an unrecoverable error occurred: error running step "failer": failed with message: fail #1 |
| Intentional fail (after) | Failed |
step "failer": failed: fail #1 |
The third commit (db7d9f4) adds a step "<alias>": prefix to non-technical errors (notably configuration errors) so the originating step is identifiable in the top-level promotion message, without the heavier error running step phrasing reserved for technical errors.
f057334 to
db7d9f4
Compare
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.
Summary
determineStepCompletionhad two issues that caused a step'sFailedstatus to be silently overridden byErrored:Failed+IsTerminal— the singleIsTerminalcase applied"an unrecoverable error occurred: ..."and setErroredregardless of whether the step had signaledFailed. A step likefailthat intentionally signals a business-logic failure ended up withErroredphase and a noisy wrapper message.Error threshold — when a non-terminal error exhausted retries, the status was unconditionally forced to
Errored, discarding anyFailedstatus the step had set on its last attempt.Changes
IsTerminalswitch case into two explicit cases:Failed && IsTerminal→ preserveFailed, useerr.Error()directly (no boilerplate prefix)IsTerminal(non-Failed) → setErroredexplicitly, keep"an unrecoverable error occurred: %s"prefix (technical failure deserves the annotation)meta.Status(FailedorErrored) instead of forcingErroredFailed-preservation pathsExample (UI)
Before:
After:
Example (API)
Before:
After:
Test plan
go test ./pkg/promotion/ -run TestLocalOrchestrator -v— all cases pass, including the two new onesfailstep → promotion phase isFailed(notErrored), message is the raw step message without"an unrecoverable error occurred:"prefix