-
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
change param array indexing validation functions to member functions #6180
change param array indexing validation functions to member functions #6180
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
47dda5e
to
e838c52
Compare
The following is the coverage report on the affected files.
|
e838c52
to
b2de238
Compare
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.
|
/assign |
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.
Thank you for splitting the PR into pieces, I'm trying to rename those funcs to make them easier to read. Please take a look.
Please note: arrayParamLength
seems useless for all funcs retrieving the indexing param references.
I tried to remove them all but I never run the code I modified so please double check :)
Learning how to make multi-lines suggestions 🙃
As a workaround, please refer to https://github.com/tektoncd/pipeline/compare/main...XinruZhang:pipeline:refactor-suggestion?expand=1
b2de238
to
d03fc5e
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Thanks a lot! Really helpful! |
return nil | ||
} | ||
|
||
func extractParamIndex(paramReference string, arrayParams map[string]int, outofBoundParams *sets.String) { | ||
// ExtractArrayIndexingParamRefs extract the array indexing references | ||
func ExtractArrayIndexingParamRefs(paramReference string, arrayParams map[string]int, arrayIndexingParams *[]string) { |
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.
"arrayParams" is not used in this function right? as well as other extractArrayIndexingRefs*()
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.
yes, I missed this one. Thanks!
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.
I removed it from all the functions used
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.
Thank you! @Yongxuanzhang
d03fc5e
to
51cf8fb
Compare
|
||
// ValidateOutofBoundArrayParams validates if the array indexing params are out of bound | ||
func ValidateOutofBoundArrayParams(arrayIndexingParams []string, arrayParams map[string]int) error { |
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.
I'm nitpicking but changing arrayParams
to arrayParamsLengths
can be clearer.
The following is the coverage report on the affected files.
|
51cf8fb
to
214c202
Compare
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.
|
8ee5a41
to
978c017
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
This commit refactors param array indexing validation to extract all references first, then validate if references are out of bound. Previously we traverse all references and collect invalid references there. The benefit of this refactoring is that it would be convenient for other validation to reuse these references. This commit also moves validation functions into api and change them to member functions. Signed-off-by: Yongxuan Zhang [email protected]
978c017
to
47343d4
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Note that the coverage drop of pipeline_validation.go and task_validation.go is due to previous uncovered cases. We should add them in a separate PR. |
arrayParamsLengths := ts.Params.extractParamArrayLengths() | ||
for k, v := range params.extractParamArrayLengths() { | ||
arrayParamsLengths[k] = v | ||
} |
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.
NIT: much cleaner with maps.Copy(dest, source) 😄
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.
Yes I didn't use it because we don't import that in the code base and I try to avoid changes like that
Thank you @Yongxuanzhang, do you mind tracking it in a issue so that its not forgotten? /lgtm /cancel hold |
/hold cancel |
Sure! Just opened one and will open a PR soon! |
@Yongxuanzhang please update release notes to include the type change |
Oh thanks! I thought I have updated it but I didn't 😞 . |
This commit updates []Param to the new Params type everywhere in the code base. This is a follow up to PR tektoncd#6180 where the type type Params []Param was introduced to allow member functions.
This commit updates []Param to the new Params type everywhere in the code base. This is a follow up to PR tektoncd#6180 where the type type Params []Param was introduced to allow member functions. This change will make it easier to make changes related to the type.
This commit updates []Param to the new Params type everywhere in the code base. This is a follow up to PR #6180 where the type type Params []Param was introduced to allow member functions. This change will make it easier to make changes related to the type.
Changes
This commit refactors param array indexing validation to extract all
references first, then validate if references are out of bound.
Previously we traverse all references and collect invalid references
there. The benefit of this refactoring is that it would be convenient
for other validation to reuse these references.
This commit also moves validation functions into api and change them to
member functions.
/kind misc
Signed-off-by: Yongxuan Zhang [email protected]
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes