-
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
[TEP-0076]Pipeline results support array #4965
[TEP-0076]Pipeline results support array #4965
Conversation
The following is the coverage report on the affected files.
|
9f49461
to
892c284
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
892c284
to
c26cf39
Compare
The following is the coverage report on the affected files.
|
c26cf39
to
abc94fa
Compare
The following is the coverage report on the affected files.
|
abc94fa
to
1ebb0e7
Compare
The following is the coverage report on the affected files.
|
1ebb0e7
to
6fbe987
Compare
/test pull-tekton-pipeline-go-coverage |
/retest |
1 similar comment
/retest |
/assign @ywluogg |
6fbe987
to
846e618
Compare
846e618
to
a5f6bf5
Compare
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.
NIce job! Everything looks good except some typos
var arrayIndexingRegex = regexp.MustCompile(arrayIndexing) | ||
|
||
// ArrayIndexingRegex is used to match `[int]` and `[*]` | ||
var ArrayIndexingRegex = regexp.MustCompile(arrayIndexing) |
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 wondering if we need to export this regex or if there is a parsing function it would make more sense to export
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.
oh that's a good point! Let me check if I can have a separate function for this
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.
could you please avoid exporting the regexes here?
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.
it is not exported now
e122b33
to
a356fdc
Compare
The following is the coverage report on the affected files.
|
/lgtm |
/retest |
intIdx, _ := strconv.Atoi(stringIdx) | ||
stringReplacements[variable] = resultValue.ArrayVal[intIdx] | ||
} else { | ||
variable = strings.TrimSuffix(strings.TrimSuffix(strings.TrimPrefix(variable, "$("), ")"), "[*]") |
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.
can this be moved into a helper function somewhere? it would be good to have all our string parsing logic in one place.
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.
sure!
a356fdc
to
a22a53c
Compare
The following is the coverage report on the affected files.
|
a22a53c
to
1e482c4
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick, ywluogg 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 |
The following is the coverage report on the affected files.
|
/lgtm |
This is part of work in TEP-0076. This commit update the PipelineResult and PipelineRunResult to uspport array results, meanwhile support the whole array and array indexing reference from PipelineResult to TaskResult. Before this commit the pipeline level result only support string.
1e482c4
to
aa139ec
Compare
The following is the coverage report on the affected files.
|
/retest |
/lgtm |
Changes
This is part of work in TEP-0076.
This commit update the PipelineResult and PipelineRunResult to uspport
array results, meanwhile support the whole array and array indexing
reference from PipelineResult to TaskResult. Before this commit the
pipeline level result only support string.
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes