TEP-0090: Refactor GetChildReferences
#5006
Conversation
Prior to this change, `GetChildReferences` treated `TaskRuns` and `Runs` differently. It added a`ChildReference` only when a `TaskRun` was not nil, but did not do the check for `Runs`. In this change, we consistently add `ChildReference` only when the `Run` or `TaskRun` is not nil.
tekton-robot
added
release-note-none
tekton-robot
added
the
size/M
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/lgtm Well, that's some simplification I should have seen in the first place. =) |
|
/remove-lgtm |
| } else if rprt.TaskRun != nil { | ||
| childRefs = append(childRefs, rprt.getChildRefForTaskRun(taskRunVersion)) | ||
| switch { | ||
| case rprt.Run != nil: |
There was a problem hiding this comment.
So I didn't think about this on first glance, but now I'm remembering that, in fact, we should be adding a ChildReference for a PipelineTask with nil Run and nil TaskRun if that PipelineTask has WhenExpressions - that is, if the WhenExpressions result in the corresponding Run or TaskRun never getting created, the PipelineTask, with the WhenExpressions, should still be in ChildReferences.
There was a problem hiding this comment.
@abayer we do not create a Run or TaskRun when a PipelineTask is skipped, and "nil" Run or TaskRun should not be added to the PipelineRun status - https://github.com/tektoncd/community/blob/main/teps/0007-conditions-beta.md#status-1
The WhenExpressions for skipped PipelineTasks are in the SkippedTasks section of the PipelineRun status - they should not be in ChildReferences
There was a problem hiding this comment.
Aaah, then we should probably remove the WhenExpressions from ChildStatusReference. =)
There was a problem hiding this comment.
If I remember correctly, the reason we added WhenExpressions to ChildStatusReference is for convenience for users to see the resolved WhenExpressions that evaluated to True and led to the execution of the PipelineTask - open to removing them if users don't find them to be useful (cc @pritidesai)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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 |
tekton-robot
added
the
approved
In tektoncd#5006, we refactored `GetChildReferences` such that it checks that `TaskRun` is non-nil before calling the helper function `getChildRefForTaskRun` that expects a non-nil `TaskRun`. In this change, we add a check that each `TaskRun` from matrixed `PipelineTask` is non-nil before calling `getChildRefForTaskRun`. Related PR: tektoncd#5008
In #5006, we refactored `GetChildReferences` such that it checks that `TaskRun` is non-nil before calling the helper function `getChildRefForTaskRun` that expects a non-nil `TaskRun`. In this change, we add a check that each `TaskRun` from matrixed `PipelineTask` is non-nil before calling `getChildRefForTaskRun`. Related PR: #5008
jerop
changed the title
GetChildReferencesGetChildReferences
Changes
Prior to this change,
GetChildReferencestreatedTaskRunsandRunsdifferently. It added aChildReferenceonly when aTaskRunwas not nil, but did not do the check forRuns.In this change, we consistently add
ChildReferenceonly when theRunorTaskRunis not nil./kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes