-
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-0090 Matrix: Refactor Matrix FanOut() to use the Matrix Struct #6246
Conversation
4b3a191
to
1ca42af
Compare
@@ -18,9 +18,9 @@ import ( | |||
) | |||
|
|||
// FanOut produces combinations of Parameters of type String from a slice of Parameters of type Array. | |||
func FanOut(params []v1beta1.Param) Combinations { | |||
func FanOut(matrix v1beta1.Matrix) Combinations { |
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 we please declare this function as a member function:
func (m Matrix) FanOut() Combinations {
var combinations Combinations
for _, parameter := range m.Params {
combinations = combinations.fanOut(parameter)
}
return combinations
}
just like:
func (combinations Combinations) fanOut(param v1beta1.Param) Combinations {
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.
/hold for now until we clarify how this works since the existing logic has matrix.FanOut()
matrixCombinations := matrix.FanOut(rpt.PipelineTask.Matrix.Params).ToMap()
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.
Chatted with Priti and decided for now we will leave as is. The way that Matrix pkg is right now does not make it is easy to simply refactor FanOut()
as a member function.
/hold cancel
/assign @pritidesai
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.
@EmmaMunley is it because of import cycle?
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.
Yes. We would have to move FanOut()
to where the type Matrix is defined in Pipeline_types.go
, however Combinations is defined within Matrix and it doesn't make sense to have Combinations defined in the API.
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 @EmmaMunley!
Yup, it results in cyclic imports 🙃 and bringing combinations
in pkg/apis/
might not be desirable based on recent discussions of avoiding copy/paste in multiple APIs.
@jerop I am happy to merge this for now but wanted to understand this package github.com/tektoncd/pipeline/pkg/matrix
. Also, the package name matrix
being same as the type Matrix
caused confusion in https://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun.go#L837
matrix.FanOut(rpt.PipelineTask.Matrix.Params).ToMap()
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.
Matrix type is defined in pkg/apis/pipeline/v1beta1/
with a couple of member functions. Matrix type has two fields: Params []Param and Include []MatrixInclude
https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/pipeline_types.go#L165
func (matrix *Matrix) MatrixHasInclude() bool { |
func (matrix *Matrix) MatrixHasParams() bool { |
There is a matrix package with matrix.go
and matrix_types.go
with a combinations
defined. Combination type has two fields: MatrixID string and Params []v1beta1.Param.
Can we replace combination
type with matrix struct and add MatrixID to matrix struct? This way we do not have maintain a separate struct and a package.
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.
@pritidesai thanks for the suggestions, refactored the implementation - #6282
1ca42af
to
70ff22d
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`. This PR refactors the FanOut() function to use the Matrix struct instead of the old []Params.
70ff22d
to
31e4f99
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pritidesai 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 |
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
Sorry Priti, I didn't realize you had unresolved comments! Thanks for opening an issue to track. |
I'm curious about what is the motivation for this change? 🤔 |
[TEP-0090: Matrix] introduced
Matrix
to thePipelineTask
specification such that thePipelineTask
executes a list ofTaskRuns
orRuns
in parallel with the specified list of inputs for aParameter
or with different combinations of the inputs for a set ofParameters
.This PR refactors the FanOut() function to use the Matrix struct instead of the old []Params.
/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