tektoncd
Clean up Regexes
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
- all the three notations (dot notation, bracket with single quote, and bracket with double quote)
- the suffix of indexing part including star operator and int indexing that was not included before.
Also the old regex coupled each notation part with the indexing part, which leads to extra step to be done in the extract-related functions i.e. have to trim the [*] every time.
In this fix, we clean up the regexes and make it as much reable and reuseble as possible.
There will be more PRs to come to clean up considering the complexity of the all regexes in substitution.go and its usage everywhere. 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:
- [ ] 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 - [ ] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
- [ ] Release notes contains the string "action required" if the change requires additional action from users switching to the new release
Release Notes
NONE
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/substitution/substitution.go | 62.2% | 63.9% | 1.7 |
/approve
Nice! Getting out of obfuscated regex hell is a good thing, to say the least.
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/substitution/substitution.go | 62.2% | 63.9% | 1.7 |
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/substitution/substitution.go | 62.2% | 63.9% | 1.7 |
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/substitution/substitution.go | 62.2% | 63.9% | 1.7 |
Ok, pull-tekton-pipeline-alpha-integration-tests keeps failing on the same example tests, which makes me think there is a legit issue.
Ok, pull-tekton-pipeline-alpha-integration-tests keeps failing on the same example tests, which makes me think there is a legit issue.
Thanks @abayer for helping retest. https://github.com/tektoncd/pipeline/pull/5222 that only updates doc files failed same 3 tests in alpha integration test. Let me retry.
@chuangw6 The fact they keep failing in exactly the same way every time really makes me think there's a problem with the regex changes. Might be worth trying to replicate param patterns in those examples as unit tests?
fwiw, I've verified locally that the Pipeline in examples/v1beta1/alpha/pipelinerun-param-array-indexing.yaml fails validation. So yeah, there's an issue. =)
Ok, I took your branch and added this as a test case to pkg/apis/pipeline/v1beta1/pipeline_validation_test.go's TestValidatePipelineParameterVariables test:
{
name: "array index param - using the value of an array index as a string param",
params: []ParamSpec{{
Name: "myArray",
Type: ParamTypeArray,
}},
tasks: []PipelineTask{{
Name: "bar",
TaskRef: &TaskRef{Name: "bar-task"},
Params: []Param{{
Name: "a-param-intended-to-be-from-an-array-index", Value: ArrayOrString{Type: ParamTypeString, StringVal: $(params.myArray[0])"},
}},
}},
}
...and the test case fails on your branch, while passing on main. So yeah, something is definitely broken there.
I'm not enough of a regex wizard (ok, I'm not at all a regex wizard!) to figure out what specifically is wrong, though.
fwiw, I've verified locally that the
Pipelinein examples/v1beta1/alpha/pipelinerun-param-array-indexing.yaml fails validation. So yeah, there's an issue. =)
Thanks @abayer for helping find out this. I'll fix it.
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/substitution/substitution.go | 62.2% | 59.4% | -2.8 |
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/substitution/substitution.go | 62.2% | 59.4% | -2.8 |
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/substitution/substitution.go | 62.2% | 57.9% | -4.3 |
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/substitution/substitution.go | 62.2% | 57.6% | -4.6 |
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/substitution/substitution.go | 62.2% | 57.6% | -4.6 |
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/substitution/substitution.go | 62.2% | 57.6% | -4.6 |