Skip to content

Commit 2ad0e55

Browse files
YongxuanzhangYongxuanzhang
committed
don't return validation error when taskrun failed/skipped
This commit aims to fix tektoncd#6383, tektoncd#6139 and supports tektoncd#3749. This commits skip the validation if the taskrun is not successful (e.g. failed or skipped) and omit the pipelinerun coresponding result. This way the skipped taskrun won't get validation error for pipelinerun. And the pipelinerun error won't be overwritten by the validation error. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
1 parent ec9306b commit 2ad0e55

File tree

4 files changed

+103
-25
lines changed

4 files changed

+103
-25
lines changed

‎examples/v1beta1/pipelineruns/6139-regression.yaml

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
apiVersion: tekton.dev/v1beta1
2+
kind: PipelineRun
3+
metadata:
4+
name: pipelinerun-test
5+
spec:
6+
pipelineSpec:
7+
params:
8+
- name: say-hello
9+
default: 'false'
10+
tasks:
11+
- name: hello
12+
taskSpec:
13+
results:
14+
- name: result-one
15+
steps:
16+
- image: alpine
17+
script: |
18+
#!/bin/sh
19+
echo "Hello world!"
20+
echo -n "RES1" > $(results.result-one.path)
21+
22+
- name: goodbye
23+
runAfter:
24+
- hello
25+
taskSpec:
26+
results:
27+
- name: result-two
28+
steps:
29+
- image: alpine
30+
script: |
31+
#!/bin/sh
32+
echo "Goodbye world!"
33+
echo -n "RES2" > $(results.result-two.path)
34+
when:
35+
- input: $(params.say-hello)
36+
operator: in
37+
values: ["true"]
38+
39+
results:
40+
- name: result-hello
41+
description: Result one
42+
value: '$(tasks.hello.results.result-one)'
43+
- name: result-goodbye
44+
description: Result two
45+
value: '$(tasks.goodbye.results.result-two)'

