Repository: tektoncd/pipeline (language: go)

Issue:
Pipeline failing with "invalid pipelineresults" when referencing results of skipped tasks
# Expected Behavior

According to the documentation pipeline results can contain references to task results. If a task did not run because of it's whenConditions, the pipeline result is not emitted but the pipeline succeeds. This worked with v0.37.5 of Tekton Pipelines available on OpenShift.

# Actual Behavior

With the update of the RedHat OpenShift Pipelines Operator, which brings Tekton Pipeline v0.41.0, pipelines with skipped tasks and composed results now fail with the message `invalid pipelineresults [result-goodbye], the referred results don't exist`.

# Steps to Reproduce the Problem

Run the following PipelineRun:

```yaml
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-test
spec:
  pipelineSpec:
    params:
      - name: say-hello
        default: 'false'
    tasks:
      - name: hello
        taskSpec:
          results:
            - name: result-one
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Hello world!"
              echo -n "RES1" > $(results.result-one.path)
  
      - name: goodbye
        runAfter:
          - hello
        taskSpec:
          results:
            - name: result-two
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Goodbye world!"
              echo -n "RES2" > $(results.result-two.path)
        when:
          - input: $(params.say-hello)
            operator: in
            values: ["true"]

    results:
      - name: result-hello
        description: Result one
        value: '$(tasks.hello.results.result-one)'
      - name: result-goodbye
        description: Result two
        value: '$(tasks.goodbye.results.result-two)'
```

# Additional Info

- Kubernetes version:

```
Server Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.4+a34b9e9", GitCommit:"b6d1f054747e9886f61dd85316deac3415e2726f", GitTreeState:"clean", BuildDate:"2023-01-10T15:55:28Z", GoVersion:"go1.19.4", Compiler:"gc", Platform:"linux/amd64"}
```

- Tekton Pipeline version:

```
Pipeline version: v0.41.0
Triggers version: v0.22.0
Openshift Pipelines Version: 1.9.0
```



