support referencing array params in when expression values #4075

pritidesai · 2021-07-06T21:38:04Z

Changes

Array params are referenced using $(params.arrayParam[*]). When expressions values is a list of string and was designed to reference and resolve an array parameter. But the implementation was limiting the array param usage in the when expression values. Adding support for the array params in when expressions.

/kind bug

Closes #4061

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

When expression values can have array parameter reference now, such as, values: [$(params.arrayParam[*])]

jerop

thanks for this fix @pritidesai 😁

could we please add a task with array params to the example pipeline with when expressions? also would be great to have an test case invalidating specifying array parameters in the input field

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

jerop · 2021-07-07T19:58:26Z

pkg/apis/pipeline/v1beta1/when_validation_test.go

@@ -51,6 +51,13 @@ func TestWhenExpressions_Valid(t *testing.T) {
 			Operator: selection.In,
 			Values:   []string{""},
 		}},
+	}, {
+		name: "valid input with string variable and values with array variables",


we could also add the array in input test case here

the set of tests here is running positive tests. We could add it as one of the failure tests but nothing changed in terms of validating the input.

These tests are validating WhenExpressions.validate() defined here. The purpose of WhenExpressions.validate() is to validate each field for non-empty values and validating task result pattern.

This test is also not necessary here and can be removed.

pritidesai · 2021-07-08T00:01:31Z

looks like a flake 😞

/test tekton-pipeline-unit-tests

vdemeester

If we support it for when expression, should we also support this on variable interpolation ? (otherwise might feel a bit inconsistent ).

pritidesai · 2021-07-08T17:56:09Z

If we support it for when expression, should we also support this on variable interpolation ? (otherwise might feel a bit inconsistent ).

I think we do support this on variable interpolations using the same syntax $(params.arrayParam[*]), we are missing support for the array indexing i.e. $(params.arrayParam[2]) in general.

vdemeester · 2021-07-09T08:41:23Z

If we support it for when expression, should we also support this on variable interpolation ? (otherwise might feel a bit inconsistent ).

I think we do support this on variable interpolations using the same syntax $(params.arrayParam[*]), we are missing support for the array indexing i.e. $(params.arrayParam[2]) in general.

Ah I see 👌🏼

jerop · 2021-07-19T15:47:21Z

pkg/apis/pipeline/v1beta1/when_validation.go

+			// one of the values could be a reference to an array param, such as, $(params.foo[*])
+			// extract the variable name from the pattern $(params.foo[*]), if the variable name matches with one of the array params
+			// validate the param as an array variable otherwise, validate it as a string variable
+			if arrayParamNames.Has(strings.TrimSuffix(strings.TrimPrefix(val, "$("+prefix+"."), "[*])")) {


we're checking if the variable is an array param in two places is slightly different ways, wondering if we can create a common function to check for array param that can be used in both places

pipeline/pkg/apis/pipeline/v1beta1/when_types.go

Line 72 in 232ec0e

if _, ok := arrayReplacements[strings.TrimSuffix(strings.TrimPrefix(val, "$("), "[*])")]; ok {

pipeline/pkg/apis/pipeline/v1beta1/when_validation.go

Line 84 in 232ec0e

if arrayParamNames.Has(strings.TrimSuffix(strings.TrimPrefix(val, "$("+prefix+"."), "[*])")) {

Yup, it's very similar but the first expression is returning params.arrayParam1 from $(params.arrayParam1[*]) while the second expression is returning arrayParam1 from $(params.arrayParam1[*]).

The arrayReplacements in the first expression hold a list of array parameters with params prefix while the arrayParamNames hold the list of parameters without such prefix.

I have created a common function to return just the parameter name which works for the second expression and added prefix back for the first expression.

Validation and applying replacements works with a different patterns because the apply.go adds the prefix before creating a list of parameter names:

pipeline/pkg/reconciler/pipelinerun/resources/apply.go

Line 44 in 2e5780d

arrayReplacements[fmt.Sprintf("params.%s", p.Name)] = p.Default.ArrayVal

We can refactor this further (in a separate PR) to have a consistent validation and replacements routines.

thanks for the refactor, looks great!

pkg/apis/pipeline/v1beta1/pipeline_validation_test.go

tekton-robot · 2021-07-19T18:54:45Z

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/v1beta1/param_types.go	96.9%	97.0%	0.1

tekton-robot · 2021-07-19T19:19:49Z

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/v1beta1/param_types.go	96.9%	97.0%	0.1

pritidesai · 2021-07-19T19:26:53Z

/retest

jerop · 2021-07-19T23:01:13Z

/test pull-tekton-pipeline-integration-tests

jerop

thanks @pritidesai!

tekton-robot · 2021-07-20T00:09:40Z

[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:

~~OWNERS~~ [jerop]

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

ghost

Requesting one additional unit test case but otherwise this lgtm!

ghost · 2021-08-03T14:03:36Z

pkg/apis/pipeline/v1beta1/when_types_test.go

+		original: &WhenExpression{
+			Input:    "$(params.path)",
+			Operator: selection.In,
+			Values:   []string{"$(params.branches[*])", "$(params.files[*])"},


One additional test-case here for completeness would be Values with string and array vars mixed together:

original: &WhenExpression{ Input: "$(params.path)", Operator: selection.In, Values: []string{"$(params.branches[*])", "$(params.matchPath)", "$(params.files[*])"}, }, replacements: map[string]string{ "params.path": "readme.md", "params.matchPath": "foo.txt", }, arrayReplacements: map[string][]string{ "params.branches": {"dev", "stage"}, "params.files": {"readme.md", "test.go"}, }, expected: &WhenExpression{ Input: "readme.md", Operator: selection.In, Values: []string{"dev", "stage", "foo.txt", "readme.md", "test.go"}, },

thanks @sbwsg for an additional unit test, done 👍

Array params are referenced using $(params.arrayParam[*]), when expressions values is a list of string and was designed to resolve an array parameter. But the implementation was limiting the array param usage in the values. Adding support for the array params in when expressions.

tekton-robot · 2021-08-05T00:11: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/v1beta1/param_types.go	96.9%	97.0%	0.1

ghost · 2021-08-05T12:06:21Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Jul 6, 2021

tekton-robot requested review from dlorenc and a user July 6, 2021 21:38

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 6, 2021

jerop reviewed Jul 7, 2021

View reviewed changes

pritidesai force-pushed the issue-4061 branch 2 times, most recently from a72b11b to 232ec0e Compare July 7, 2021 23:17

vdemeester reviewed Jul 8, 2021

View reviewed changes

pritidesai added this to the Pipelines v0.27 milestone Jul 13, 2021

jerop self-assigned this Jul 13, 2021

jerop reviewed Jul 19, 2021

View reviewed changes

pritidesai force-pushed the issue-4061 branch from 232ec0e to 52ccee3 Compare July 19, 2021 18:52

pritidesai force-pushed the issue-4061 branch from 52ccee3 to 198203b Compare July 19, 2021 19:16

jerop approved these changes Jul 20, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2021

ghost reviewed Aug 3, 2021

View reviewed changes

pritidesai force-pushed the issue-4061 branch from 198203b to a88b55c Compare August 5, 2021 00:09

tekton-robot assigned ghost Aug 5, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

tekton-robot merged commit 557c393 into tektoncd:main Aug 5, 2021

pritidesai deleted the issue-4061 branch August 5, 2021 16:20

VeereshAradhya mentioned this pull request Nov 29, 2021

RHDEVDOCS-3308 [Recurring] Release Notes, Known Issues, and Bug Fixes… openshift/openshift-docs#38489

Closed

MikhailKorol-saritasa mentioned this pull request Apr 28, 2022

feature: kaniko extra args saritasa-nest/saritasa-devops-helm-charts#25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support referencing array params in when expression values #4075

support referencing array params in when expression values #4075

pritidesai commented Jul 6, 2021

jerop left a comment

jerop Jul 7, 2021

pritidesai Jul 7, 2021 •

edited

Loading

pritidesai commented Jul 8, 2021

vdemeester left a comment

pritidesai commented Jul 8, 2021 •

edited

Loading

vdemeester commented Jul 9, 2021

jerop Jul 19, 2021

pritidesai Jul 19, 2021 •

edited

Loading

jerop Jul 19, 2021

tekton-robot commented Jul 19, 2021

tekton-robot commented Jul 19, 2021

pritidesai commented Jul 19, 2021

jerop commented Jul 19, 2021

jerop left a comment

tekton-robot commented Jul 20, 2021

ghost left a comment

ghost Aug 3, 2021

pritidesai Aug 5, 2021

tekton-robot commented Aug 5, 2021

ghost commented Aug 5, 2021

support referencing array params in when expression values #4075

support referencing array params in when expression values #4075

Conversation

pritidesai commented Jul 6, 2021

Changes

Submitter Checklist

Release Notes

jerop left a comment

Choose a reason for hiding this comment

jerop Jul 7, 2021

Choose a reason for hiding this comment

pritidesai Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

pritidesai commented Jul 8, 2021

vdemeester left a comment

Choose a reason for hiding this comment

pritidesai commented Jul 8, 2021 • edited Loading

vdemeester commented Jul 9, 2021

jerop Jul 19, 2021

Choose a reason for hiding this comment

pritidesai Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

jerop Jul 19, 2021

Choose a reason for hiding this comment

tekton-robot commented Jul 19, 2021

tekton-robot commented Jul 19, 2021

pritidesai commented Jul 19, 2021

jerop commented Jul 19, 2021

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 20, 2021

ghost left a comment

Choose a reason for hiding this comment

ghost Aug 3, 2021

Choose a reason for hiding this comment

pritidesai Aug 5, 2021

Choose a reason for hiding this comment

tekton-robot commented Aug 5, 2021

ghost commented Aug 5, 2021

pritidesai Jul 7, 2021 •

edited

Loading

pritidesai commented Jul 8, 2021 •

edited

Loading

pritidesai Jul 19, 2021 •

edited

Loading