Fix bug where step status ordering did not match step container ordering #3679
Conversation
Prior to this commit it was possible for Tekton Pipelines to produce an ordering of Step Statuses that did not correctly match the order of Step Containers. This confuses the reconciler when figuring out the Succeeded status - the TaskRun could end up with the incorrect Step recorded as the original failure. This also meant that downstream projects like the Dashboard became confused about how to render the TaskRun: they'd display the wrong order of steps and incorrectly show that steps as "Not Running" when in fact they did. This commit removes the custom `sort` code and just walks the list of step containers in order, setting the status for that container to the same position container.
|
The following is the coverage report on the affected files.
|
|
Running unit tests locally I don't see any errors. I'm going to try rerunning that. /test tekton-pipeline-unit-tests |
|
The build tests passed and the integration tests are "still running" according to their prow pages. There's something really funky going on with the github status reporting here. Re-running one-at-a-time to see what happens. /test pull-tekton-pipeline-build-tests |
|
/test pull-tekton-pipeline-integration-tests |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
wow a huge simplification, thanks @sbwsg 👍 Just by creating a map of container names and its status, then iterating over the desired order of step containers and changing the order of container statuses based on that order. This might leave us with a list of container statuses with duplicate containers and/or some missing if the for loop is interrupted which can only be possible when a process is killed. And should not be concern here. /lgtm |
Changes
Fixes #3677
Pipelines sorts container statuses so they're in the same order as steps. The sorting is not currently working right. See #3677 for an example of this where
image-digest-exporterwas sorted before some user steps. This confuses a lot of things - theSucceededstatus records the wrong step as original source of failure, the Dashboard shows the wrong step as the source of failure in a TaskRun, and also shows some Steps as "Not Run" even if they did.This commit replaces the custom sort code with a much simpler version: just walk the containers and place the statuses in the same order.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Release Notes