TEP-0116: Referencing Finally Task Results in Pipeline Results

docs/pipelines.md

@@ -39,6 +39,7 @@ weight: 400
     - [Specifying `Parameters` in `finally` tasks](#specifying-parameters-in-finally-tasks)
     - [Specifying `matrix` in `finally` tasks](#specifying-matrix-in-finally-tasks)
     - [Consuming `Task` execution results in `finally`](#consuming-task-execution-results-in-finally)
+    - [Consuming `Pipeline` result with `finally`](#consuming-pipeline-result-with-finally)
     - [`PipelineRun` Status with `finally`](#pipelinerun-status-with-finally)
     - [Using Execution `Status` of `pipelineTask`](#using-execution-status-of-pipelinetask)
     - [Using Aggregate Execution `Status` of All `Tasks`](#using-aggregate-execution-status-of-all-tasks)
@@ -50,7 +51,6 @@ weight: 400
     - [Known Limitations](#known-limitations)
       - [Specifying `Resources` in `finally` tasks](#specifying-resources-in-finally-tasks)
       - [Cannot configure the `finally` task execution order](#cannot-configure-the-finally-task-execution-order)
-      - [Cannot configure `Pipeline` result with `finally`](#cannot-configure-pipeline-result-with-finally)
   - [Using Custom Tasks](#using-custom-tasks)
     - [Specifying the target Custom Task](#specifying-the-target-custom-task)
     - [Specifying a Custom Task Spec in-line (or embedded)](#specifying-a-custom-task-spec-in-line-or-embedded)
@@ -1268,6 +1268,24 @@ uninitialized task result `commit`, the `finally` Task `discover-git-commit` wil
 `skippedTasks` and continues executing rest of the `finally` tasks. The pipeline exits with `completion` instead of
 `success` if a `finally` task is added to the list of `skippedTasks`.
 
+### Consuming `Pipeline` result with `finally`
+
+`finally` tasks can emit `Results` and these results emitted from the `finally` tasks can be configured in the
+[Pipeline Results](#emitting-results-from-a-pipeline). References of `Results` from `finally` will follow the same naming conventions as referencing `Results` from `tasks`: ```$(finally.<finally-pipelinetask-name>.result.<result-name>)```.
+
+```yaml
+results:
+  - name: comment-count-validate
+    value: $(finally.check-count.results.comment-count-validate)
+finally:
+  - name: check-count
+    taskRef: 
+      name: example-task-name
+```
+
+In this example, `pipelineResults` in `status` will show the name-value pair for the result `comment-count-validate` which is produced in the `Task` `example-task-name`.
+
+
 ### `PipelineRun` Status with `finally`
 
 With `finally`, `PipelineRun` status is calculated based on `PipelineTasks` under `tasks` section and `finally` tasks.
@@ -1544,21 +1562,6 @@ It's not possible to configure or modify the execution order of the `finally` ta
 all `finally` tasks run simultaneously and start executing once all `PipelineTasks` under `tasks` have settled which means
 no `runAfter` can be specified in `finally` tasks.
 
-#### Cannot configure `Pipeline` result with `finally`
-
-`finally` tasks can emit `Results` but results emitted from the `finally` tasks can not be configured in the
-[Pipeline Results](#emitting-results-from-a-pipeline). We are working on adding support for this
-(tracked in issue [#2710](https://github.com/tektoncd/pipeline/issues/2710)).
-
-```yaml
-results:
-  - name: comment-count-validate
-    value: $(finally.check-count.results.comment-count-validate)
-```
-
-In this example, `pipelineResults` in `status` will exclude the name-value pair for that result `comment-count-validate`.
-
-
 ## Using Custom Tasks
 
 **Note: This is only allowed if `enable-custom-tasks` is set to

examples/v1beta1/pipelineruns/pipelinerun-with-final-results.yaml

@@ -0,0 +1,77 @@
+apiVersion: tekton.dev/v1beta1
+kind: Pipeline
+metadata:
+  name: sum-and-multiply-pipeline
+spec:
+  params:
+    - name: a
+      type: string
+      default: "1"
+    - name: b
+      type: string
+      default: "1"
+  results:
+    - name: task-result
+      description: "grabbing results from the tasks section"
+      value: $(tasks.multiply-inputs.results.product)
+    - name: finally-result
+      description: "grabbing results from the finally section"
+      value: $(finally.exponent.results.product)
+  tasks:
+    - name: multiply-inputs
+      taskRef:
+        name: multiply
+      params:
+        - name: a
+          value: "$(params.a)"
+        - name: b
+          value: "$(params.b)"
+  finally:
+    - name: exponent
+      taskRef:
+        name: multiply
+      params:
+        - name: a
+          value: "$(tasks.multiply-inputs.results.product)$(tasks.multiply-inputs.results.product)"
+        - name: b
+          value: "$(tasks.multiply-inputs.results.product)$(tasks.multiply-inputs.results.product)"
+---
+apiVersion: tekton.dev/v1beta1
+kind: Task
+metadata:
+  name: multiply
+  annotations:
+    description: |
+      A simple task that multiplies the two provided integers
+spec:
+  params:
+    - name: a
+      type: string
+      default: "1"
+      description: The first integer
+    - name: b
+      type: string
+      default: "1"
+      description: The second integer
+  results:
+    - name: product
+      description: The product of the two provided integers
+  steps:
+    - name: product
+      image: bash:latest
+      script: |
+        #!/usr/bin/env bash
+        echo -n $(( "$(params.a)" * "$(params.b)" )) | tee $(results.product.path)
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  generateName: sum-and-multiply-pipeline-run-
+spec:
+  pipelineRef:
+    name: sum-and-multiply-pipeline
+  params:
+    - name: a
+      value: "2"
+    - name: b
+      value: "1"

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -69,7 +69,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Tasks).ViaField("tasks"))
 	errs = errs.Also(validatePipelineWorkspacesUsage(ps.Workspaces, ps.Finally).ViaField("finally"))
 	// Validate the pipeline's results
-	errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks))
+	errs = errs.Also(validatePipelineResults(ps.Results, ps.Tasks, ps.Finally))
 	errs = errs.Also(validateTasksAndFinallySection(ps))
 	errs = errs.Also(validateFinalTasks(ps.Tasks, ps.Finally))
 	errs = errs.Also(validateWhenExpressions(ps.Tasks, ps.Finally))
@@ -255,8 +255,9 @@ func filter(arr []string, cond func(string) bool) []string {
 }
 
 // validatePipelineResults ensure that pipeline result variables are properly configured
-func validatePipelineResults(results []PipelineResult, tasks []PipelineTask) (errs *apis.FieldError) {
+func validatePipelineResults(results []PipelineResult, tasks []PipelineTask, finally []PipelineTask) (errs *apis.FieldError) {
 	pipelineTaskNames := getPipelineTasksNames(tasks)
+	pipelineFinallyTaskNames := getPipelineTasksNames(finally)
 	for idx, result := range results {
 		expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result)
 		if !ok {
@@ -276,7 +277,7 @@ func validatePipelineResults(results []PipelineResult, tasks []PipelineTask) (er
 				"value").ViaFieldIndex("results", idx))
 		}
 
-		if !taskContainsResult(result.Value.StringVal, pipelineTaskNames) {
+		if !taskContainsResult(result.Value.StringVal, pipelineTaskNames, pipelineFinallyTaskNames) {
 			errs = errs.Also(apis.ErrInvalidValue("referencing a nonexistent task",
 				"value").ViaFieldIndex("results", idx))
 		}
@@ -297,16 +298,26 @@ func getPipelineTasksNames(pipelineTasks []PipelineTask) sets.String {
 
 // taskContainsResult ensures the result value is referenced within the
 // task names
-func taskContainsResult(resultExpression string, pipelineTaskNames sets.String) bool {
+func taskContainsResult(resultExpression string, pipelineTaskNames sets.String, pipelineFinallyTaskNames sets.String) bool {
 	// split incase of multiple resultExpressions in the same result.Value string
 	// i.e "$(task.<task-name).result.<result-name>) - $(task2.<task2-name).result2.<result2-name>)"
 	split := strings.Split(resultExpression, "$")
 	for _, expression := range split {
 		if expression != "" {
-			pipelineTaskName, _, _, _, _ := parseExpression(stripVarSubExpression("$" + expression))
-			if !pipelineTaskNames.Has(pipelineTaskName) {
+			value := stripVarSubExpression("$" + expression)
+			pipelineTaskName, _, _, _, err := parseExpression(value)
+
+			if err != nil {
+				return false
+			}
+
+			if strings.HasPrefix(value, "tasks") && !pipelineTaskNames.Has(pipelineTaskName) {
 				return false
 			}
+			if strings.HasPrefix(value, "finally") && !pipelineFinallyTaskNames.Has(pipelineTaskName) {
+				return false
+			}
+
 		}
 	}
 	return true

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

@@ -1149,7 +1149,7 @@ func TestValidatePipelineResults_Success(t *testing.T) {
 		Description: "this is my pipeline result",
 		Value:       *NewArrayOrString("$(tasks.a-task.results.gitrepo.commit)"),
 	}}
-	if err := validatePipelineResults(results, []PipelineTask{{Name: "a-task"}}); err != nil {
+	if err := validatePipelineResults(results, []PipelineTask{{Name: "a-task"}}, []PipelineTask{}); err != nil {
 		t.Errorf("Pipeline.validatePipelineResults() returned error for valid pipeline: %s: %v", desc, err)
 	}
 }
@@ -1160,14 +1160,23 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
 		results       []PipelineResult
 		expectedError apis.FieldError
 	}{{
-		desc: "invalid pipeline result reference",
+		desc: "invalid pipeline task result reference",
 		results: []PipelineResult{{
 			Name:        "my-pipeline-result",
 			Description: "this is my pipeline result",
 			Value:       *NewArrayOrString("$(tasks.a-task.results.output.key1.extra)"),
 		}},
 		expectedError: *apis.ErrInvalidValue(`expected all of the expressions [tasks.a-task.results.output.key1.extra] to be result expressions but only [] were`, "results[0].value").Also(
 			apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")),
+	}, {
+		desc: "invalid pipeline finally result reference variable",
+		results: []PipelineResult{{
+			Name:        "my-pipeline-result",
+			Description: "this is my pipeline result",
+			Value:       *NewArrayOrString("$(finally.a-task.results.output.key1.extra)"),
+		}},
+		expectedError: *apis.ErrInvalidValue(`expected all of the expressions [finally.a-task.results.output.key1.extra] to be result expressions but only [] were`, "results[0].value").Also(
+			apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")),
 	}, {
 		desc: "invalid pipeline result value with static string",
 		results: []PipelineResult{{
@@ -1189,7 +1198,7 @@ func TestValidatePipelineResults_Failure(t *testing.T) {
 			apis.ErrInvalidValue("referencing a nonexistent task", "results[0].value")),
 	}}
 	for _, tt := range tests {
-		err := validatePipelineResults(tt.results, []PipelineTask{{Name: "a-task"}})
+		err := validatePipelineResults(tt.results, []PipelineTask{{Name: "a-task"}}, []PipelineTask{})
 		if err == nil {
 			t.Errorf("Pipeline.validatePipelineResults() did not return for invalid pipeline: %s", tt.desc)
 		}
@@ -1226,7 +1235,42 @@ func TestFinallyTaskResultsToPipelineResults_Success(t *testing.T) {
 					}},
 				}},
 			},
-		}},
+		}}, {
+		name: "referencing existent finally task result",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Results: []PipelineResult{{
+					Name:  "initialized",
+					Value: *NewArrayOrString("$(finally.check-git-commit.results.init)"),
+				}},
+				Tasks: []PipelineTask{{
+					Name: "clone-app-repo",
+					TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
+						Results: []TaskResult{{
+							Name: "current-date-unix-timestamp",
+							Type: "string",
+						}},
+						Steps: []Step{{
+							Name: "foo", Image: "bar",
+						}},
+					}},
+				}},
+				Finally: []PipelineTask{{
+					Name: "check-git-commit",
+					TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
+						Results: []TaskResult{{
+							Name: "init",
+							Type: "string",
+						}},
+						Steps: []Step{{
+							Name: "foo2", Image: "bar",
+						}},
+					}},
+				}},
+			},
+		},
+	},
 	}
 
 	for _, tt := range tests {
@@ -1288,6 +1332,45 @@ func TestFinallyTaskResultsToPipelineResults_Failure(t *testing.T) {
 			Message: `invalid value: referencing a nonexistent task`,
 			Paths:   []string{"spec.results[0].value"},
 		},
+	}, {
+		desc: "referencing nonexistent finally task result",
+		p: &Pipeline{
+			ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
+			Spec: PipelineSpec{
+				Results: []PipelineResult{{
+					Name:  "initialized",
+					Value: *NewArrayOrString("$(finally.nonexistent-task.results.init)"),
+				}},
+				Tasks: []PipelineTask{{
+					Name: "clone-app-repo",
+					TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
+						Results: []TaskResult{{
+							Name: "current-date-unix-timestamp",
+							Type: "string",
+						}},
+						Steps: []Step{{
+							Name: "foo", Image: "bar",
+						}},
+					}},
+				}},
+				Finally: []PipelineTask{{
+					Name: "check-git-commit",
+					TaskSpec: &EmbeddedTask{TaskSpec: TaskSpec{
+						Results: []TaskResult{{
+							Name: "init",
+							Type: "string",
+						}},
+						Steps: []Step{{
+							Name: "foo2", Image: "bar",
+						}},
+					}},
+				}},
+			},
+		},
+		expectedError: apis.FieldError{
+			Message: `invalid value: referencing a nonexistent task`,
+			Paths:   []string{"spec.results[0].value"},
+		},
 	}}
 
 	for _, tt := range tests {

pkg/apis/pipeline/v1beta1/resultref.go

@@ -39,6 +39,8 @@ const (
 	objectResultExpressionFormat = "tasks.<taskName>.results.<objectResultName>.<individualAttribute>"
 	// ResultTaskPart Constant used to define the "tasks" part of a pipeline result reference
 	ResultTaskPart = "tasks"
+	// ResultFinallyPart Constant used to define the "finally" part of a pipeline result reference
+	ResultFinallyPart = "finally"
 	// ResultResultPart Constant used to define the "results" part of a pipeline result reference
 	ResultResultPart = "results"
 	// TODO(#2462) use one regex across all substitutions
@@ -99,7 +101,7 @@ func LooksLikeContainsResultRefs(expressions []string) bool {
 // looksLikeResultRef attempts to check if the given string looks like it contains any
 // result references. Returns true if it does, false otherwise
 func looksLikeResultRef(expression string) bool {
-	return strings.HasPrefix(expression, "task") && strings.Contains(expression, ".result")
+	return (strings.HasPrefix(expression, "task") || strings.HasPrefix(expression, "finally")) && strings.Contains(expression, ".result")
 }
 
 // GetVarSubstitutionExpressionsForParam extracts all the value between "$(" and ")"" for a parameter
@@ -172,7 +174,7 @@ func parseExpression(substitutionExpression string) (string, string, int, string
 
 	// For string result: tasks.<taskName>.results.<stringResultName>
 	// For array result: tasks.<taskName>.results.<arrayResultName>[index]
-	if len(subExpressions) == 4 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart {
+	if len(subExpressions) == 4 && (subExpressions[0] == ResultTaskPart || subExpressions[0] == ResultFinallyPart) && subExpressions[2] == ResultResultPart {
 		resultName, stringIdx := ParseResultName(subExpressions[3])
 		if stringIdx != "" {
 			intIdx, _ := strconv.Atoi(stringIdx)
@@ -182,11 +184,11 @@ func parseExpression(substitutionExpression string) (string, string, int, string
 	}
 
 	// For object type result: tasks.<taskName>.results.<objectResultName>.<individualAttribute>
-	if len(subExpressions) == 5 && subExpressions[0] == ResultTaskPart && subExpressions[2] == ResultResultPart {
+	if len(subExpressions) == 5 && (subExpressions[0] == ResultTaskPart || subExpressions[0] == ResultFinallyPart) && subExpressions[2] == ResultResultPart {
 		return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil
 	}
 
-	return "", "", 0, "", fmt.Errorf("Must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
+	return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat)
 }
 
 // ParseResultName parse the input string to extract resultName and result index.