From d5a91fdddc520d407e53324569b5af2d95e612b3 Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Tue, 5 May 2020 16:40:55 +0100 Subject: [PATCH] Split ResolveResultRefs in two ResolveResultRefs is always invoked with either the 2nd or the 3rd parameter set to nil, so it's actually two different things into one function. The second part of the function had not tests, so split it into a separate function and add tests for it. --- pkg/reconciler/pipelinerun/pipelinerun.go | 4 +- .../resources/resultrefresolution.go | 11 +- .../resources/resultrefresolution_test.go | 119 +++++++++++++++++- .../pipelinerun/resources/validate_params.go | 2 +- 4 files changed, 130 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 6d407ae593b..d84b9800a17 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -318,7 +318,7 @@ func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1alpha1.Pip }) } } - resolvedResultRefs, _ := resources.ResolveResultRefs(pipelineState, nil, pipelineSpec.Results) + resolvedResultRefs := resources.ResolvePipelineResultRefs(pipelineState, pipelineSpec.Results) pr.Status.PipelineResults = getPipelineRunResults(pipelineSpec, resolvedResultRefs) } } @@ -557,7 +557,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er } nextRprts := pipelineState.GetNextTasks(candidateTasks) - resolvedResultRefs, err := resources.ResolveResultRefs(pipelineState, nextRprts, nil) + resolvedResultRefs, err := resources.ResolveResultRefs(pipelineState, nextRprts) if err != nil { c.Logger.Infof("Failed to resolve all task params for %q with error %v", pr.Name, err) pr.Status.SetCondition(&apis.Condition{ diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index 66e963b5bd9..f0f42ebeb61 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -37,7 +37,7 @@ type ResolvedResultRef struct { } // ResolveResultRefs resolves any ResultReference that are found in the target ResolvedPipelineRunTask -func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunState, pipelineResults []v1beta1.PipelineResult) (ResolvedResultRefs, error) { +func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunState) (ResolvedResultRefs, error) { var allResolvedResultRefs ResolvedResultRefs for _, target := range targets { resolvedResultRefs, err := convertParamsToResultRefs(pipelineRunState, target) @@ -46,13 +46,20 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta } allResolvedResultRefs = append(allResolvedResultRefs, resolvedResultRefs...) } + return removeDup(allResolvedResultRefs), nil +} + +// ResolvePipelineResultRefs takes a list of PipelineResults and resolves any references they +// include to Task results in the given PipelineRunState +func ResolvePipelineResultRefs(pipelineRunState PipelineRunState, pipelineResults []v1beta1.PipelineResult) ResolvedResultRefs { + var allResolvedResultRefs ResolvedResultRefs for _, result := range pipelineResults { resolvedResultRefs := convertPipelineResultToResultRefs(pipelineRunState, result) if resolvedResultRefs != nil { allResolvedResultRefs = append(allResolvedResultRefs, resolvedResultRefs...) } } - return removeDup(allResolvedResultRefs), nil + return removeDup(allResolvedResultRefs) } // extractResultRefs resolves any ResultReference that are found in param or pipeline result diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 69e62f0043c..6a15b8bdb68 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -10,6 +10,8 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" tb "github.com/tektoncd/pipeline/test/builder" + corev1 "k8s.io/api/core/v1" + "knative.dev/pkg/apis" ) func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { @@ -366,7 +368,7 @@ func TestResolveResultRefs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := ResolveResultRefs(tt.args.pipelineRunState, tt.args.targets, nil) + got, err := ResolveResultRefs(tt.args.pipelineRunState, tt.args.targets) sort.SliceStable(got, func(i, j int) bool { return strings.Compare(got[i].FromTaskRun, got[j].FromTaskRun) < 0 }) @@ -380,3 +382,118 @@ func TestResolveResultRefs(t *testing.T) { }) } } + +func TestResolvePipelineResultRefs(t *testing.T) { + type args struct { + pipelineRunState PipelineRunState + pipelineResults []v1beta1.PipelineResult + } + pipelineRunState := PipelineRunState{ + { + TaskRunName: "aTaskRun", + TaskRun: tb.TaskRun("aTaskRun", tb.TaskRunStatus( + tb.TaskRunResult("aResult", "aResultValue"), + )), + PipelineTask: &v1alpha1.PipelineTask{ + Name: "aTask", + TaskRef: &v1alpha1.TaskRef{Name: "aTask"}, + }, + }, { + TaskRunName: "bTaskRun", + TaskRun: tb.TaskRun("bTaskRun", tb.TaskRunStatus( + tb.StatusCondition(apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse})), + ), + PipelineTask: &v1alpha1.PipelineTask{ + Name: "bTask", + TaskRef: &v1alpha1.TaskRef{Name: "bTask"}, + }, + }, { + PipelineTask: &v1alpha1.PipelineTask{ + Name: "cTask", + TaskRef: &v1alpha1.TaskRef{Name: "cTask"}, + }, + }, + } + + tests := []struct { + name string + args args + want ResolvedResultRefs + }{ + { + name: "Test pipeline result from a successful task", + args: args{ + pipelineRunState: pipelineRunState, + pipelineResults: []v1beta1.PipelineResult{ + { + Name: "from-a", + Value: "$(tasks.aTask.results.aResult)", + Description: "a result from a", + }, + }, + }, + want: ResolvedResultRefs{ + { + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "aResultValue", + }, + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aTask", + Result: "aResult", + }, + FromTaskRun: "aTaskRun", + }, + }, + }, + { + name: "Test results from a task that did not run and one and failed", + args: args{ + pipelineRunState: pipelineRunState, + pipelineResults: []v1beta1.PipelineResult{ + { + Name: "from-a", + Value: "$(tasks.aTask.results.aResult)", + Description: "a result from a", + }, + { + Name: "from-b", + Value: "$(tasks.bTask.results.bResult)", + Description: "a result from b", + }, + { + Name: "from-c", + Value: "$(tasks.cTask.results.cResult)", + Description: "a result from c", + }, + }, + }, + want: ResolvedResultRefs{ + { + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "aResultValue", + }, + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aTask", + Result: "aResult", + }, + FromTaskRun: "aTaskRun", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ResolvePipelineResultRefs(tt.args.pipelineRunState, tt.args.pipelineResults) + sort.SliceStable(got, func(i, j int) bool { + return strings.Compare(got[i].FromTaskRun, got[j].FromTaskRun) < 0 + }) + if d := cmp.Diff(tt.want, got); d != "" { + t.Fatalf("ResolveResultRef -want, +got: %v", d) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index 00b0a30e96a..e0fc78c0018 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -22,7 +22,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" ) -// Validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type. +// ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type. func ValidateParamTypesMatching(p *v1alpha1.PipelineSpec, pr *v1alpha1.PipelineRun) error { // Build a map of parameter names/types declared in p. paramTypes := make(map[string]v1alpha1.ParamType)