-
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
Add matrix support for using references to entire PipelineRun array parameters #6516
Add matrix support for using references to entire PipelineRun array parameters #6516
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
4c342f5
to
1aa7a91
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
1aa7a91
to
2d869cc
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
2d869cc
to
3028b72
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
3028b72
to
4086e94
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
4086e94
to
7dedfc6
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-alpha-integration-tests |
c8b2634
to
2be3808
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.
|
2be3808
to
3c25384
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
echo "$(params.platform) and $(params.browser)" | ||
`) | ||
|
||
expectedTaskRuns := []*v1beta1.TaskRun{ |
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 wonder if we could use for loop to create the expectedTaskRuns
to reduce the size of the test code? The difference between each case seem to be the params
, we could use %s in the mustParseTaskRunWithObjectMeta
to pass the params?
e.g.
expectedTaskRun := mustParseTaskRunWithObjectMeta(t, |
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.
We could ignore this comment in this PR, I just feel like it would be helpful if we could clean up it.
The suggestion in my mind is something like:
data := []struct {
name string
browser string
platform string
}{{
name: "pr-platforms-and-browsers-0",
browser: "chrome",
platform: "linux",
}, {
name: "pr-platforms-and-browsers-1",
browser: "chrome",
platform: "mac",
}}
expectedTaskRuns := []*v1beta1.TaskRun{}
for _, d := range data {
expectedTaskRuns1 = append(expectedTaskRuns,
mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta(d.name, "foo",
"pr", "p-dag", "platforms-and-browsers", false),
fmt.Sprintf(`
spec:
params:
- name: browser
value: %s
- name: platform
value: %s
serviceAccountName: test-sa
taskRef:
name: mytask
kind: Task
labels:
tekton.dev/memberOf: tasks
tekton.dev/pipeline: p-dag
`, d.browser, d.platform)),
)
}
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 for the suggestion @Yongxuanzhang. I updated this here and think in the future we could refactor all of the tests to use this and perhaps make a createExpectedTaskRuns()
function that can abstract some of this even more from a mapping of parameter names and values.
I also think there's opportunity to create more variables (pr name, namespace, pipelineTaskName, etc that can be passed between the specs to avoid some of the manual errors and pain points around formatting the yaml syntax.
…arameters This commit adds support for using whole array PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056
3c25384
to
e555157
Compare
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
/test pull-tekton-pipeline-alpha-integration-tests |
This is a known flake: #4567 |
/test pull-tekton-pipeline-go-coverage-df |
@EmmaMunley: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
/test pull-tekton-pipeline-go-coverage |
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.
|
Changes
This PR adds support for using whole array PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements.
Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR.
This issue is described in more detail here: #6056 (comment)
Thanks to @abayer for investigating and co-authoring this PR: #6056 😄
/kind feature
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