fix param array indexing validation error reason and error log #6179

Yongxuanzhang · 2023-02-16T02:22:34Z

Changes

This commit fixes the param array indexing validation error reason and error log, the current reason and error messages are wrongly copied from object validation. This commit fixes this error and adds tests.

/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:

N/A 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
N/A Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
N/A Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

Yongxuanzhang · 2023-02-16T02:23:31Z

This is divided from #6120 to break it into smaller prs

tekton-robot · 2023-02-16T02:28:17Z

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	87.1%	87.4%	0.3

tekton-robot · 2023-02-16T02:30:41Z

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	87.1%	87.4%	0.3

XinruZhang · 2023-02-16T16:09:08Z

/assign

XinruZhang · 2023-02-16T19:32:34Z

pkg/reconciler/pipelinerun/pipelinerun.go

-		pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
-			"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
+		pr.Status.MarkFailed(ReasonParamArrayIndexingInvalid,
+			"PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s",


The error message looks confusing, could you please rephrase it a little bit? Is this function ValidateParamArrayIndex only for validating the out-of-boundary error? if so, can we make it clearer?

Thank you!

It is currently out-of-boundary error, but we want to add other checks later (the feature gate check).
Besides, the error from ValidateParamArrayIndex will also be added to this log. It will contain detailed log messages

Proposed codes suggestions has been merged, so I guess we can resolve this comment thread :) Thank you

XinruZhang · 2023-02-16T20:06:38Z

pkg/reconciler/pipelinerun/pipelinerun.go

