simplifying matrix combinations #6312

pritidesai · 2023-03-07T23:32:14Z

Changes

Matrix combinations is created in [][]Param format which is very hard to process with matrix.Include. The parameters value can change in a combination or a new combination can be added based on matrix.Include.Params.

Its easy to compare and work with map[string]string instead of []Param.

Now, this combination can be directly consumed while processing matrix.Include as demonstrated here: https://play.golang.com/p/Hy48eFFtlVo

/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

tekton-robot · 2023-03-07T23:32:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/apis/pipeline/v1/matrix_types.go

pritidesai · 2023-03-08T20:51:58Z

@EmmaMunley this is still WIP but just opened it up for an early review and to unblock your efforts, thanks!

pkg/apis/pipeline/v1beta1/matrix_types.go

pritidesai · 2023-03-09T02:07:32Z

docs/pipeline-api.md

@@ -8658,11 +8714,7 @@ The names of the <code>params</code> must match the names of the <code>params</c
 <h3 id="tekton.dev/v1beta1.Param">Param
 </h3>
 <p>
-<<<<<<< HEAD


I am surprised to see this kind of github tags here? 🤔

its coming from https://github.com/tektoncd/pipeline/blob/e8f26371d22a3420458d23c2684b12a961df9198/docs/pipeline-api.md#param-1

#6210
I think we should prioritize this issue-- I will hopefully have some time to look at it

tekton-robot · 2023-03-09T03:23:07Z

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/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T03:28:15Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T03:58:15Z

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/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T03:58:19Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T04:50:34Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T05:06:46Z

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/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T05:38:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

lbernick · 2023-03-09T15:06:33Z

docs/pipeline-api.md

@@ -8658,11 +8714,7 @@ The names of the <code>params</code> must match the names of the <code>params</c
 <h3 id="tekton.dev/v1beta1.Param">Param
 </h3>
 <p>
-<<<<<<< HEAD


#6210
I think we should prioritize this issue-- I will hopefully have some time to look at it

pkg/apis/pipeline/v1/matrix_types.go

pkg/apis/pipeline/v1beta1/matrix_types.go

lbernick · 2023-03-09T15:13:11Z

pkg/apis/pipeline/v1/matrix_types.go

