Add support for array task results as matrix parameters #6056

abayer · 2023-01-26T19:46:29Z

Changes

/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

Array task results can now be used as the value for matrix parameters.

abayer · 2023-01-26T19:55:04Z

/retest

abayer · 2023-01-26T21:04:50Z

/retest

abayer · 2023-01-26T21:12:40Z

/hold
Until I add an example test. =)

afrittoli · 2023-01-26T21:42:15Z

pkg/reconciler/pipelinerun/resources/apply.go

@@ -178,7 +178,7 @@ func ApplyTaskResults(targets PipelineRunState, resolvedResultRefs ResolvedResul
 			pipelineTask := resolvedPipelineRunTask.PipelineTask.DeepCopy()
 			pipelineTask.Params = replaceParamValues(pipelineTask.Params, stringReplacements, arrayReplacements, objectReplacements)
 			if pipelineTask.IsMatrixed() {
-				pipelineTask.Matrix.Params = replaceParamValues(pipelineTask.Matrix.Params, stringReplacements, nil, nil)
+				pipelineTask.Matrix.Params = replaceParamValues(pipelineTask.Matrix.Params, stringReplacements, arrayReplacements, nil)


would it make sense to handle object replacements too?

I'm not sure if objects actually make sense in a matrix params context - thoughts, @jerop?

I also don't think we should handle object results - unless we're speaking of getting a particular value from the object as an item in the matrix array, but even then I'd like to leave that out for when we have use cases and explore it in detail

Thank you, never mind, it was a silly comment.
We might support objects in future there, to make it easier for a task to trigger a specific set of combinations of parameters through results, but that would be a new feature.

afrittoli · 2023-01-27T10:57:50Z

Given the size of the change, and considering that without the feature "feeding results to a matrix" is incomplete, I would consider this PR for a v0.44.1. v0.44 is an LTS release, and it would be a pity for it to be this close to having this functionality.
wdyt?

abayer · 2023-01-27T13:45:40Z

/hold cancel

@afrittoli I agree - #6061 may also make sense to backport.

jerop

thank you @abayer! 🙏🏾

only request is to add some details to the commit message, specifically that array results was supported after matrix was added and add a link to the section of TEP-0090 discussing this feature - https://github.com/tektoncd/community/blob/main/teps/0090-matrix.md#specifying-results-in-the-matrix

jerop · 2023-01-27T15:11:44Z

@abayer it seems some changes are needed in the validation:
"validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: parameters of type array only are allowed in matrix: spec.pipelineSpec.tasks[2].matrix[browser]

pkg/reconciler/pipelinerun/resources/apply_test.go

tekton-robot · 2023-01-27T17:12:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [lbernick]

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

abayer · 2023-01-27T17:57:29Z

it seems some changes are needed in the validation: "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: parameters of type array only are allowed in matrix: spec.pipelineSpec.tasks[2].matrix[browser]

Oh my, yes, this is a rathole. =) So the problem is that we're checking what the raw parsed type is - which will be a string, not an array, since the value is $(tasks.get-browsers.results.browsers[*]). If we turn off the validation check in validateParametersInTaskMatrix, things do just work (or rather, I thought they did, but in my local test, the controller is panicking in ValidateResolvedTaskResources, so there might be something more busted). Aaaanyway - the validation problem is that we're requiring the param value to be an array, but we don't actually know that the initially-parsed string $(tasks.get-browsers.results.browsers[*]) will end up being an array. And now it also appears that there are other issues with resolved task resources, but I haven't dug into that yet.

abayer · 2023-01-27T18:36:11Z

Ok, I have a fix for the validation that I'm fairly happy with, but the next problem is that by the time GetNamesOfTaskRuns(pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name, pipelineTask.GetMatrixCombinationsCount()) gets called within ResolvePipelineTask, the array results parameter is still a string, meaning GetMatrixCombinationsCount() ends up returning 0 (because the "string" param has len(param.Value.ArrayVal) == 0). Digging further.

EDIT:
...yup, there's the problem. It's that matrix, as a whole, can't handle dynamically generated params, not specifically that it can't handle array results. It just doesn't know the size of the matrix at PipelineRun reconciliation time and that breaks a bunch of stuff.

With my validation change (basically just saying "ok, this matrix param is valid if either it's ParamTypeArray or it matches the form of $(whatever[*])), PipelineRun parameters can be used just fine, but not results. This is gonna require some serious thinking to figure out since PipelineRun reconciliation wants to know all the PipelineTasks it'll have to execute immediately, and when we're dependent on an earlier PipelineTask to finish before we know what the matrix combination count is, it's impossible to generate all the matrixed PipelineTask names until that earlier PipelineTask has completed.

cc @jerop

fixes tektoncd#5925 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer · 2023-01-27T19:45:45Z

I've updated the PR with my fix for the validation issue, and with a unit test tweak to use an array result as a matrix parameter, to make it easier to dig into the problem.

afrittoli · 2023-01-27T20:44:53Z

Thanks @abayer for the detailed investigation on this.

So it sounds like this:

this is not something we should backport to v0.44 as it will be a non-trivial change
we could perhaps detect and have a clearer error message for users that attempt to pass results into parameters, and backport that fix instead

wdyt?

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit moves the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: This is only a validation change. Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit moves the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: This is only a validation change. Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

…rameters This commit adds support for using whole array PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

…arameters This commit adds support for using whole array PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit moves the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: This is only a validation change. Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit moves the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: This is only a validation change. Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056

…arameters This commit adds support for using whole array PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: #6056

abayer added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 26, 2023

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 26, 2023

tekton-robot requested review from dibyom and jerop January 26, 2023 19:46

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 26, 2023

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023

afrittoli reviewed Jan 26, 2023

View reviewed changes

abayer force-pushed the array-results-matrix branch from 08c0e15 to 21e8fc4 Compare January 27, 2023 13:27

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2023

abayer force-pushed the array-results-matrix branch from 21e8fc4 to d034cd6 Compare January 27, 2023 14:02

jerop reviewed Jan 27, 2023

View reviewed changes

jerop added this to the Pipelines v0.45 milestone Jan 27, 2023

lbernick reviewed Jan 27, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/apply_test.go Show resolved Hide resolved

lbernick approved these changes Jan 27, 2023

View reviewed changes

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

Add support for array task results as matrix parameters

513a55e

fixes tektoncd#5925 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

abayer force-pushed the array-results-matrix branch from d034cd6 to 513a55e Compare January 27, 2023 19:44

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 27, 2023

jerop closed this Apr 20, 2023

EmmaMunley mentioned this pull request Jun 7, 2023

Change ordering of ResolveResultRefs/ApplyTaskResults #6792

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for array task results as matrix parameters #6056

Add support for array task results as matrix parameters #6056

abayer commented Jan 26, 2023

abayer commented Jan 26, 2023

abayer commented Jan 26, 2023

abayer commented Jan 26, 2023

afrittoli Jan 26, 2023

abayer Jan 26, 2023

jerop Jan 26, 2023 •

edited

Loading

afrittoli Jan 27, 2023

afrittoli commented Jan 27, 2023

abayer commented Jan 27, 2023

jerop left a comment

jerop commented Jan 27, 2023

tekton-robot commented Jan 27, 2023

abayer commented Jan 27, 2023

abayer commented Jan 27, 2023 •

edited

Loading

abayer commented Jan 27, 2023

afrittoli commented Jan 27, 2023

Add support for array task results as matrix parameters #6056

Add support for array task results as matrix parameters #6056

Conversation

abayer commented Jan 26, 2023

Changes

Submitter Checklist

Release Notes

abayer commented Jan 26, 2023

abayer commented Jan 26, 2023

abayer commented Jan 26, 2023

afrittoli Jan 26, 2023

Choose a reason for hiding this comment

abayer Jan 26, 2023

Choose a reason for hiding this comment

jerop Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

afrittoli Jan 27, 2023

Choose a reason for hiding this comment

afrittoli commented Jan 27, 2023

abayer commented Jan 27, 2023

jerop left a comment

Choose a reason for hiding this comment

jerop commented Jan 27, 2023

tekton-robot commented Jan 27, 2023

abayer commented Jan 27, 2023

abayer commented Jan 27, 2023 • edited Loading

abayer commented Jan 27, 2023

afrittoli commented Jan 27, 2023

jerop Jan 26, 2023 •

edited

Loading

abayer commented Jan 27, 2023 •

edited

Loading