-		pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
-			"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
+		pr.Status.MarkFailed(ReasonParamArrayIndexingInvalid,
+			"PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s",


Suggested change

"PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s",

"Failed to validate PipelineRun %s/%s: failed to validate Pipeline %s/%s's parameters: %s",

I think we cannot use "Failed" as the first word, it fails the linter of duplicate workds. 😂

tekton-robot · 2023-02-16T20:33:18Z

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	87.1%	87.4%	0.3

XinruZhang

Thank you @Yongxuanzhang

/lgtm

tekton-robot · 2023-02-16T20:37:16Z

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	87.1%	87.4%	0.3

XinruZhang · 2023-02-16T20:38:04Z

/lgtm

tekton-robot · 2023-02-16T20:41:16Z

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	87.1%	87.4%	0.3

tekton-robot · 2023-02-16T20:43:18Z

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	87.1%	87.4%	0.3

XinruZhang · 2023-02-16T20:43:35Z

/test pull-tekton-pipeline-beta-integration-tests

This seems like a network flake:

read tcp 10.53.120.3:41728->185.199.108.154:443: read: connection reset by peer

Yongxuanzhang · 2023-02-16T20:46:22Z

/test pull-tekton-pipeline-beta-integration-tests

This seems like a network flake:

read tcp 10.53.120.3:41728->185.199.108.154:443: read: connection reset by peer

Yes I have seen this a lot recently

tekton-robot · 2023-02-16T21:07:25Z

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	87.1%	87.4%	0.3

tekton-robot · 2023-02-16T21:09: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	87.1%	87.4%	0.3

pritidesai · 2023-02-20T23:45:21Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+		permanentError: true,
+		wantEvents: []string{
+			"Normal Started",
+			"Warning Failed PipelineRun foo/pipeline-param-array-out-of-bound array indexing params fail validation: failed to validate Pipeline foo/a-pipeline-with-array-indexing-params's parameters: non-existent param references:[$(params.some-param[2]",


missing closing brackets )] in [$(params.some-param[2]

is it possible to include the length of an array and the index in the error message non-existent param references:[$(params.some-param[2]? It will help the users to learn exactly what is the issue.

An array index \"2\" is out of bound, parameter \"some-param\" has two elements in \"$(params.some-param[2])\".

the bracket is missed intentionally otherwise it will fail the test. Since we will read the string as regex pattern, and if I adds bracket it will lead to the error. I don't know if there's a better way

is it possible to include the length of an array and the index in the error message non-existent param references:[$(params.some-param[2]? It will help the users to learn exactly what is the issue.

An array index \"2\" is out of bound, parameter \"some-param\" has two elements in \"$(params.some-param[2])\".

That's possible and makes sense. Maybe we could update it after #6180 is merged?

Can you please open an issue to track it?

the bracket is missed intentionally otherwise it will fail the test. Since we will read the string as regex pattern, and if I adds bracket it will lead to the error. I don't know if there's a better way

It looks half baked, how is the error returned to the user when they try to create a pipelineRun?

the bracket is missed intentionally otherwise it will fail the test. Since we will read the string as regex pattern, and if I adds bracket it will lead to the error. I don't know if there's a better way

It looks half baked, how is the error returned to the user when they try to create a pipelineRun?

I think this is just for test purpose, the regex function is only used in tests to check the events. Since event is complicated so it uses regex to match the content of the event. The users should be able to see the full events

Can you please open an issue to track it?

Sure!

pritidesai · 2023-02-20T23:54:17Z

pkg/reconciler/pipelinerun/pipelinerun.go

-		pr.Status.MarkFailed(ReasonObjectParameterMissKeys,
-			"PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s",
+		pr.Status.MarkFailed(ReasonParamArrayIndexingInvalid,
+			"PipelineRun %s/%s array indexing params fail validation: failed to validate Pipeline %s/%s's parameters: %s",


suggest rephrasing to:

PipelineRun \"%s/%s\" failed validation: Failed to validate Pipeline \"%s/%s\"'s parameter which has an invalid index while referring to an array: %s

tekton-robot · 2023-02-21T22:49:43Z

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	87.1%	87.4%	0.3

tekton-robot · 2023-02-21T22:52:55Z

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	87.1%	87.4%	0.3

pritidesai · 2023-02-22T19:29:26Z

thank you @Yongxuanzhang 👍

/approve

tekton-robot · 2023-02-22T19:29:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai, XinruZhang

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

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

Yongxuanzhang · 2023-02-22T19:41:38Z

Thanks! I opened an issue to fix comment: #6212

XinruZhang · 2023-02-22T20:02:02Z

/lgtm

This commit fixes the param array indexing validation error reason and error log, the current reason and error messages are wrongly copied from object validation. This commit fixes this error and adds tests. Signed-off-by: Yongxuan Zhang [email protected]

tekton-robot · 2023-02-22T20:45:28Z

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	87.1%	87.4%	0.3

tekton-robot · 2023-02-22T20:53:13Z

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	87.1%	87.4%	0.3

XinruZhang · 2023-02-22T21:47:06Z

/lgtm

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesnt merit a release note. labels Feb 16, 2023

tekton-robot requested review from dibyom and wlynch February 16, 2023 02:22

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 16, 2023

Yongxuanzhang mentioned this pull request Feb 16, 2023

add feature gate check for param array indexing #6120

Closed

7 tasks

tekton-robot assigned XinruZhang Feb 16, 2023

XinruZhang reviewed Feb 16, 2023

View reviewed changes

XinruZhang approved these changes Feb 16, 2023

View reviewed changes

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

Yongxuanzhang force-pushed the fix-param-error-reason branch from 703a783 to ae4f5a0 Compare February 16, 2023 20:35

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

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

Yongxuanzhang force-pushed the fix-param-error-reason branch from ae4f5a0 to 9097e36 Compare February 16, 2023 21:01

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

pritidesai reviewed Feb 20, 2023

View reviewed changes

Yongxuanzhang force-pushed the fix-param-error-reason branch from 9097e36 to dd6e3aa Compare February 21, 2023 22:44

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

Yongxuanzhang requested review from pritidesai and removed request for wlynch and dibyom February 22, 2023 19:31

Yongxuanzhang mentioned this pull request Feb 22, 2023

include the length of an array and the index in the error message #6212

Closed

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

Yongxuanzhang force-pushed the fix-param-error-reason branch from dd6e3aa to 5cef1f3 Compare February 22, 2023 20:40

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023

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

tekton-robot merged commit 991fb56 into tektoncd:main Feb 22, 2023

	"PipelineRun %s/%s array indexing params fail validation by Pipeline %s/%s's parameters: %s",
	"Failed to validate PipelineRun %s/%s: failed to validate Pipeline %s/%s's parameters: %s",

fix param array indexing validation error reason and error log #6179

fix param array indexing validation error reason and error log #6179

Conversation

Yongxuanzhang commented Feb 16, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

Yongxuanzhang commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

XinruZhang commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 16, 2023

XinruZhang left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 16, 2023

XinruZhang commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

XinruZhang commented Feb 16, 2023

Yongxuanzhang commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

tekton-robot commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 21, 2023

tekton-robot commented Feb 21, 2023

pritidesai commented Feb 22, 2023

tekton-robot commented Feb 22, 2023

Yongxuanzhang commented Feb 22, 2023

XinruZhang commented Feb 22, 2023

tekton-robot commented Feb 22, 2023

tekton-robot commented Feb 22, 2023

XinruZhang commented Feb 22, 2023

Yongxuanzhang commented Feb 16, 2023 •

edited

Loading