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

Yongxuanzhang · 2023-11-23T00:55:47Z

Changes

This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses are considered and final tasks are missing. This PR fixes this.

/kind bug

Signed-off-by: Yongxuan Zhang [email protected]

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-11-23T01:02:42Z

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/pipelinerun.go	92.3%	92.3%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-23T01:04:24Z

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/pipelinerun.go	92.3%	92.3%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

chitrangpatel · 2023-12-06T18:05:14Z

/approve

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 [email protected]

tekton-robot · 2023-12-06T18:36:41Z

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/pipelinerun.go	92.3%	92.3%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-12-06T18:39:08Z

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/pipelinerun.go	92.3%	92.3%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-12-07T13:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [chitrangpatel,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

QuanZhang-William · 2023-12-07T20:34:15Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -542,6 +542,30 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string {
 	return tStatus
 }

+// GetPipelineTaskStatus returns the status of a PipelineFinalTask depending on its taskRun
+func (facts *PipelineRunFacts) GetPipelineFinalTaskStatus() map[string]string {
+	// construct a map of tasks.<pipelineTask>.status and its state


Looks like this code is very similar to GetPipelineTaskStatus. Can we refactor this part 🙏

just the switch part is overlapping, I think that's fine? 🤔

QuanZhang-William · 2023-12-07T20:34:51Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

+				// None includes skipped as well
+				s = PipelineTaskStateNone
+			}
+			tStatus[PipelineTaskStatusPrefix+t.PipelineTask.Name+PipelineTaskStatusSuffix] = s


let's use fmt.Sprintf()?

did you mean fmt.Sprintf(PipelineTaskStatusPrefix, t.PipelineTask.Name, PipelineTaskStatusSuffix)?
we don't have format here, the ide will complain

I was talking about using fmt.Sprintf("%s%s%s", aa, bb, cc). Not really a big deal though

Ohhh 😂 my bad

QuanZhang-William · 2023-12-08T15:53:31Z

Thanks!

/lgtm

vdemeester · 2023-12-13T13:01:30Z

@Yongxuanzhang @QuanZhang-William @chitrangpatel we should probably backport this to affected LTS as well, make sense ?

vdemeester · 2023-12-13T13:02:33Z

/cherry-pick release-v0.47.x

vdemeester · 2023-12-13T13:02:41Z

/cherry-pick release-v0.50.x

vdemeester · 2023-12-13T13:02:47Z

/cherry-pick release-v0.53.x

tekton-robot · 2023-12-13T13:03:14Z

@vdemeester: #7407 failed to apply on top of branch "release-v0.47.x":

Applying: don't return validation error when final tasks failed/skipped
Using index info to reconstruct a base tree...
A	examples/v1/pipelineruns/6139-regression.yaml
M	pkg/reconciler/pipelinerun/pipelinerun.go
M	pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
M	pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Auto-merging pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
Auto-merging pkg/reconciler/pipelinerun/pipelinerun.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/pipelinerun.go
Auto-merging examples/v1beta1/pipelineruns/6139-regression.yaml
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 final tasks 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.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.

tekton-robot · 2023-12-13T13:03:21Z

@vdemeester: new pull request created: #7485

In response to this:

/cherry-pick release-v0.50.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-12-13T13:03:26Z

@vdemeester: new pull request created: #7486

In response to this:

/cherry-pick release-v0.53.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.

Yongxuanzhang · 2023-12-13T16:19:53Z

@Yongxuanzhang @QuanZhang-William @chitrangpatel we should probably backport this to affected LTS as well, make sense ?

Yes! Thanks for helping me to patch this fix. I don't have time this week. >_< But may be able to do so next week or next month after holidays...

vdemeester · 2023-12-20T10:48:54Z

This also affects 0.54.x 🙃
/cherry-pick release-v0.54.x

tekton-robot · 2023-12-20T10:49:54Z

@vdemeester: new pull request created: #7510

In response to this:

This also affects 0.54.x 🙃
/cherry-pick release-v0.54.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.

concaf · 2024-01-03T12:09:22Z

@vdemeester in case you missed it, it failed to apply on 0.47.x - #7407 (comment)

Add some missing functions. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester · 2024-01-03T14:20:22Z

#7487 I did the cherry-pick manually

Add some missing functions. Signed-off-by: Vincent Demeester <[email protected]>

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2023

tekton-robot requested review from afrittoli and bobcatfish November 23, 2023 00:55

chitrangpatel reviewed Dec 6, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go Show resolved Hide resolved

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

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

f3d0f7e

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 [email protected]

Yongxuanzhang force-pushed the fix-6139-2 branch from e7c0f6a to f3d0f7e Compare December 6, 2023 18:30

vdemeester approved these changes Dec 7, 2023

View reviewed changes

QuanZhang-William reviewed Dec 7, 2023

View reviewed changes

tekton-robot assigned QuanZhang-William Dec 8, 2023

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

tekton-robot merged commit 7c040de into tektoncd:main Dec 8, 2023
13 checks passed

tekton-robot mentioned this pull request Dec 13, 2023

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

Merged

tekton-robot mentioned this pull request Dec 13, 2023

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

Merged

tekton-robot mentioned this pull request Dec 20, 2023

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

Merged

vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Jan 3, 2024

Adapt cherry-pick from tektoncd#7407 to 0.47.x

695526e

Add some missing functions. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Jan 3, 2024

Adapt cherry-pick from tektoncd#7407 to 0.47.x

98067a1

Add some missing functions. Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot pushed a commit that referenced this pull request Jan 3, 2024

Adapt cherry-pick from #7407 to 0.47.x

37920b1

Add some missing functions. Signed-off-by: Vincent Demeester <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Yongxuanzhang commented Nov 23, 2023

tekton-robot commented Nov 23, 2023

tekton-robot commented Nov 23, 2023

chitrangpatel commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 7, 2023

QuanZhang-William Dec 7, 2023 •

edited

Loading

Yongxuanzhang Dec 8, 2023

QuanZhang-William Dec 7, 2023

Yongxuanzhang Dec 8, 2023

QuanZhang-William Dec 8, 2023

Yongxuanzhang Dec 8, 2023

QuanZhang-William commented Dec 8, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

Yongxuanzhang commented Dec 13, 2023 •

edited

Loading

vdemeester commented Dec 20, 2023

tekton-robot commented Dec 20, 2023

concaf commented Jan 3, 2024

vdemeester commented Jan 3, 2024

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

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

Conversation

Yongxuanzhang commented Nov 23, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 23, 2023

tekton-robot commented Nov 23, 2023

chitrangpatel commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 6, 2023

tekton-robot commented Dec 7, 2023

QuanZhang-William Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Yongxuanzhang Dec 8, 2023

Choose a reason for hiding this comment

QuanZhang-William Dec 7, 2023

Choose a reason for hiding this comment

Yongxuanzhang Dec 8, 2023

Choose a reason for hiding this comment

QuanZhang-William Dec 8, 2023

Choose a reason for hiding this comment

Yongxuanzhang Dec 8, 2023

Choose a reason for hiding this comment

QuanZhang-William commented Dec 8, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

vdemeester commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

tekton-robot commented Dec 13, 2023

Yongxuanzhang commented Dec 13, 2023 • edited Loading

vdemeester commented Dec 20, 2023

tekton-robot commented Dec 20, 2023

concaf commented Jan 3, 2024

vdemeester commented Jan 3, 2024

QuanZhang-William Dec 7, 2023 •

edited

Loading

Yongxuanzhang commented Dec 13, 2023 •

edited

Loading