an aggregate status of tasks in finally #3817
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The TEP is merged now 🎉 in |
| @@ -412,6 +412,28 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string { | |||
| tStatus[PipelineTaskStatusPrefix+t.PipelineTask.Name+PipelineTaskStatusSuffix] = s | |||
| } | |||
| } | |||
| // initialize aggregate status of all dag tasks to None | |||
| aggregateStatus := PipelineTaskStateNone | |||
There was a problem hiding this comment.
Maybe we should skip adding this to the map completely if checkDAGTasksDone is false? I don't see a way to end up in a None state except where Pipelines has incorrectly decided to run finally before tasks is finished, which would be a bug, I think?
There was a problem hiding this comment.
yup, there is no way to end up in a None state from finally. This is added mainly for the unit testing. Any better alternative please? 🙏
There was a problem hiding this comment.
Ah, ok. The only other option I guess would be not unit testing this if we don't think it ever happens. But I don't feel strongly either way so I think this is good!
ghost
modified the milestones:
OK cool, moved this one to milestone 0.24, thanks for updating the PR with this context, very helpful! |
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests |
|
/lgtm |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
/lgtm |
Implementing TEP-0049, it is now possible to access aggregate execution status of all tasks using `$(tasks.status)`. This context variable is only available in a finally task.
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerop 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
|
/lgtm |
| | `Completed` | all `tasks` completed successfully including one or more skipped tasks | | ||
| | `None` | no aggregate execution status available (i.e. none of the above), one or more `tasks` could be pending/running/cancelled/timedout | | ||
|
|
||
| For an end-to-end example, see [`$(tasks.status)` usage in a `Pipeline`](../examples/v1beta1/pipelineruns/pipelinerun-task-execution-status.yaml). |
| @@ -24,7 +24,7 @@ For instructions on using variable substitutions see the relevant section of [th | |||
| | `context.pipelineRun.uid` | The uid of the `PipelineRun` that this `Pipeline` is running in. | | |||
| | `context.pipeline.name` | The name of this `Pipeline` . | | |||
| | `tasks.<pipelineTaskName>.status` | The execution status of the specified `pipelineTask`, only available in `finally` tasks. | | |||
|
|
|||
| | `tasks.status` | An aggregate status of all `tasks`, only available in `finally` tasks. | | |||
There was a problem hiding this comment.
NIT: might be worth saying that this is an aggregated status of all tasks not in finally - just for 100% clarity :)
There was a problem hiding this comment.
thanks @afrittoli have created a PR to fix this #3919
| if !LooksLikeContainsResultRefs(ps) { | ||
| for _, p := range ps { | ||
| // check if it contains context variable accessing execution status - $(tasks.taskname.status) | ||
| // or an aggregate status - $(tasks.status) | ||
| if containsExecutionStatusRef(p) { |
There was a problem hiding this comment.
Nice, the same function catches the new variable too :)
Implemented in tektoncd/pipeline#3817 And update table of contents
Implemented in tektoncd/pipeline#3817 And update table of contents
Changes
Implementing TEP-0049, it is now possible to access an aggregate execution status of all the tasks using
$(tasks.status). This context variable is only available in a finally task./kind feature
Partially Closes #1020
Closes #3806
Implements TEP #0049
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes