TEP-0118: Add validation for matrix pipeline context parameter variables #6238

EmmaMunley · 2023-02-27T17:26:04Z

[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 PR validates that matrix pipeline context parameter variables including include params are used appropriately.

Note: This is still in preview mode. Other forms of validation and 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:

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-02-27T17:32:19Z

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/apis/pipeline/v1/param_types.go	98.3%	98.3%	0.1
pkg/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/param_types.go	98.2%	98.3%	0.1
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-27T17:35:11Z

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/apis/pipeline/v1/param_types.go	98.3%	98.3%	0.1
pkg/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/param_types.go	98.2%	98.3%	0.1
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

lbernick

thanks @EmmaMunley! it looks like your commit message/PR description describe the changes to context variable validation, but not param type validation. Can you update them to reflect all the changes in this PR?

pkg/apis/pipeline/v1/param_types.go

pkg/apis/pipeline/v1/pipeline_validation_test.go

tekton-robot · 2023-02-28T02:35:54Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T02:38:58Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T02:55:19Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T02:57:57Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T15:47:06Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T15:48:03Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

pkg/apis/pipeline/v1/pipeline_validation_test.go

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

tekton-robot · 2023-02-28T16:02:30Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T16:02:35Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

…les include Matrix.Include.Params [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 validates that matrix pipeline context parameter variables including include params are used appropriately. Note: This is still in preview mode. Other forms of validation and implementation logic will be added in subsequent commits.

tekton-robot · 2023-02-28T16:29:26Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

tekton-robot · 2023-02-28T16:53:09Z

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/apis/pipeline/v1/pipeline_validation.go	99.4%	99.5%	0.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0

jerop

@EmmaMunley - Thanks for breaking this up into small PRs!

tekton-robot · 2023-02-28T20:54:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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:

~~pkg/apis/OWNERS~~ [jerop]

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

pritidesai · 2023-02-28T22:15:52Z

pkg/apis/pipeline/v1/pipeline_validation.go

@@ -184,13 +184,25 @@ func validatePipelineContextVariables(tasks []PipelineTask) *apis.FieldError {
 	var paramValues []string
 	for _, task := range tasks {
 		var matrixParams []Param
+		var includeParams []Param


Just having a discussion in other PR #6180, we could introduce a new type and use that instead of []Param:

type Params []Param

Not a blocker for this PR though!

Hey @EmmaMunley, please make sure to follow up on this once PR #6180 is resolved.

pritidesai · 2023-02-28T22:20:03Z

pkg/apis/pipeline/v1/pipeline_validation.go

 		for _, param := range append(task.Params, matrixParams...) {
 			paramValues = append(paramValues, param.Value.StringVal)
 			paramValues = append(paramValues, param.Value.ArrayVal...)
 		}
+
+		if task.Matrix.MatrixHasInclude() {
+			for _, param := range append(task.Params, includeParams...) {
+				paramValues = append(paramValues, param.Value.StringVal)
+			}
+		}


We could leverage a common function retrieving values from params:

https://github.com/tektoncd/pipeline/pull/6180/files#diff-16493f3be05dfe9ce9d789d3b4b277f77d13deb8673084b3efe9034d20eb2987R121-R131

Also at this point in the execution, it must be safe to rely on this member function Params.extractParamValuesFromParams which is collecting values from an objects as well since we have a validation upfront for limiting such usage.

pritidesai · 2023-02-28T23:04:09Z

/lgtm

Thank you @EmmaMunley 👍 Also, please highlight the changes in the PR description since it's a part of a bigger effort.

This PR validates that matrix pipeline context parameter variables including include params are used appropriately.

[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 adds isolated functionality testing for valid and invalid matrix context as a follow up to tektoncd#6238.** Note: This is still in preview mode. Implementation logic will be added in subsequent commits.

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 27, 2023

tekton-robot requested review from lbernick and pritidesai February 27, 2023 17:26

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 27, 2023

lbernick reviewed Feb 27, 2023

View reviewed changes

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

EmmaMunley force-pushed the validate-pipeline-context-vars branch from f5a07ad to ee1fcb1 Compare February 28, 2023 02:49

EmmaMunley requested review from lbernick and removed request for pritidesai February 28, 2023 13:57

EmmaMunley force-pushed the validate-pipeline-context-vars branch from ee1fcb1 to 58a9d3b Compare February 28, 2023 15:41

EmmaMunley force-pushed the validate-pipeline-context-vars branch from 58a9d3b to b2fed33 Compare February 28, 2023 15:55

lbernick reviewed Feb 28, 2023

View reviewed changes

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

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

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go Show resolved Hide resolved

EmmaMunley force-pushed the validate-pipeline-context-vars branch from b2fed33 to 8a721b3 Compare February 28, 2023 16:22

jerop added this to the Pipelines v0.46 milestone Feb 28, 2023

jerop approved these changes Feb 28, 2023

View reviewed changes

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

EmmaMunley requested a review from lbernick February 28, 2023 21:41

pritidesai reviewed Feb 28, 2023

View reviewed changes

tekton-robot assigned pritidesai Feb 28, 2023

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

tekton-robot merged commit adff467 into tektoncd:main Feb 28, 2023

lbernick mentioned this pull request Mar 2, 2023

TEP-0118: Add validation for matrix include pipeline parameter variables #6235

Merged

7 tasks

EmmaMunley mentioned this pull request Mar 2, 2023

TEP-0118: Add tests for isolating valid and invalid context matrix #6279

Closed

6 tasks

EmmaMunley deleted the validate-pipeline-context-vars branch March 7, 2023 15:26

EmmaMunley mentioned this pull request Mar 13, 2023

TEP-0118: Matrix with Explicit Combinations of Parameters #6351

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0118: Add validation for matrix pipeline context parameter variables #6238

TEP-0118: Add validation for matrix pipeline context parameter variables #6238

EmmaMunley commented Feb 27, 2023 •

edited

Loading

tekton-robot commented Feb 27, 2023

tekton-robot commented Feb 27, 2023

lbernick left a comment

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

jerop left a comment

tekton-robot commented Feb 28, 2023

pritidesai Feb 28, 2023

pritidesai Feb 28, 2023

pritidesai Feb 28, 2023

pritidesai Feb 28, 2023

pritidesai commented Feb 28, 2023

TEP-0118: Add validation for matrix pipeline context parameter variables #6238

TEP-0118: Add validation for matrix pipeline context parameter variables #6238

Conversation

EmmaMunley commented Feb 27, 2023 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Feb 27, 2023

tekton-robot commented Feb 27, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

tekton-robot commented Feb 28, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 28, 2023

pritidesai Feb 28, 2023

Choose a reason for hiding this comment

pritidesai Feb 28, 2023

Choose a reason for hiding this comment

pritidesai Feb 28, 2023

Choose a reason for hiding this comment

pritidesai Feb 28, 2023

Choose a reason for hiding this comment

pritidesai commented Feb 28, 2023

EmmaMunley commented Feb 27, 2023 •

edited

Loading