‎pkg/reconciler/pipelinerun/pipelinerun.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -688,7 +688,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
688688
pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
689689
if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
690690
pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
691-
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks)
691+
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus())
692692
if err != nil {
693693
pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
694694
return err

‎pkg/reconciler/pipelinerun/resources/apply.go

+16-10
Original file line numberDiff line numberDiff line change
@@ -334,13 +334,9 @@ func ApplyTaskResultsToPipelineResults(
334334
results []v1beta1.PipelineResult,
335335
taskRunResults map[string][]v1beta1.TaskRunResult,
336336
customTaskResults map[string][]v1beta1.CustomRunResult,
337-
skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) {
337+
taskstatus map[string]string) ([]v1beta1.PipelineRunResult, error) {
338338
var runResults []v1beta1.PipelineRunResult
339339
var invalidPipelineResults []string
340-
skippedTaskNames := map[string]bool{}
341-
for _, t := range skippedTasks {
342-
skippedTaskNames[t.Name] = true
343-
}
344340

345341
stringReplacements := map[string]string{}
346342
arrayReplacements := map[string][]string{}
@@ -362,11 +358,7 @@ func ApplyTaskResultsToPipelineResults(
362358
continue
363359
}
364360
variableParts := strings.Split(variable, ".")
365-
// if the referenced task is skipped, we should also skip the results replacements
366-
if _, ok := skippedTaskNames[variableParts[1]]; ok {
367-
validPipelineResult = false
368-
continue
369-
}
361+
370362
if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
371363
validPipelineResult = false
372364
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
@@ -402,6 +394,13 @@ func ApplyTaskResultsToPipelineResults(
402394
} else if resultValue := runResultValue(taskName, resultName, customTaskResults); resultValue != nil {
403395
stringReplacements[variable] = *resultValue
404396
} else {
397+
// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
398+
if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
399+
if status != v1beta1.TaskRunReasonSuccessful.String() {
400+
validPipelineResult = false
401+
continue
402+
}
403+
}
405404
// referred result name is not existent
406405
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
407406
validPipelineResult = false
@@ -419,6 +418,13 @@ func ApplyTaskResultsToPipelineResults(
419418
validPipelineResult = false
420419
}
421420
} else {
421+
// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
422+
if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
423+
if status != v1beta1.TaskRunReasonSuccessful.String() {
424+
validPipelineResult = false
425+
continue
426+
}
427+
}
422428
// referred result name is not existent
423429
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
424430
validPipelineResult = false

‎pkg/reconciler/pipelinerun/resources/apply_test.go

+41-14
Original file line numberDiff line numberDiff line change
@@ -3511,7 +3511,7 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
35113511
results []v1beta1.PipelineResult
35123512
taskResults map[string][]v1beta1.TaskRunResult
35133513
runResults map[string][]v1beta1.CustomRunResult
3514-
skippedTasks []v1beta1.SkippedTask
3514+
taskstatus map[string]string
35153515
expectedResults []v1beta1.PipelineRunResult
35163516
}{{
35173517
description: "non-reference-results",
@@ -3788,13 +3788,47 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
37883788
Value: *v1beta1.NewStructuredValues("rae"),
37893789
}},
37903790
},
3791-
skippedTasks: []v1beta1.SkippedTask{{
3792-
Name: "skippedTask",
3791+
taskstatus: map[string]string{PipelineTaskStatusPrefix + "skippedTask" + PipelineTaskStatusSuffix: PipelineTaskStateNone},
3792+
expectedResults: nil,
3793+
}, {
3794+
description: "unsuccessful-taskrun-no-results",
3795+
results: []v1beta1.PipelineResult{{
3796+
Name: "foo",
3797+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
37933798
}},
3799+
taskResults: map[string][]v1beta1.TaskRunResult{},
3800+
taskstatus: map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
37943801
expectedResults: nil,
3802+
}, {
3803+
description: "unsuccessful-taskrun-no-returned-result-object-ref",
3804+
results: []v1beta1.PipelineResult{{
3805+
Name: "foo",
3806+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key1)"),
3807+
}},
3808+
taskResults: map[string][]v1beta1.TaskRunResult{},
3809+
taskstatus: map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
3810+
expectedResults: nil,
3811+
}, {
3812+
description: "unsuccessful-taskrun-with-results",
3813+
results: []v1beta1.PipelineResult{{
3814+
Name: "foo",
3815+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[*])"),
3816+
}},
3817+
taskResults: map[string][]v1beta1.TaskRunResult{
3818+
"pt1": {
3819+
{
3820+
Name: "foo",
3821+
Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
3822+
},
3823+
}},
3824+
taskstatus: map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
3825+
expectedResults: []v1beta1.PipelineRunResult{{
3826+
Name: "foo",
3827+
Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
3828+
}},
37953829
}} {
37963830
t.Run(tc.description, func(t *testing.T) {
3797-
received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
3831+
received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus)
37983832
if err != nil {
37993833
t.Errorf("Got unecpected error:%v", err)
38003834
}
@@ -3811,6 +3845,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
38113845
results []v1beta1.PipelineResult
38123846
taskResults map[string][]v1beta1.TaskRunResult
38133847
runResults map[string][]v1beta1.CustomRunResult
3848+
taskstatus map[string]string
38143849
expectedResults []v1beta1.PipelineRunResult
38153850
expectedError error
38163851
}{{
@@ -3920,15 +3955,6 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
39203955
},
39213956
expectedResults: nil,
39223957
expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
3923-
}, {
3924-
description: "unsuccessful-taskrun-no-returned-result",
3925-
results: []v1beta1.PipelineResult{{
3926-
Name: "foo",
3927-
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
3928-
}},
3929-
taskResults: map[string][]v1beta1.TaskRunResult{},
3930-
expectedResults: nil,
3931-
expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
39323958
}, {
39333959
description: "mixed-success-tasks-some-returned-results",
39343960
results: []v1beta1.PipelineResult{{
@@ -4010,9 +4036,10 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
40104036
expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
40114037
}} {
40124038
t.Run(tc.description, func(t *testing.T) {
4013-
received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/)
4039+
received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus)
40144040
if err == nil {
40154041
t.Errorf("Expect error but got nil")
4042+
return
40164043
}
40174044

40184045
if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {

0 commit comments

Comments
 (0)