fix: refine error resean with invalid pipelinename in taskrunspecs #6957

cugykw · 2023-07-21T09:35:30Z

Changes

This PR tries to modify the scene where the pipelineRun containing an invalid pipeline name in taskRunSpecs displays an error mismatch.

fix: #6956

/kind bug

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

When the taskRunSpecs of the pipelineRun contains an invalid pipeline task name, the cause of the `InvalidTaskRunSpecs` error is displayed.

tekton-robot · 2023-07-21T09:35:39Z

Hi @cugykw. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-07-21T09:44:38Z

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	91.8%	92.1%	0.3

dibyom · 2023-07-21T19:33:44Z

/ok-to-test

tekton-robot · 2023-07-21T19:33:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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~~ [dibyom]

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

tekton-robot · 2023-07-21T19:41:10Z

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	91.8%	92.1%	0.3

cugykw · 2023-07-27T11:17:21Z

/cc @vdemeester

vdemeester · 2023-08-09T08:43:10Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -83,8 +83,9 @@ const (
 	// ReasonInvalidWorkspaceBinding indicates that a Pipeline expects a workspace but a
 	// PipelineRun has provided an invalid binding.
 	ReasonInvalidWorkspaceBinding = "InvalidWorkspaceBindings"
-	// ReasonInvalidServiceAccountMapping indicates that PipelineRun.Spec.TaskRunSpecs[].TaskServiceAccountName is defined with a wrong taskName
-	ReasonInvalidServiceAccountMapping = "InvalidServiceAccountMappings"


🤔 Couldn't this happen still ?
I understand that we were sending this error wrongly (as it could have bee n something different that invalid service account mapping, but.. could/would we have cases where issuing this error is still valid ?

ok, i kept this reason.

I think this reason related function is removed, and deprecated? I don't see a reason to keep it.
#3028

It is indeed not used here. This commit will be deleted first, and if it is used again later, it will be added?

tekton-robot · 2023-08-10T12:17:37Z

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.4%	92.7%	0.3

tekton-robot · 2023-08-10T12:19:44Z

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.4%	92.7%	0.3

Yongxuanzhang · 2023-08-15T16:51:46Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -83,8 +83,9 @@ const (
 	// ReasonInvalidWorkspaceBinding indicates that a Pipeline expects a workspace but a
 	// PipelineRun has provided an invalid binding.
 	ReasonInvalidWorkspaceBinding = "InvalidWorkspaceBindings"
-	// ReasonInvalidServiceAccountMapping indicates that PipelineRun.Spec.TaskRunSpecs[].TaskServiceAccountName is defined with a wrong taskName
-	ReasonInvalidServiceAccountMapping = "InvalidServiceAccountMappings"


I think this reason related function is removed, and deprecated? I don't see a reason to keep it.
#3028

Yongxuanzhang · 2023-08-15T16:57:18Z

pkg/reconciler/pipelinerun/pipelinerun.go

+	// ReasonInvalidTaskRunSpec indicates that PipelineRun.Spec.TaskRunSpecs[].PipelineTaskName is defined with
+	// a not exist taskName in pipelineSpec.
+	ReasonInvalidTaskRunSpec = "InvalidTaskRunSpecs"


InvalidTaskRunSpecs seems a bit vague here, we are actually validating the pipeline task name from taskrunspec is defined in pipeline tasks. I'm not sure, I won't disagree if we keep this naming..

The ValidateTaskRunSpecs function currently only validates the name, but there may be other validations in the future? InvalidTaskRunSpecs indicates a large error type, combined with the message field to indicate a specific error?

tekton-robot · 2023-08-16T01:44:15Z

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.4%	92.7%	0.3

tekton-robot · 2023-08-16T01:46:27Z

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.4%	92.7%	0.3

Yongxuanzhang · 2023-08-16T15:22:55Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Jul 21, 2023

tekton-robot requested review from abayer and imjasonh July 21, 2023 09:35

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 21, 2023

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 21, 2023

dibyom approved these changes Jul 21, 2023

View reviewed changes

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

tekton-robot requested a review from vdemeester July 27, 2023 11:17

vdemeester reviewed Aug 9, 2023

View reviewed changes

cugykw force-pushed the refine-taskrunspec-err-reason branch from 5362f58 to 4caef5f Compare August 10, 2023 12:10

Yongxuanzhang reviewed Aug 15, 2023

View reviewed changes

fix: refine error resean with invalid pipelinename in taskrunspecs

a685221

cugykw force-pushed the refine-taskrunspec-err-reason branch from 4caef5f to a685221 Compare August 16, 2023 01:38

tekton-robot assigned Yongxuanzhang Aug 16, 2023

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

tekton-robot merged commit 7884311 into tektoncd:main Aug 16, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

cugykw commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

dibyom commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

cugykw commented Jul 27, 2023

vdemeester Aug 9, 2023

cugykw Aug 10, 2023

Yongxuanzhang Aug 15, 2023

cugykw Aug 16, 2023

tekton-robot commented Aug 10, 2023

tekton-robot commented Aug 10, 2023

Yongxuanzhang Aug 15, 2023

Yongxuanzhang Aug 15, 2023

cugykw Aug 16, 2023

tekton-robot commented Aug 16, 2023

tekton-robot commented Aug 16, 2023

Yongxuanzhang commented Aug 16, 2023

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

fix: refine error resean with invalid pipelinename in taskrunspecs #6957

Conversation

cugykw commented Jul 21, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

dibyom commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

tekton-robot commented Jul 21, 2023

cugykw commented Jul 27, 2023

vdemeester Aug 9, 2023

Choose a reason for hiding this comment

cugykw Aug 10, 2023

Choose a reason for hiding this comment

Yongxuanzhang Aug 15, 2023

Choose a reason for hiding this comment

cugykw Aug 16, 2023

Choose a reason for hiding this comment

tekton-robot commented Aug 10, 2023

tekton-robot commented Aug 10, 2023

Yongxuanzhang Aug 15, 2023

Choose a reason for hiding this comment

Yongxuanzhang Aug 15, 2023

Choose a reason for hiding this comment

cugykw Aug 16, 2023

Choose a reason for hiding this comment

tekton-robot commented Aug 16, 2023

tekton-robot commented Aug 16, 2023

Yongxuanzhang commented Aug 16, 2023