Clean up Regexes #5255
Clean up Regexes #5255
Conversation
|
The following is the coverage report on the affected files.
|
|
/approve Nice! Getting out of obfuscated regex hell is a good thing, to say the least. |
|
/test pull-tekton-pipeline-alpha-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-alpha-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/retest |
|
Ok, pull-tekton-pipeline-alpha-integration-tests keeps failing on the same example tests, which makes me think there is a legit issue. |
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/retest |
1 similar comment
|
/retest |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
Hi @abayer, |
|
/lgtm Yay! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The following is the coverage report on the affected files.
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Prior to this commit, the regexes in substitution.go file were hard to
understand and hard to reuse because of the complexity of the regexes.
For example, we have to consider
- all the three notations (dot notation, bracket with single quote,
and bracket with double quote)
- the operator suffix including star `[*]` operator and
int indexing that was not considerd before.
Also the old regex coupled each notation part with the operator
part, which leads to extra step to be done in the extract-related functions
i.e. have to trim the [*] every time.
In this PR, we
- clean up the regexes and make it as much reable and reuseble as possible
- make the method `extractVariablesFromString` be capable of extracting
both variable name and the operator (indexing operator or star operator)
examples:
- $(params.myString) - name: "myString", operator: ""
- $(params['myArray'][2]) - name: "myArray", operator: "1"
- $(params.myObject[*]) - name: "myObject", operator: "*"
Enabling `extractVariablesFromString` to extract both name and operator
is particularly useful for array indexing validation. There will be more
PRs to come to clean up considering the complexity of the all regexes.
Also results-related regexes in resultref.go file need cleanup too.
183737e to
8551303
Compare
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
/retest |
|
@lbernick Since this PR has been changed a bit, please take a look again. |
| @@ -228,7 +221,7 @@ func (arrayOrString *ArrayOrString) applyOrCorrect(stringReplacements map[string | |||
|
|
|||
| // if the stringVal is a string literal or a string that mixed with var references | |||
| // just do the normal string replacement | |||
| if !exactVariableSubstitutionRegex.MatchString(stringVal) { | |||
| if !isolatedResultVariableSubstitutionRegex.MatchString(stringVal) && !substitution.IsIsolatedArrayOrObjectParamRef(stringVal) { | |||
There was a problem hiding this comment.
hm it seems like the substitution package is responsible for determining whether a reference is an isolated reference or not. Can the regex being used be moved to that package instead (and put into a more useful helper function instead of exporting the regex)? With the existing code, each package is only partially responsible for determining how the param reference should be parsed, and I'd like to see better separation of concerns.
| var VariableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat) | ||
| var exactVariableSubstitutionRegex = regexp.MustCompile(exactVariableSubstitutionFormat) | ||
| // ResultVariableSubstitutionRegex is a regex to find all result matching substitutions | ||
| var ResultVariableSubstitutionRegex = regexp.MustCompile(resultVariableSubstitutionFormat) |
There was a problem hiding this comment.
does this need to be exported? based on its usage in the TaskRun reconciler, it looks like it would be more useful as part of a validation helper function.
|
|
||
| // parseIndex parses integer from a string. | ||
| // If the string is not numeric, the second returned value will be false. | ||
| func parseIndex(s string) (int, bool) { |
There was a problem hiding this comment.
nit: Wondering if we should stick with strconv.Atoi here? This func seems to be trimming the error message which we might be interested to boolean.
There was a problem hiding this comment.
And also this func doesn't seem to be used elsewhere at the moment?
|
@chuangw6: PR needs rebase. 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. |
Changes
Prior to this commit, the regexes in substitution.go file were hard to
understand and hard to reuse because of the complexity of the regexes.
For example, we have to consider
and bracket with double quote)
[*]operator andint indexing that was not considerd before.
Also the old regex coupled each notation part with the operator
part, which leads to extra step to be done in the extract-related functions
i.e. have to trim the [*] every time.
This PR
extractVariablesFromStringbe capable of extractingboth variable name and the operator (indexing operator or star operator)
examples:
Enabling
extractVariablesFromStringto extract both name and operatoris particularly useful for array indexing validation. There will be more
PRs to come to clean up considering the complexity of the all regexes.
Also results-related regexes in resultref.go file need cleanup too.
/kind cleanup
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