Switch ApplyTaskResultsToPipelineResults to not use status maps

This comes out of discussions on tektoncd#4739 - with the new minimal embedded status
changes which will be introduced in that PR, we can see that we're currently
using the output of `pipelineRunFacts.State.GetTaskRunsStatus(pr)` and
`pipelineRunFacts.State.GetRunsStatus(pr)` for two separate purposes:

* To set `pr.Status.TaskRuns` and `pr.Status.Runs` with the full embedded status
* To pass to `resources.ApplyTaskResultsToPipelineResults` for populating results

It's understandable why `ApplyTaskResultsToPipelineResults` is using
the maps from `pr.Status.[TaskRuns|Runs]`, since those maps do contain everything
needed for propagating results up from the tasks to the pipeline run, but if you
look at the current implementation, you can see that it's shuffling the maps
around into a different form that's more suited for what it's doing than the
original form.

So this PR reworks `ApplyTaskResultsToPipelineResults` to instead take maps in
the form the current implementation uses internally, with new functions on
`PipelineRunState` to get these new maps without needing to use the `pr.Status.[TaskRuns|Runs]`
form as an intermediary. This makes the pre-minimal-embedded-status
implementation cleaner, and is particularly helpful in that regard once we do
have minimal embedded status in place.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -577,7 +577,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 	pr.Status.Runs = pipelineRunFacts.State.GetRunsStatus(pr)
 	pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
 	if after.Status == corev1.ConditionTrue {
-		pr.Status.PipelineResults = resources.ApplyTaskResultsToPipelineResults(pipelineSpec.Results, pr.Status.TaskRuns, pr.Status.Runs)
+		pr.Status.PipelineResults = resources.ApplyTaskResultsToPipelineResults(pipelineSpec.Results,
+			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults())
 	}
 
 	logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after)

pkg/reconciler/pipelinerun/resources/apply.go

@@ -21,10 +21,10 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/tektoncd/pipeline/pkg/apis/run/v1alpha1"
+
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/substitution"
-	corev1 "k8s.io/api/core/v1"
-	"knative.dev/pkg/apis"
 )
 
 // ApplyParameters applies the params from a PipelineRun.Params to a PipelineSpec.
@@ -183,17 +183,8 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st
 // results are invalid.
 func ApplyTaskResultsToPipelineResults(
 	results []v1beta1.PipelineResult,
-	taskRunStatuses map[string]*v1beta1.PipelineRunTaskRunStatus,
-	runStatuses map[string]*v1beta1.PipelineRunRunStatus) []v1beta1.PipelineRunResult {
-
-	taskStatuses := map[string]*v1beta1.PipelineRunTaskRunStatus{}
-	for _, trStatus := range taskRunStatuses {
-		taskStatuses[trStatus.PipelineTaskName] = trStatus
-	}
-	customTaskStatuses := map[string]*v1beta1.PipelineRunRunStatus{}
-	for _, runStatus := range runStatuses {
-		customTaskStatuses[runStatus.PipelineTaskName] = runStatus
-	}
+	taskRunResults map[string][]v1beta1.TaskRunResult,
+	customTaskResults map[string][]v1alpha1.RunResult) []v1beta1.PipelineRunResult {
 
 	var runResults []v1beta1.PipelineRunResult
 	stringReplacements := map[string]string{}
@@ -207,9 +198,9 @@ func ApplyTaskResultsToPipelineResults(
 			variableParts := strings.Split(variable, ".")
 			if len(variableParts) == 4 && variableParts[0] == "tasks" && variableParts[2] == "results" {
 				taskName, resultName := variableParts[1], variableParts[3]
-				if resultValue := taskResultValue(taskName, resultName, taskStatuses); resultValue != nil {
+				if resultValue := taskResultValue(taskName, resultName, taskRunResults); resultValue != nil {
 					stringReplacements[variable] = *resultValue
-				} else if resultValue := runResultValue(taskName, resultName, customTaskStatuses); resultValue != nil {
+				} else if resultValue := runResultValue(taskName, resultName, customTaskResults); resultValue != nil {
 					stringReplacements[variable] = *resultValue
 				} else {
 					validPipelineResult = false
@@ -234,43 +225,23 @@ func ApplyTaskResultsToPipelineResults(
 	return runResults
 }
 
-// taskResultValue checks if a TaskRun result exists for a given pipeline task and result name.
-// A nil pointer is returned if the variable is invalid for any reason.
-func taskResultValue(taskName string, resultName string, taskStatuses map[string]*v1beta1.PipelineRunTaskRunStatus) *string {
-
-	status, taskExists := taskStatuses[taskName]
-	if !taskExists || status.Status == nil {
-		return nil
-	}
-
-	cond := status.Status.GetCondition(apis.ConditionSucceeded)
-	if cond == nil || cond.Status != corev1.ConditionTrue {
-		return nil
-	}
-
-	for _, trResult := range status.Status.TaskRunResults {
+// taskResultValue returns the result value for a given pipeline task name and result name in a map of TaskRunResults for
+// pipeline task names. It returns nil if either the pipeline task name isn't present in the map, or if there is no
+// result with the result name in the pipeline task name's slice of results.
+func taskResultValue(taskName string, resultName string, taskResults map[string][]v1beta1.TaskRunResult) *string {
+	for _, trResult := range taskResults[taskName] {
 		if trResult.Name == resultName {
 			return &trResult.Value
 		}
 	}
 	return nil
 }
 
-// runResultValue checks if a Run result exists for a given pipeline task and result name.
-// A nil pointer is returned if the variable is invalid for any reason.
-func runResultValue(taskName string, resultName string, runStatuses map[string]*v1beta1.PipelineRunRunStatus) *string {
-
-	status, runExists := runStatuses[taskName]
-	if !runExists || status.Status == nil {
-		return nil
-	}
-
-	cond := status.Status.GetCondition(apis.ConditionSucceeded)
-	if cond == nil || cond.Status != corev1.ConditionTrue {
-		return nil
-	}
-
-	for _, runResult := range status.Status.Results {
+// runResultValue returns the result value for a given pipeline task name and result name in a map of RunResults for
+// pipeline task names. It returns nil if either the pipeline task name isn't present in the map, or if there is no
+// result with the result name in the pipeline task name's slice of results.
+func runResultValue(taskName string, resultName string, runResults map[string][]v1alpha1.RunResult) *string {
+	for _, runResult := range runResults[taskName] {
 		if runResult.Name == resultName {
 			return &runResult.Value
 		}