Discussion / hints:
Success on v0.37.5:
![pipeline-success-v0 37 5](https://user-images.githubusercontent.com/1577511/217885012-846347f5-a1b1-4c22-a58f-e7ab62551d71.png)

Failure on v0.41.0:
![pipeline-failure-v0 41 0](https://user-images.githubusercontent.com/1577511/217885006-197d9eec-4758-4418-9053-47665cd4c5b0.png)
Hi @thomascube, thanks!
I think the validation is added intentionally,  do you think it makes more sense to remove the check?
Since I may lack the knowledge of the use cases, I'd like to learn more about your opinons.
@Yongxuanzhang We would like to display these values in our UI tool (if they are available). Is there any way to default to empty, if the task was not run?
e.g.  something like `'$(tasks.hello.results.result-one || '')'` or `'$(tasks.hello && tasks.hello.results.result-one)'`?
@kyubisation we don't have default result yet but it's something that @vinamra28 has been looking into -- considering a syntax similar to the first one you suggested: `'$(tasks.hello.results.result-one || '')'`
> @Yongxuanzhang We would like to display these values in our UI tool (if they are available). Is there any way to default to empty, if the task was not run? e.g. something like `'$(tasks.hello.results.result-one || '')'` or `'$(tasks.hello && tasks.hello.results.result-one)'`?

I think default results would be a solution. But it may take much longer time?

I wonder before this update, if the results are not available, there will be nothing showing in UI?
The reason I add the check is to help debugging. So you would know which results are not emitted. 
I'm thinking if there is a better way to meet both needs? 
e.g. We don't fail the pipelinerun but only log the warning message?

Or actually we don't want to have any check/validation on this? If so I think we could just remove the validation.

@kyubisation @jerop any suggestions? 
@Yongxuanzhang I think the old behavior which is also [described in the documentation](https://tekton.dev/docs/pipelines/pipelines/#emitting-results-from-a-pipeline) under "A Pipeline Result is not emitted if any of the following are true" makes perfectly sense and we were running fine with it. With the new check and the failure of the pipeline it's impossible to use task results in conjunction with when conditions.
@Yongxuanzhang Having validation is reasonable, but directly breaking the pipeline without a workaround/migration possibility is not ideal. I'd prefer to have this either be opt-in or have to have a fallback/default available at the same time.
> @Yongxuanzhang I think the old behavior which is also [described in the documentation](https://tekton.dev/docs/pipelines/pipelines/#emitting-results-from-a-pipeline) under "A Pipeline Result is not emitted if any of the following are true" makes perfectly sense and we were running fine with it. With the new check and the failure of the pipeline it's impossible to use task results in conjunction with when conditions.

In the doc you linked it says:

>A PipelineTask referenced by the Pipeline Result didn’t emit the referenced Task Result. This should be considered a bug in the Task and [may fail a PipelineTask in future](https://github.com/tektoncd/pipeline/issues/3497).

It looks like we want to fail the run if the results are not emitted but referenced? 
You case is like the results may or may not emit and it depends on the task's when condition. 

I'm ok with @kyubisation's suggestion. 


@Yongxuanzhang apparently I read the following statement differently

> This should be considered a bug in the Task and [may fail a PipelineTask in the future](https://github.com/tektoncd/pipeline/issues/3497).

For me this sounds like a case where a task specifies a result but no write to that result file was made in any of the steps. I didn't make the connection to skipped tasks.

Anyway, you asked about my use-case: we have pipelines for building software projects such as a Java Maven application. The pipelines includes tasks for build the jar, testing it, running quality checks (sonar scan) and creating a Docker image from it. Some tasks are optional (e.g. sonar scan or docker build) and can be enabled/disabled by pipeline parameters consumed by when expressions guarding the task execution. These tasks also emit results (if executed) which are mapped into pipeline results. If for example a docker image was built, the image name and tag are visible in the pipeline results and vice versa: if the docker build was disabled by parameter, no such pipeline result exists.

Or a bit shorter: _we run pipelines with optional tasks that yield optional results._

I was just surprised to suddenly see pipeline fail although all tasks have executed successfully. For possible solutions I can just copy @kyubisation's suggestions.



Thanks for your input and suggestions! @thomascube @kyubisation  
I have a better understanding of the use cases. I will fix it soon!
Will keep you updated. :D
/assign
I believe https://github.com/tektoncd/community/pull/954 is a long term solution. But we should fix this to unblock the use cases

Fix patch:
diff --git a/docs/pipelines.md b/docs/pipelines.md
index a2f3b89ab5d..2640d18cc09 100644
--- a/docs/pipelines.md
+++ b/docs/pipelines.md
@@ -336,7 +336,7 @@ tasks:
       value: https://github.com/tektoncd/catalog.git
     - name: revision
       # value can use params declared at the pipeline level or a static value like main
-      value: $(params.gitRevision) 
+      value: $(params.gitRevision)
     - name: pathInRepo
       value: task/golang-build/0.3/golang-build.yaml
 ```
@@ -1079,6 +1079,7 @@ This will cause an `InvalidTaskResultReference` validation error during `Pipelin
 
 **Note:** Since a `Pipeline Result` can contain references to multiple `Task Results`, if any of those
 `Task Result` references are invalid the entire `Pipeline Result` is not emitted.
+**Note:** If a `PipelineTask` referenced by the `Pipeline Result` was skipped, the `Pipeline Result` will not be emitted and the `PipelineRun` will not fail due to a missing result.
 
 ## Configuring the `Task` execution order
 
@@ -1322,7 +1323,7 @@ results:
     value: $(finally.check-count.results.comment-count-validate)
 finally:
   - name: check-count
-    taskRef: 
+    taskRef:
       name: example-task-name
 ```
 
@@ -1810,7 +1811,7 @@ Consult the documentation of the custom task that you are using to determine whe
 Pipelines do not support the following items with custom tasks:
 * Pipeline Resources
 
-### Known Custom Tasks 
+### Known Custom Tasks
 
 We try to list as many known Custom Tasks as possible here so that users can easily find what they want. Please feel free to share the Custom Task you implemented in this table.
 
diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go
index a84ca645fc1..12386b72c32 100644
--- a/pkg/reconciler/pipelinerun/pipelinerun.go
+++ b/pkg/reconciler/pipelinerun/pipelinerun.go
@@ -722,8 +722,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 
 	pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
 	if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
-		pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(pipelineSpec.Results,
-			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults())
+		pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
+			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks)
 		if err != nil {
 			pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
 			return err
diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go
index 470485a30fc..c6efa692e64 100644
--- a/pkg/reconciler/pipelinerun/resources/apply.go
+++ b/pkg/reconciler/pipelinerun/resources/apply.go
@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
+
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
 	"github.com/tektoncd/pipeline/pkg/substitution"
@@ -313,11 +314,18 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st
 // and omitted from the returned slice. A nil slice is returned if no results are passed in or all
 // results are invalid.
 func ApplyTaskResultsToPipelineResults(
+	ctx context.Context,
 	results []v1beta1.PipelineResult,
 	taskRunResults map[string][]v1beta1.TaskRunResult,
-	customTaskResults map[string][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) {
+	customTaskResults map[string][]v1beta1.CustomRunResult,
+	skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) {
 	var runResults []v1beta1.PipelineRunResult
 	var invalidPipelineResults []string
+	skippedTaskNames := map[string]bool{}
+	for _, t := range skippedTasks {
+		skippedTaskNames[t.Name] = true
+	}
+
 	stringReplacements := map[string]string{}
 	arrayReplacements := map[string][]string{}
 	objectReplacements := map[string]map[string]string{}
@@ -338,6 +346,11 @@ func ApplyTaskResultsToPipelineResults(
 				continue
 			}
 			variableParts := strings.Split(variable, ".")
+			// if the referenced task is skipped, we should also skip the results replacements
+			if _, ok := skippedTaskNames[variableParts[1]]; ok {
+				validPipelineResult = false
+				continue
+			}
 			if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
 				validPipelineResult = false
 				invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)


Test patch:
diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go
index 0bdaa3c830a..45389c62204 100644
--- a/pkg/reconciler/pipelinerun/resources/apply_test.go
+++ b/pkg/reconciler/pipelinerun/resources/apply_test.go
@@ -3131,7 +3131,7 @@ func TestApplyFinallyResultsToPipelineResults(t *testing.T) {
 		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
+			received, _ := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /* skippedTasks */)
 			if d := cmp.Diff(tc.expected, received); d != "" {
 				t.Errorf(diff.PrintWantGot(d))
 			}
@@ -3145,6 +3145,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) {
 		results         []v1beta1.PipelineResult
 		taskResults     map[string][]v1beta1.TaskRunResult
 		runResults      map[string][]v1beta1.CustomRunResult
+		skippedTasks    []v1beta1.SkippedTask
 		expectedResults []v1beta1.PipelineRunResult
 		expectedError   error
 	}{{
@@ -3599,9 +3600,28 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) {
 			Name:  "pipeline-result-2",
 			Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"),
 		}},
+	}, {
+		description: "multiple-results-skipped-and-normal-tasks",
+		results: []v1beta1.PipelineResult{{
+			Name:  "pipeline-result-1",
+			Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"),
+		}, {
+			Name:  "pipeline-result-2",
+			Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), $(tasks.normaltask.results.baz)"),
+		}},
+		taskResults: map[string][]v1beta1.TaskRunResult{
+			"normaltask": {{
+				Name:  "baz",
+				Value: *v1beta1.NewStructuredValues("rae"),
+			}},
+		},
+		skippedTasks: []v1beta1.SkippedTask{{
+			Name: "skippedTask",
+		}},
+		expectedResults: nil,
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
+			received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
 			if tc.expectedError != nil {
 				if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {
 					t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))