-
Couldn't load subscription status.
- Fork 1.8k
an aggregate status of tasks in finally #3817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks @afrittoli have created a PR to fix this #3919 |
||
|
|
||
| ## Variables available in a `Task` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,25 +203,30 @@ func validateExecutionStatusVariablesInTasks(tasks []PipelineTask) (errs *apis.F | |
| for _, param := range t.Params { | ||
| // retrieve a list of substitution expression from a param | ||
| if ps, ok := GetVarSubstitutionExpressionsForParam(param); ok { | ||
| // validate tasks.pipelineTask.status if this expression is not a result reference | ||
| // validate tasks.pipelineTask.status/tasks.status if this expression is not a result reference | ||
| if !LooksLikeContainsResultRefs(ps) { | ||
| for _, p := range ps { | ||
| // check if it contains context variable accessing execution status - $(tasks.taskname.status) | ||
pritidesai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // or an aggregate status - $(tasks.status) | ||
| if containsExecutionStatusRef(p) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, the same function catches the new variable too :) |
||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("pipeline tasks can not refer to execution status of any other pipeline task"), | ||
| "value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx)) | ||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("pipeline tasks can not refer to execution status of any other pipeline task"+ | ||
| " or aggregate status of tasks"), "value").ViaFieldKey("params", param.Name).ViaFieldIndex("tasks", idx)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| for i, we := range t.WhenExpressions { | ||
| // retrieve a list of substitution expression from a when expression | ||
| if expressions, ok := we.GetVarSubstitutionExpressions(); ok { | ||
| // validate tasks.pipelineTask.status/tasks.status if this expression is not a result reference | ||
| if !LooksLikeContainsResultRefs(expressions) { | ||
| for _, e := range expressions { | ||
| // check if it contains context variable accessing execution status - $(tasks.taskname.status) | ||
| // or an aggregate status - $(tasks.status) | ||
| if containsExecutionStatusRef(e) { | ||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("when expressions in pipeline tasks can not refer to execution status of any other pipeline task"), | ||
| "").ViaFieldIndex("when", i).ViaFieldIndex("tasks", idx)) | ||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("when expressions in pipeline tasks can not refer to execution status of any other pipeline task"+ | ||
| " or aggregate status of tasks"), "").ViaFieldIndex("when", i).ViaFieldIndex("tasks", idx)) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -257,6 +262,10 @@ func validateExecutionStatusVariablesExpressions(expressions []string, ptNames s | |
| // validate tasks.pipelineTask.status if this expression is not a result reference | ||
| if !LooksLikeContainsResultRefs(expressions) { | ||
| for _, expression := range expressions { | ||
| // its a reference to aggregate status of dag tasks - $(tasks.status) | ||
| if expression == PipelineTasksAggregateStatus { | ||
| continue | ||
| } | ||
| // check if it contains context variable accessing execution status - $(tasks.taskname.status) | ||
pritidesai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if containsExecutionStatusRef(expression) { | ||
| // strip tasks. and .status from tasks.taskname.status to further verify task name | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should skip adding this to the map completely if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup, there is no way to end up in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
| if facts.checkDAGTasksDone() { | ||
| // all dag tasks are done, change the aggregate status to succeeded | ||
| // will reset it to failed/skipped if needed | ||
| aggregateStatus = v1beta1.PipelineRunReasonSuccessful.String() | ||
| for _, t := range facts.State { | ||
| if facts.isDAGTask(t.PipelineTask.Name) { | ||
| // if any of the dag task failed, change the aggregate status to failed and return | ||
| if t.IsConditionStatusFalse() { | ||
| aggregateStatus = v1beta1.PipelineRunReasonFailed.String() | ||
| break | ||
| } | ||
| // if any of the dag task skipped, change the aggregate status to completed | ||
| // but continue checking for any other failure | ||
| if t.Skip(facts) { | ||
| aggregateStatus = v1beta1.PipelineRunReasonCompleted.String() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| tStatus[v1beta1.PipelineTasksAggregateStatus] = aggregateStatus | ||
| return tStatus | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