don't return validation error when taskrun failed/skipped #6395

Yongxuanzhang · 2023-03-20T15:47:28Z

Changes

This commit aims to fix #6383, #6139 and supports #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.

/kind bug

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

If taskrun fails and task results not emitted, pipelinerun fails because of taskrun fails rather than results validation error.

tekton-robot · 2023-03-20T15:47:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Yongxuanzhang · 2023-03-20T15:47:54Z

/test all

tekton-robot · 2023-03-20T15:54:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.1%	96.7%	-1.4

tekton-robot · 2023-03-20T15:54:14Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.1%	96.7%	-1.4

tekton-robot · 2023-03-20T19:14:09Z

The following Tekton test failed:

Test name	Commit	Details	Required	Rerun command
pull-tekton-pipeline-go-coverage-df	`455e0e0`	link	true	`/test pull-tekton-pipeline-go-coverage-df`

tekton-robot · 2023-03-20T19:21:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.1%	98.1%	0.0

tekton-robot · 2023-03-20T19:22:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.1%	98.1%	0.0

Yongxuanzhang · 2023-03-20T20:03:30Z

/retest

Yongxuanzhang · 2023-03-20T20:43:41Z

Hi @pritidesai, @vdemeester , could you help me take a look at this PR and see if it could be a fix for those issues? Comments and suggestions are welcome!

vdemeester · 2023-03-21T03:37:23Z

@Yongxuanzhang @pritidesai does it make sense to cherry-pick this for 0.41 and 0.44 (LTS), 0.45 and 0.46 ? (as this is a behavior something that changed)

vdemeester

I would love to have one more yaml for regression test, but other than this, LGTM I think

examples/v1beta1/pipelineruns/6139-regression.yaml

vdemeester · 2023-03-21T15:54:58Z

pkg/reconciler/pipelinerun/resources/apply_test.go

-	}, {
-		description: "unsuccessful-taskrun-no-returned-result",
-		results: []v1beta1.PipelineResult{{
-			Name:  "foo",
-			Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
-		}},
-		taskResults:     map[string][]v1beta1.TaskRunResult{},
-		expectedResults: nil,
-		expectedError:   fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),


Shouldn't we "update" that Test, the expected Error should, however change (not invalid result, but something else) ? But maybe this is not at the same level.

I don't remember why I removed this test, could be a mistake

added this back

Yongxuanzhang · 2023-03-21T17:36:27Z

We better have an e2e test for this pr

vdemeester · 2023-03-29T04:21:00Z

@Yongxuanzhang Any update on this ?

QuanZhang-William · 2023-04-12T17:46:32Z

/assign

Yongxuanzhang · 2023-05-02T20:34:50Z

Hi @pritidesai, wonder if you have availability/interested to review this PR ? Thanks!

pritidesai · 2023-05-16T16:44:40Z

/assign @pritidesai

pritidesai · 2023-05-18T17:49:44Z

pkg/reconciler/pipelinerun/resources/apply_test.go