-		Name:  name,
-		Value: ParamValue{Type: ParamTypeString, StringVal: value},
+// sortCombination sorts the given Combination based on the param names to produce a deterministic ordering
+func (c Combination) sortCombination() ([]string, Combination) {


just curious-- would it make sense to sort in tests instead?

Storing the sorted combinations guarantees the same order and its deterministic - sorted based on param names.

We could sort in tests (matrix_types_test.go and pipelinerun_test.go) instead (also means sorting will be necessary in any tests we add in future) but I could not figure out how to sort params in &taskRuns.Items[i] without making it complicated, please feel free to suggest any fix.

pipeline/pkg/reconciler/pipelinerun/pipelinerun_test.go

Line 8531 in 55d217b

if d := cmp.Diff(expectedTaskRun, &taskRuns.Items[i], ignoreResourceVersion, ignoreTypeMeta); d != "" {

ok, sorting the params here sounds fine. I was a bit worried about people coming to depend on this functionality even if we don't guarantee it. Maybe we could just note in the docs that param ordering is not guaranteed?

the ordering is only for the internal data structure combinations ([]Params) which is not exposed to the users.

ah ok, nvm!

pritidesai · 2023-03-09T16:27:36Z

Hey @jerop and @lbernick, @EmmaMunley is waiting for this PR, will appreciate if we can expedite the review 🙏

pritidesai · 2023-03-09T16:31:35Z

I am pair programming with @EmmaMunley to support Matrix.Include. These changes are a pre-requisite to simplify the existing data structure such that its consumable and expanding matrix to support `include is clean.

lbernick · 2023-03-09T17:07:50Z

Hey @jerop and @lbernick, @EmmaMunley is waiting for this PR, will appreciate if we can expedite the review 🙏

np! feel free to use the "request review" button (I do tend to check emails from GH) or DM me when you need another round

tekton-robot · 2023-03-09T18:27:36Z

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/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T18:28:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T19:34:13Z

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/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

tekton-robot · 2023-03-09T19:36:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	88.3%	88.3%	0.0

pkg/apis/pipeline/v1/matrix_types.go

jerop

one small change requested, happy to merge after it and lee's comment is addressed ,thanks @pritidesai 🙏🏾

pkg/reconciler/pipelinerun/pipelinerun.go

Matrix combinations is created in [][]Param format which is very hard to process with matrix.Include. The parameters value can change in a combination or a new combination can be added based on matrix.Include.Params. Its easy to compare and work with map[string]string instead of []Param. Signed-off-by: pritidesai <[email protected]>

pritidesai · 2023-03-09T22:06:17Z

one small change requested, happy to merge after it and lee's comment is addressed ,thanks @pritidesai 🙏🏾

thanks @jerop, both yours and @lbernick's comments addressed, thanks!

jerop

/lgtm

🎉

tekton-robot · 2023-03-09T22:34:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, lbernick

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:

~~OWNERS~~ [jerop,lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

EmmaMunley · 2023-03-10T17:06:47Z

Thank you Priti for making these changes! This is a big help with moving forward with the FanOut() matrix for include parameters and helped unblocked my PR building on top of this: #6341

tekton-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. labels Mar 7, 2023

tekton-robot requested review from afrittoli and lbernick March 7, 2023 23:32

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 7, 2023

jerop reviewed Mar 8, 2023

View reviewed changes

pkg/apis/pipeline/v1/matrix_types.go Outdated Show resolved Hide resolved

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

pritidesai force-pushed the combinations-to-slice-of-map branch from 230ed74 to 7f695cb Compare March 8, 2023 20:27

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

pritidesai marked this pull request as ready for review March 8, 2023 20:51

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2023

tekton-robot requested a review from jerop March 8, 2023 20:51

jerop reviewed Mar 8, 2023

View reviewed changes

pkg/apis/pipeline/v1beta1/matrix_types.go Outdated Show resolved Hide resolved

jerop reviewed Mar 8, 2023

View reviewed changes

pkg/apis/pipeline/v1beta1/matrix_types.go Outdated Show resolved Hide resolved

pritidesai force-pushed the combinations-to-slice-of-map branch from 7f695cb to 70949c5 Compare March 9, 2023 01:40

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 9, 2023

pritidesai force-pushed the combinations-to-slice-of-map branch from 70949c5 to 9a79d28 Compare March 9, 2023 02:05

pritidesai commented Mar 9, 2023

View reviewed changes

pritidesai force-pushed the combinations-to-slice-of-map branch 2 times, most recently from d96db68 to 8c29357 Compare March 9, 2023 03:17

pritidesai force-pushed the combinations-to-slice-of-map branch from 8c29357 to f9c07ed Compare March 9, 2023 03:50

pritidesai force-pushed the combinations-to-slice-of-map branch from f9c07ed to 50e272d Compare March 9, 2023 04:42

pritidesai force-pushed the combinations-to-slice-of-map branch from 50e272d to 1eb5dc7 Compare March 9, 2023 05:01

lbernick reviewed Mar 9, 2023

View reviewed changes

pritidesai force-pushed the combinations-to-slice-of-map branch from 1eb5dc7 to e3164f6 Compare March 9, 2023 18:21

pritidesai force-pushed the combinations-to-slice-of-map branch from e3164f6 to 42d941d Compare March 9, 2023 19:28

lbernick approved these changes Mar 9, 2023

View reviewed changes

pkg/apis/pipeline/v1/matrix_types.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2023

jerop reviewed Mar 9, 2023

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

pritidesai force-pushed the combinations-to-slice-of-map branch from 42d941d to c561322 Compare March 9, 2023 22:04

jerop approved these changes Mar 9, 2023

View reviewed changes

tekton-robot assigned jerop Mar 9, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2023

tekton-robot merged commit c8ac4d4 into tektoncd:main Mar 9, 2023

pritidesai deleted the combinations-to-slice-of-map branch March 9, 2023 23:06

pritidesai mentioned this pull request Mar 10, 2023

TEP-0118: Implement Fanning Out logic to support Matrix Include Parameters in a Task Run #6341

Merged

7 tasks

lbernick mentioned this pull request Aug 1, 2023

[TEP-0140] Produce Results from Matrixed PipelineTasks tektoncd/community#1047

Merged

lbernick mentioned this pull request Aug 21, 2023

Change ordering of Matrixed TaskRuns Produced to Not Be Deterministic #7056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplifying matrix combinations #6312

simplifying matrix combinations #6312

pritidesai commented Mar 7, 2023

tekton-robot commented Mar 7, 2023

pritidesai commented Mar 8, 2023

pritidesai Mar 9, 2023

pritidesai Mar 9, 2023

lbernick Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

lbernick Mar 9, 2023

lbernick Mar 9, 2023

pritidesai Mar 9, 2023

lbernick Mar 9, 2023

pritidesai Mar 9, 2023

lbernick Mar 9, 2023

pritidesai commented Mar 9, 2023

pritidesai commented Mar 9, 2023

lbernick commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

jerop left a comment

pritidesai commented Mar 9, 2023

jerop left a comment

tekton-robot commented Mar 9, 2023

EmmaMunley commented Mar 10, 2023

simplifying matrix combinations #6312

simplifying matrix combinations #6312

Conversation

pritidesai commented Mar 7, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 7, 2023

pritidesai commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented Mar 9, 2023

pritidesai commented Mar 9, 2023

lbernick commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

tekton-robot commented Mar 9, 2023

jerop left a comment

Choose a reason for hiding this comment

pritidesai commented Mar 9, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 9, 2023

EmmaMunley commented Mar 10, 2023