don't validate skipped task results for pipeline results

This commit skip the validation for skipped task results in pipeline results. This fixes tektoncd#6139. The skipped task results are not emitted by design thus we shouldn't validate if they are emitted. Pipeline results referenced to skipped task will remain as the original unsubstituted references. Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang · Mar 10, 2023 · 72a556d · 72a556d
1 parent 67abc4a
commit 72a556d
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 7 deletions.
diff --git 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 remain as the original unsubstituted references.
 
 ## 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
@@ -723,7 +723,7 @@ 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())
+			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks, logger)
 		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
@@ -23,6 +23,8 @@ import (
 	"strings"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"go.uber.org/zap"
+
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
 	"github.com/tektoncd/pipeline/pkg/substitution"
@@ -315,9 +317,16 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st
 func ApplyTaskResultsToPipelineResults(
 	results []v1beta1.PipelineResult,
 	taskRunResults map[string][]v1beta1.TaskRunResult,
-	customTaskResults map[string][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) {
+	customTaskResults map[string][]v1beta1.CustomRunResult,
+	skippedTasks []v1beta1.SkippedTask,
+	logger *zap.SugaredLogger) ([]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 +347,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 {
+				logger.Debugf("Result reference %s is not substituted because referenced task %s is skipped", variable, variableParts[1])
+				continue
+			}
 			if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
 				validPipelineResult = false
 				invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)

diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go
@@ -27,6 +27,7 @@ import (
 	"github.com/tektoncd/pipeline/test/diff"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/selection"
+	logtesting "knative.dev/pkg/logging/testing"
 )
 
 func TestApplyParameters(t *testing.T) {
@@ -3131,7 +3132,7 @@ func TestApplyFinallyResultsToPipelineResults(t *testing.T) {
 		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
+			received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil, logtesting.TestLogger(t))
 			if d := cmp.Diff(tc.expected, received); d != "" {
 				t.Errorf(diff.PrintWantGot(d))
 			}
@@ -3145,6 +3146,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 +3601,34 @@ 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: []v1beta1.PipelineRunResult{{
+			Name:  "pipeline-result-1",
+			Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"),
+		}, {
+			Name:  "pipeline-result-2",
+			Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), rae"),
+		}},
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
+			received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, tc.skippedTasks, logtesting.TestLogger(t))
 			if tc.expectedError != nil {
 				if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {
 					t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))