Add support for finally task results in pipeline results

Prior to this commit, finally tasks can emit Results but
results emitted from the finally tasks can not be configured in
the Pipeline Results.

In this commit, we add implementation for supporting finally tasks
in Pipeline results, update documentation to reflect these changes,
and add examples for the users to better understand the new
feature being added.

References [TEP-0116](tektoncd/community#746)

/kind feature
/cc @jerop

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.