TEP-0118: Enable pipeline to handle matrix include params #6248
Conversation
tekton-robot
added
kind/feature
|
[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 |
tekton-robot
added
the
size/XL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
2 times, most recently
from
d72aa67
to
430f575
Compare
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/assign |
|
/assign |
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
from
430f575
to
627d912
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/hold Thanks Quan! Holding this for now. May need to include implementation FanOut() logic with this as well. |
tekton-robot
added
the
do-not-merge/hold
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
2 times, most recently
from
cb8a770
to
77c38a3
Compare
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
2 times, most recently
from
74532d8
to
c5ff9b3
Compare
|
The following is the coverage report on the affected files.
|
EmmaMunley
changed the title
|
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.
|
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
from
c5ff9b3
to
fbea2b2
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
from
fbea2b2
to
3c23992
Compare
|
/ unhold |
1 similar comment
|
/ unhold |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
from
3c23992
to
6447e5d
Compare
[TEP-0090: Matrix] introduced `Matrix` to the `PipelineTask` specification such that the `PipelineTask` executes a list of `TaskRuns` or `Runs` in parallel with the specified list of inputs for a `Parameter` or with different combinations of the inputs for a set of `Parameters`. To build on this, Tep-0018 introduced Matrix.Include, which allows passing in a specific combinations of `Parameters` into the `Matrix`. This commit enables pipeline to handle matrix include params Note: This feature is still in preview mode. Implementation logic will be added in subsequent commits.
EmmaMunley
force-pushed
the
pipeline-conversion-matrix-include
branch
from
6447e5d
to
c9a3de6
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.
|
|
Thanks for all the changes Emma! 😺 /unhold |
tekton-robot
removed
the
do-not-merge/hold
| @@ -259,6 +259,16 @@ func (m *Matrix) convertTo(ctx context.Context, sink *v1.Matrix) { | |||
| param.convertTo(ctx, &new) | |||
| sink.Params = append(sink.Params, new) | |||
| } | |||
| if m.hasInclude() { | |||
There was a problem hiding this comment.
nit: the check here might also be removed
There was a problem hiding this comment.
"handling matrix include params" is ambiguous, let's clarify what this means in the commit and PR description 🙏🏾
looking into the PR itself, it seems there are three things happening:
- refactoring extraction of parameters from a pipelinetask
- conversion of matrix.include to/from v1
- substituting result references in matrix.include.params
I recommend splitting this PR into three PRs such that each does only one thing 🙏🏾
|
@EmmaMunley related to Jerop's comment, with a lot of background copy-pasted between matrix include commit messages, it's hard to tell what's actually changing as a result of the commit. Can you focus the commit messages more on the specific changes being introduced? |
|
Thank you for the feedback and suggestions. @JeromeJu @JeromeJu @lbernick I have addressed the comments and broken this out into multiple PRs and shortened the commit messages to be more specific.
|
|
thanks for breaking the PRs up Emma! |
[TEP-0090: Matrix] introduced
Matrixto thePipelineTaskspecification such that thePipelineTaskexecutes a list ofTaskRunsorRunsin parallel with the specified list of inputs for aParameteror with different combinations of the inputs for a set ofParameters.To build on this TEP-0018 introduced Matrix.Include, which allows passing in a specific combinations of
Parametersinto theMatrix.This PR enables pipeline to handle matrix include params.
Note: This feature is still in preview mode. Implementation logic will be added in subsequent commits.
/kind feature
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