-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup: Move array indexing validation out of apis package #6617
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is a huge improvement!
I wonder should we remove these checks as well?
pipeline/pkg/reconciler/taskrun/resources/apply.go
Lines 66 to 69 in 09d422c
if config.CheckAlphaOrBetaAPIFields(ctx) { | |
for i := 0; i < len(p.Default.ArrayVal); i++ { | |
stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Default.ArrayVal[i] | |
} |
pipeline/pkg/reconciler/taskrun/resources/apply.go
Lines 109 to 112 in 09d422c
if config.CheckAlphaOrBetaAPIFields(ctx) { | |
for i := 0; i < len(p.Value.ArrayVal); i++ { | |
stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Value.ArrayVal[i] | |
} |
pipeline/pkg/reconciler/pipelinerun/resources/apply.go
Lines 67 to 70 in 09d422c
if config.CheckAlphaOrBetaAPIFields(ctx) { | |
for i := 0; i < len(p.Default.ArrayVal); i++ { | |
stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Default.ArrayVal[i] | |
} |
pipeline/pkg/reconciler/pipelinerun/resources/apply.go
Lines 118 to 121 in 09d422c
if config.CheckAlphaOrBetaAPIFields(ctx) { | |
for i := 0; i < len(p.Value.ArrayVal); i++ { | |
stringReplacements[fmt.Sprintf(pattern+"[%d]", p.Name, i)] = p.Value.ArrayVal[i] | |
} |
Thanks @Yongxuanzhang I've removed the additional reconciler checks you linked. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
@tektoncd/core-maintainers please take a look, this is one of our remaining v1 blockers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! Just have some minor comments/questions.
Let me know when we need a lgtm
@Yongxuanzhang done, thanks! |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
We validate that indexing references to array parameters are in bounds, based on the default values of the parameters and the parameter values passed in from a PipelineRun/TaskRun. This validation happens in the reconciler, since it requires comparing the Task/Pipeline spec with the parameters defined in the TaskRun/PipelineRun. Prior to this commit, validation code was in the apis package, even though it was called in the reconciler. This is confusing, because this code cannot be used to validate a Task/Pipeline spec in isolation; it can only be used in the context of TaskRuns and PipelineRuns. This commit moves this validation logic to the reconciler and does not introduce any functional changes.
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/approve |
[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:
Approvers can indicate their approval by writing |
/lgtm |
not sure if the requested changes will block this pr or not, but I think we have split this PR into smaller ones. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/retest |
We validate that indexing references to array parameters are in bounds,
based on the default values of the parameters and the parameter values
passed in from a PipelineRun/TaskRun. This validation happens in the reconciler,
since it requires comparing the Task/Pipeline spec with the parameters
defined in the TaskRun/PipelineRun.
Prior to this commit, validation code was in the apis package, even though it was called
in the reconciler. This is confusing, because this code cannot be used to validate a Task/Pipeline
spec in isolation; it can only be used in the context of TaskRuns and PipelineRuns.
This commit moves this validation logic to the reconciler and does not introduce any functional changes.
Related: #6607
/kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes