Skip to content

Commit bca7030

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 8e8c163 commit bca7030

File tree

5 files changed

+187
-15
lines changed

5 files changed

+187
-15
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
@@ -687,7 +687,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
687687
pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
688688
if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
689689
pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
690-
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks)
690+
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus())
691691
if err != nil {
692692
pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
693693
return err

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

+16-10
Original file line numberDiff line numberDiff line change
@@ -338,13 +338,9 @@ func ApplyTaskResultsToPipelineResults(
338338
results []v1beta1.PipelineResult,
339339
taskRunResults map[string][]v1beta1.TaskRunResult,
340340
customTaskResults map[string][]v1beta1.CustomRunResult,
341-
skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) {
341+
taskstatus map[string]string) ([]v1beta1.PipelineRunResult, error) {
342342
var runResults []v1beta1.PipelineRunResult
343343
var invalidPipelineResults []string
344-
skippedTaskNames := map[string]bool{}
345-
for _, t := range skippedTasks {
346-
skippedTaskNames[t.Name] = true
347-
}
348344

349345
stringReplacements := map[string]string{}
350346
arrayReplacements := map[string][]string{}
@@ -366,11 +362,7 @@ func ApplyTaskResultsToPipelineResults(
366362
continue
367363
}
368364
variableParts := strings.Split(variable, ".")
369-
// if the referenced task is skipped, we should also skip the results replacements
370-
if _, ok := skippedTaskNames[variableParts[1]]; ok {
371-
validPipelineResult = false
372-
continue
373-
}
365+
374366
if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
375367
validPipelineResult = false
376368
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
@@ -406,6 +398,13 @@ func ApplyTaskResultsToPipelineResults(
406398
} else if resultValue := runResultValue(taskName, resultName, customTaskResults); resultValue != nil {
407399
stringReplacements[variable] = *resultValue
408400
} else {
401+
// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
402+
if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
403+
if status != v1beta1.TaskRunReasonSuccessful.String() {
404+
validPipelineResult = false
405+
continue
406+
}
407+
}
409408
// referred result name is not existent
410409
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
411410
validPipelineResult = false
@@ -423,6 +422,13 @@ func ApplyTaskResultsToPipelineResults(
423422
validPipelineResult = false
424423
}
425424
} else {
425+
// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
426+
if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
427+
if status != v1beta1.TaskRunReasonSuccessful.String() {
428+
validPipelineResult = false
429+
continue
430+
}
431+
}
426432
// referred result name is not existent
427433
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
428434
validPipelineResult = false

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

+39-3
Original file line numberDiff line numberDiff line change
@@ -3512,6 +3512,7 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
35123512
results []v1beta1.PipelineResult
35133513
taskResults map[string][]v1beta1.TaskRunResult
35143514
runResults map[string][]v1beta1.CustomRunResult
3515+
taskstatus map[string]string
35153516
skippedTasks []v1beta1.SkippedTask
35163517
expectedResults []v1beta1.PipelineRunResult
35173518
}{{
@@ -3789,13 +3790,47 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
37893790
Value: *v1beta1.NewStructuredValues("rae"),
37903791
}},
37913792
},
3792-
skippedTasks: []v1beta1.SkippedTask{{
3793-
Name: "skippedTask",
3793+
taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "skippedTask" + resources.PipelineTaskStatusSuffix: resources.PipelineTaskStateNone},
3794+
expectedResults: nil,
3795+
}, {
3796+
description: "unsuccessful-taskrun-no-results",
3797+
results: []v1beta1.PipelineResult{{
3798+
Name: "foo",
3799+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
37943800
}},
3801+
taskResults: map[string][]v1beta1.TaskRunResult{},
3802+
taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
37953803
expectedResults: nil,
3804+
}, {
3805+
description: "unsuccessful-taskrun-no-returned-result-object-ref",
3806+
results: []v1beta1.PipelineResult{{
3807+
Name: "foo",
3808+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key1)"),
3809+
}},
3810+
taskResults: map[string][]v1beta1.TaskRunResult{},
3811+
taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
3812+
expectedResults: nil,
3813+
}, {
3814+
description: "unsuccessful-taskrun-with-results",
3815+
results: []v1beta1.PipelineResult{{
3816+
Name: "foo",
3817+
Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[*])"),
3818+
}},
3819+
taskResults: map[string][]v1beta1.TaskRunResult{
3820+
"pt1": {
3821+
{
3822+
Name: "foo",
3823+
Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
3824+
},
3825+
}},
3826+
taskstatus: map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
3827+
expectedResults: []v1beta1.PipelineRunResult{{
3828+
Name: "foo",
3829+
Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
3830+
}},
37963831
}} {
37973832
t.Run(tc.description, func(t *testing.T) {
3798-
received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
3833+
received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus)
37993834
if err != nil {
38003835
t.Errorf("Got unecpected error:%v", err)
38013836
}
@@ -4014,6 +4049,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
40144049
received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/)
40154050
if err == nil {
40164051
t.Errorf("Expect error but got nil")
4052+
return
40174053
}
40184054

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

‎test/pipelinerun_test.go

+86-1
Original file line numberDiff line numberDiff line change
@@ -744,7 +744,7 @@ metadata:
744744
spec:
745745
params:
746746
- name: HELLO
747-
value: "Hello World!"
747+
value: "Hello World!"
748748
pipelineRef:
749749
name: %s
750750
`, namespace, pipelineName))
@@ -952,3 +952,88 @@ func getLimitRange(name, namespace, resourceCPU, resourceMemory, resourceEphemer
952952
},
953953
}
954954
}
955+
956+
func TestPipelineRunTaskFailed(t *testing.T) {
957+
ctx := context.Background()
958+
ctx, cancel := context.WithCancel(ctx)
959+
defer cancel()
960+
c, namespace := setup(ctx, t)
961+
962+
knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
963+
defer tearDown(ctx, t, c, namespace)
964+
965+
taskName := helpers.ObjectNameForTest(t)
966+
pipelineName := helpers.ObjectNameForTest(t)
967+
prName := helpers.ObjectNameForTest(t)
968+
969+
t.Logf("Creating Task, Pipeline, and Pending PipelineRun %s in namespace %s", prName, namespace)
970+
971+
if _, err := c.V1beta1TaskClient.Create(ctx, parse.MustParseV1beta1Task(t, fmt.Sprintf(`
972+
metadata:
973+
name: %s
974+
namespace: %s
975+
spec:
976+
steps:
977+
- image: ubuntu
978+
command: ['/bin/bash']
979+
args: ['-c', 'echo hello, world']
980+
`, taskName, namespace)), metav1.CreateOptions{}); err != nil {
981+
t.Fatalf("Failed to create Task `%s`: %s", taskName, err)
982+
}
983+
984+
if _, err := c.V1beta1PipelineClient.Create(ctx, parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(`
985+
metadata:
986+
name: %s
987+
namespace: %s
988+
spec:
989+
tasks:
990+
- name: task
991+
taskRef:
992+
name: %s
993+
`, pipelineName, namespace, taskName)), metav1.CreateOptions{}); err != nil {
994+
t.Fatalf("Failed to create Pipeline `%s`: %s", pipelineName, err)
995+
}
996+
997+
pipelineRun, err := c.V1beta1PipelineRunClient.Create(ctx, parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`
998+
metadata:
999+
name: %s
1000+
namespace: %s
1001+
spec:
1002+
pipelineSpec:
1003+
results:
1004+
- name: abc
1005+
value: "$(tasks.xxx.results.abc)"
1006+
tasks:
1007+
- name: xxx
1008+
taskSpec:
1009+
results:
1010+
- name: abc
1011+
steps:
1012+
- name: update-sa
1013+
image: bash:latest
1014+
script: |
1015+
echo 'test' > $(results.abc.path)
1016+
exit 1
1017+
`, prName, namespace)), metav1.CreateOptions{})
1018+
if err != nil {
1019+
t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err)
1020+
}
1021+
// Wait for the PipelineRun to fail.
1022+
if err := WaitForPipelineRunState(ctx, c, prName, timeout, PipelineRunFailed(prName), "PipelineRunFailed", v1beta1Version); err != nil {
1023+
t.Fatalf("Waiting for PipelineRun to fail: %v", err)
1024+
}
1025+
1026+
pipelineRun, err = c.V1beta1PipelineRunClient.Get(ctx, prName, metav1.GetOptions{})
1027+
fmt.Println("!!!pipelineRun",pipelineRun.Status.Conditions)
1028+
if err != nil {
1029+
t.Fatalf("Error getting PipelineRun %s: %s", prName, err)
1030+
}
1031+
1032+
if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
1033+
t.Errorf("Expected PipelineRun to fail but found condition: %s", pipelineRun.Status.GetCondition(apis.ConditionSucceeded))
1034+
}
1035+
expectedMessage := "Tasks Completed: 1 (Failed: 1, Cancelled 0), Skipped: 0"
1036+
if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message!=expectedMessage{
1037+
t.Errorf("Expected PipelineRun to fail with condition message: %s but got: %s", expectedMessage, pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message)
1038+
}
1039+
}

0 commit comments

Comments
 (0)