+		taskstatus:      map[string]string{resources.PipelineTaskStatusPrefix + "pt1" + resources.PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
+		expectedResults: nil,
+	}, {
+		description: "unsuccessful-taskrun-with-results",


thank you for adding this test case which will be add more value and affirmation to #6510

pritidesai · 2023-05-18T17:56:16Z

@Yongxuanzhang Sorry for the delay 🙏

Thanks a bunch for adding an example and also e2e test, it was very helpful in reviewing this PR.

/lgtm

once this is merged, let's cherry pick this into 0.41, 0.44, and 0.47, thanks!

pritidesai · 2023-05-18T19:31:20Z

/cherry-pick release-v0.41.x

pritidesai · 2023-05-18T19:31:30Z

/cherry-pick release-v0.44.x

pritidesai · 2023-05-18T19:31:41Z

/cherry-pick release-v0.47.x

tekton-robot · 2023-05-18T19:32:11Z

@pritidesai: #6395 failed to apply on top of branch "release-v0.41.x":

Applying: don't return validation error when taskrun failed/skipped
Using index info to reconstruct a base tree...
M	pkg/reconciler/pipelinerun/pipelinerun.go
M	pkg/reconciler/pipelinerun/resources/apply.go
M	pkg/reconciler/pipelinerun/resources/apply_test.go
M	test/pipelinerun_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/pipelinerun_test.go
Auto-merging pkg/reconciler/pipelinerun/resources/apply_test.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/apply_test.go
Auto-merging pkg/reconciler/pipelinerun/resources/apply.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/apply.go
Auto-merging pkg/reconciler/pipelinerun/pipelinerun.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 don't return validation error when taskrun failed/skipped
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.41.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2023-05-18T19:32:13Z

@pritidesai: #6395 failed to apply on top of branch "release-v0.44.x":

Applying: don't return validation error when taskrun failed/skipped
Using index info to reconstruct a base tree...
M	pkg/reconciler/pipelinerun/pipelinerun.go
M	pkg/reconciler/pipelinerun/resources/apply.go
M	pkg/reconciler/pipelinerun/resources/apply_test.go
M	test/pipelinerun_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/pipelinerun_test.go
Auto-merging pkg/reconciler/pipelinerun/resources/apply_test.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/apply_test.go
Auto-merging pkg/reconciler/pipelinerun/resources/apply.go
Auto-merging pkg/reconciler/pipelinerun/pipelinerun.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 don't return validation error when taskrun failed/skipped
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.44.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2023-05-18T19:32:17Z

@pritidesai: new pull request created: #6688

In response to this:

/cherry-pick release-v0.47.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This commit closes tektoncd#6139, in previous fix PR: tektoncd#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes #6139, in previous fix PR: #6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes tektoncd#6139, in previous fix PR: tektoncd#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes #6139, in previous fix PR: #6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes tektoncd#6139, in previous fix PR: tektoncd#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com (cherry picked from commit 7c040de)

This commit closes tektoncd#6139, in previous fix PR: tektoncd#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com (cherry picked from commit 7c040de) Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

This commit closes tektoncd#6139, in previous fix PR: tektoncd#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes #6139, in previous fix PR: #6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

This commit closes #6139, in previous fix PR: #6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com (cherry picked from commit 7c040de) Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 20, 2023

tekton-robot requested review from afrittoli and bobcatfish March 20, 2023 15:47

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 20, 2023

Yongxuanzhang force-pushed the 6383-fix branch 2 times, most recently from 2ad0e55 to 455e0e0 Compare March 20, 2023 19:11

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Mar 20, 2023

Yongxuanzhang marked this pull request as ready for review March 20, 2023 19:14

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023

tekton-robot requested review from imjasonh and vdemeester March 20, 2023 19:14

vdemeester requested changes Mar 21, 2023

View reviewed changes

jerop added this to the Pipelines v0.47 milestone Apr 12, 2023

tekton-robot assigned QuanZhang-William Apr 12, 2023

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2023

tekton-robot assigned pritidesai May 16, 2023

pritidesai reviewed May 18, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2023

pritidesai added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label May 18, 2023

tekton-robot merged commit e61deda into tektoncd:main May 18, 2023

tekton-robot mentioned this pull request May 18, 2023

[release-v0.47.x] don't return validation error when taskrun failed/skipped #6688

Merged

Yongxuanzhang mentioned this pull request Nov 23, 2023

don't return validation error when final tasks failed/skipped #7407

Merged

7 tasks

vdemeester mentioned this pull request Dec 13, 2023

[release-v0.47.x] don't return validation error when final tasks failed/skipped #7487

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't return validation error when taskrun failed/skipped #6395

don't return validation error when taskrun failed/skipped #6395

Yongxuanzhang commented Mar 20, 2023 •

edited

Loading

tekton-robot commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

vdemeester commented Mar 21, 2023

vdemeester left a comment

vdemeester Mar 21, 2023

Yongxuanzhang Apr 24, 2023

Yongxuanzhang Apr 25, 2023

Yongxuanzhang commented Mar 21, 2023

vdemeester commented Mar 29, 2023

QuanZhang-William commented Apr 12, 2023

Yongxuanzhang commented May 2, 2023

pritidesai commented May 16, 2023

pritidesai May 18, 2023

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

don't return validation error when taskrun failed/skipped #6395

don't return validation error when taskrun failed/skipped #6395

Conversation

Yongxuanzhang commented Mar 20, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

tekton-robot commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

Yongxuanzhang commented Mar 20, 2023

vdemeester commented Mar 21, 2023

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Mar 21, 2023

Choose a reason for hiding this comment

Yongxuanzhang Apr 24, 2023

Choose a reason for hiding this comment

Yongxuanzhang Apr 25, 2023

Choose a reason for hiding this comment

Yongxuanzhang commented Mar 21, 2023

vdemeester commented Mar 29, 2023

QuanZhang-William commented Apr 12, 2023

Yongxuanzhang commented May 2, 2023

pritidesai commented May 16, 2023

pritidesai May 18, 2023

Choose a reason for hiding this comment

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

pritidesai commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

Yongxuanzhang commented Mar 20, 2023 •

edited

Loading