fix(SRVKP-14519): report snippet on new taskrun failures - #2986
Conversation
Paco Review 🔍This PR refactors the task failure reason handling in CollectFailedTasksLogSnippet to use explicit lookup maps (reasonsWithoutPodLogs and reasonsWithPodLogs) instead of a long inline condition chain. It adds coverage for several new Tekton failure reasons (StepFailed, StepOOM, SidecarFailed, PodEvicted, InitContainerFailed) and introduces a warning log for unknown failure reasons. Test coverage is significantly expanded, and the E2E golden file is normalized to handle variable Tekton failure reason strings across versions. Review difficulty: 3/5 (Moderate) — Moderate complexity refactor touching failure-path logic with meaningful behavioral changes; the new unknown-reason warning path and the change to how task conditions are checked add non-trivial correctness surface. 1 new inline comment(s) found. Reviewed commit: 6472694 |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Add the existing validation failure reason to message-only handling so it is not dropped.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates TaskRun failure handling so snippets and annotations work with granular Tekton failure reasons.
Changes:
- Categorizes log-based and message-only failure reasons.
- Adds unit and Gitea test coverage.
- Warns on unknown failure reasons.
File summaries
| File | Description |
|---|---|
test/testdata/TestGiteaErrorSnippetCustomLines.golden |
Stabilizes reason-dependent golden output. |
test/gitea_error_snippets_test.go |
Verifies rendered failure snippets. |
pkg/kubeinteraction/status/task_status.go |
Classifies failure reasons and collects snippets. |
pkg/kubeinteraction/status/task_status_test.go |
Tests granular failure handling. |
Review details
Suppressed comments (2)
pkg/kubeinteraction/status/task_status.go:43
SidecarFailedandSidecarOOMare added to the log-fetching set, but the fetch loop below only iteratestask.Status.Steps; Tekton reports sidecar termination intask.Status.Sidecars(TaskRunStatusFields.Sidecars/SidecarState). Consequently these newly supported reasons are returned with only the condition message and never receive the sidecar log snippet promised by this change. Include terminated sidecars when collecting logs (and add a regression test withTaskRunStatusFields.Sidecars).
tektonv1.TaskRunReasonSidecarFailed.String(): {},
tektonv1.TaskRunReasonSidecarOOM.String(): {},
pkg/kubeinteraction/status/task_status.go:30
- Several failure reasons already defined by the bundled Tekton API are still classified as unknown and therefore dropped:
TaskRunResolutionFailed,TaskValidationFailed,InvalidParamValue, andResourceVerificationFailed. These represent resolution/validation/configuration failures that do not produce user step logs, so their condition messages should be included inreasonsWithoutPodLogs; otherwise snippets and provider annotations remain missing for those failures and an avoidable warning is emitted.
tektonv1.TaskRunReasonFailedValidation.String(): {},
tektonv1.TaskRunReasonCancelled.String(): {},
tektonv1.TaskRunReasonTimedOut.String(): {},
tektonv1.TaskRunReasonImagePullFailed.String(): {},
tektonv1.TaskRunReasonCreateContainerConfigError.String(): {},
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
=======================================
Coverage 91.56% 91.57%
=======================================
Files 164 164
Lines 12530 12532 +2
=======================================
+ Hits 11473 11476 +3
+ Misses 1056 1055 -1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Tekton now describes a failed task with more precise reasons than the old generic "Failed", a step that exits with an error is reported as "StepFailed" and there are new reasons for out of memory kills, sidecar failures, evicted pods and failed init containers. Pipelines-as-Code only recognised the old name, so every step failure was quietly ignored: no failure snippet at the bottom of the comment or check run, and no inline error annotations on the changed files in GitHub. The failure reasons are now listed explicitly, split between the ones where the container logs can be fetched and the ones that fail before anything runs and only have a message to show, which also covers the validation, resolution and parameter errors that were only partially handled before. Failures that the pipeline author chose to ignore are still left out. An unknown reason is now logged as a warning instead of being dropped silently, so the next addition on the Tekton side is easy to spot. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
6472694 to
81eb706
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Sidecar failure reasons do not fetch logs from TaskRun sidecar states.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| tektonv1.TaskRunReasonSidecarFailed.String(): {}, | ||
| tektonv1.TaskRunReasonSidecarOOM.String(): {}, |
📝 Description of the Change
Failure snippets stopped showing up on failed PipelineRuns, and the GitHub
inline error annotations disappeared with them.
Tekton now reports granular TaskRun failure reasons instead of the generic
Failed: a step exiting non-zero is reported asStepFailed, and there arealso
StepOOM,SidecarFailed,SidecarOOM,InitContainerFailed,InitContainerOOM,PodEvictedandFailureIgnored.CollectFailedTasksLogSnippetonly accepted the literal reasonFailed:so every step failure was filtered out, the map came back empty,
getFailureSnippetreturned an empty string and the{{- if not (eq .Mt.FailureSnippet "")}}block of the status template wasnever rendered. The same function feeds
getFailuresMessageAsAnnotationsinthe GitHub provider, hence the missing annotations.
The reasons are now listed explicitly in two sets:
Failed,StepFailed,StepOOM,SidecarFailed,SidecarOOMandPodEvicted;never produce step logs: validation, cancelled, timeout, image pull,
container config and pod creation failures, plus
InitContainerFailedandInitContainerOOM.FailureIgnoredis still skipped, so a successful PipelineRun does not grow afailure snippet. Any unknown failure reason is now logged as a warning instead
of being dropped silently, so the next Tekton addition is visible.
Verified on a live cluster: the failed TaskRun reported
Succeeded=False reason=StepFailedwith message"step-lint" exited with code 2: Errorand no snippet was posted.🔗 Linked GitHub Issue
Fixes #
Jira: https://issues.redhat.com/browse/SRVKP-14519
🧪 Testing Strategy
TestCollectFailedTasksLogSnippetgained cases forStepFailed,StepOOM,SidecarFailed,PodEvicted,InitContainerFailed(message fallback),FailureIgnored(skipped) and an unknown reason (skipped plus warning).TestGiteaErrorSnippetnow asserts theFailure snippet:section is present,not only the log line.
TestGiteaErrorSnippetCustomLinesnormalizes thereason in the comment before the golden comparison, since the reason string
depends on the Tekton version.
🤖 AI Assistance
Investigation, fix and tests were done with AI assistance; the root cause was
confirmed against a running cluster and the change was reviewed and tested
locally with
make testandmake lint.✅ Submitter Checklist
make testandmake lintlocally