Parameter variable substitution support for conditionals #1143

dibyom · 2019-07-31T22:50:59Z

Changes

Add parameter variable substitution support for conditionals
A Condition's spec.Check can refer to its declared parameters using
the ${params.param-name} syntax just like in a Task. Values can be
provided via the pipelinerun's parameters or via default values in the
condition or in the PipelineTaskCondition

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Part of #1019, #1137

Release Notes

- Adds the ability to declare and use parameters in a Condition 
whose values can be defined in a pipelinerun.

tekton-robot · 2019-07-31T22:54:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.3%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	76.7%	80.0%	3.3
pkg/templating/container_replacements.go	Do not exist	100.0%
test/builder/pipeline.go	86.1%	86.7%	0.7

tekton-robot · 2019-07-31T23:28:37Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.3%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	76.7%	80.0%	3.3
pkg/templating/container_replacements.go	Do not exist	100.0%
test/builder/pipeline.go	86.1%	86.7%	0.7

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go

ghost · 2019-08-01T13:29:52Z

pkg/templating/container_replacements_test.go

+	"github.com/tektoncd/pipeline/pkg/templating"
+)
+
+func TestApplyContainerReplacements(t *testing.T) {


Thanks for improving the test coverage here!
Since you're at it 👼 it would be nice to test the case of replacement when the variable is not defined.

tekton-robot · 2019-08-01T15:30:10Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.3%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	76.7%	80.0%	3.3
pkg/templating/container_replacements.go	Do not exist	100.0%
test/builder/pipeline.go	86.1%	86.7%	0.7

docs/conditions.md

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go

tekton-robot · 2019-08-01T17:20:59Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.3%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	76.7%	80.0%	3.3
pkg/templating/container_replacements.go	Do not exist	100.0%
test/builder/pipeline.go	86.1%	86.7%	0.7

ghost · 2019-08-01T17:29:29Z

/lgtm

afrittoli · 2019-08-01T19:10:47Z

I've not reviewed the code yet, but I noticed that the params are limited to string - I wonder if we should support the newly added array params too? That could be a separate PR of course.

dibyom · 2019-08-01T20:24:28Z

I wonder if we should support the newly added array params too?

Array params should be supported for the args and command fields just like in a task. Could not think of a good use case for it for the examples at the moment, but I could add it in!

This makes it easier to reuse this functionality across taskruns and conditions. Signed-off-by: Dibyo Mukherjee <[email protected]>

A Condition's `spec.Check` can refer to its declared parameters using the ${params.param-name} syntax just like in a Task. Values can be provided via the pipelinerun's parameters or via default values in the condition or in the PipelineTaskCondition Signed-off-by: Dibyo Mukherjee <[email protected]>

tekton-robot · 2019-08-01T21:03:57Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	76.7%	80.0%	3.3
pkg/templating/container_replacements.go	Do not exist	100.0%
test/builder/pipeline.go	86.1%	86.7%	0.7

ghost · 2019-08-02T14:38:43Z

/lgtm

afrittoli

Thank you, nice work!!
I have a couple of minor comments, but nothing important so I'm going to approve this anyways.

afrittoli · 2019-08-02T15:00:02Z

docs/conditions.md

+parameter name, `barIsBa$` or `0banana` are not.
+
+Each declared parameter has a type field, assumed to be string if not provided by the user. 
+The other possible type is array — useful,checking a pushed branch name doesn't match any of 


NIT: space after the comma

afrittoli · 2019-08-02T15:00:03Z

docs/conditions.md

+parameter name, `barIsBa$` or `0banana` are not.
+
+Each declared parameter has a type field, assumed to be string if not provided by the user. 
+The other possible type is array — useful,checking a pushed branch name doesn't match any of 


NIT: space after the comma

afrittoli · 2019-08-02T15:06:01Z

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

 	}

 	return p
 }
+
+func replaceParamValues(params []v1alpha1.Param, stringReplacements map[string]string, arrayReplacements map[string][]string) []v1alpha1.Param {


afrittoli · 2019-08-02T16:38:51Z

pkg/reconciler/v1alpha1/pipelinerun/resources/apply_test.go

@@ -69,6 +70,49 @@ func TestApplyParameters(t *testing.T) {
 				tb.PipelineTask("first-task-1", "first-task",
 					tb.PipelineTaskParam("first-task-first-param", "${input.workspace.default-value}"),
 				))),
+	}, {
+		name: "single parameter in task condition",


NIT: I guess the test name refers to the amount of parameters passed in the run? One is left to default and one not. Perhaps the name could be changed?

afrittoli · 2019-08-02T16:39:46Z

pkg/reconciler/v1alpha1/pipelinerun/resources/apply_test.go

+					),
+				))),
+	}, {
+		name: "pipeline parameter nested inside condition parameter",


NIT: again, the name of the test is a bit confusing for me, in this case there is only one parameter and it's left to default.

afrittoli · 2019-08-02T20:16:08Z

pkg/templating/container_replacements_test.go

+	"github.com/tektoncd/pipeline/pkg/templating"
+)
+
+func TestApplyContainerReplacements(t *testing.T) {


Thanks for improving the test coverage here!
Since you're at it 👼 it would be nice to test the case of replacement when the variable is not defined.

afrittoli · 2019-08-02T20:18:34Z

/lgtm
/approve

tekton-robot · 2019-08-02T20:18:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dibyom

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~~ [afrittoli]

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

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2019

tekton-robot requested a review from afrittoli July 31, 2019 22:51

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 31, 2019

tekton-robot requested a review from bobcatfish July 31, 2019 22:51

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 31, 2019

dibyom force-pushed the cond-params branch from cabb5e0 to 670cd54 Compare July 31, 2019 23:26

ghost reviewed Aug 1, 2019

View reviewed changes

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Show resolved Hide resolved

ghost reviewed Aug 1, 2019

View reviewed changes

dibyom force-pushed the cond-params branch from 670cd54 to 49d13ee Compare August 1, 2019 15:27

dibyom changed the title ~~WIP: Parameter variable substitution support for conditionals~~ Parameter variable substitution support for conditionals Aug 1, 2019

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 Aug 1, 2019

ghost reviewed Aug 1, 2019

View reviewed changes

docs/conditions.md Outdated Show resolved Hide resolved

docs/conditions.md Outdated Show resolved Hide resolved

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go Outdated Show resolved Hide resolved

dibyom force-pushed the cond-params branch from 49d13ee to b6ac172 Compare August 1, 2019 17:18

tekton-robot assigned ghost Aug 1, 2019

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

dibyom added 2 commits August 1, 2019 16:54

Move container variable substitution to templating package

41f8b16

This makes it easier to reuse this functionality across taskruns and conditions. Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom force-pushed the cond-params branch from b6ac172 to baf31cf Compare August 1, 2019 21:01

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2019

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

afrittoli reviewed Aug 2, 2019

View reviewed changes

tekton-robot assigned afrittoli Aug 2, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019

tekton-robot merged commit 7b06d63 into tektoncd:master Aug 2, 2019

dibyom deleted the cond-params branch August 2, 2019 20:47

dibyom mentioned this pull request Aug 2, 2019

Minor docs fixes and extra templating test #1152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter variable substitution support for conditionals #1143

Parameter variable substitution support for conditionals #1143

dibyom commented Jul 31, 2019 •

edited

Loading

tekton-robot commented Jul 31, 2019

tekton-robot commented Jul 31, 2019

ghost Aug 1, 2019

afrittoli Aug 2, 2019

tekton-robot commented Aug 1, 2019

tekton-robot commented Aug 1, 2019

ghost commented Aug 1, 2019

afrittoli commented Aug 1, 2019

dibyom commented Aug 1, 2019

tekton-robot commented Aug 1, 2019

ghost commented Aug 2, 2019

afrittoli left a comment •

edited

Loading

afrittoli Aug 2, 2019

afrittoli Aug 2, 2019

afrittoli Aug 2, 2019

afrittoli Aug 2, 2019

afrittoli Aug 2, 2019

afrittoli Aug 2, 2019

afrittoli commented Aug 2, 2019

tekton-robot commented Aug 2, 2019

Parameter variable substitution support for conditionals #1143

Parameter variable substitution support for conditionals #1143

Conversation

dibyom commented Jul 31, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 31, 2019

tekton-robot commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Aug 1, 2019

tekton-robot commented Aug 1, 2019

ghost commented Aug 1, 2019

afrittoli commented Aug 1, 2019

dibyom commented Aug 1, 2019

tekton-robot commented Aug 1, 2019

ghost commented Aug 2, 2019

afrittoli left a comment • edited Loading

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

Choose a reason for hiding this comment

afrittoli commented Aug 2, 2019

tekton-robot commented Aug 2, 2019

dibyom commented Jul 31, 2019 •

edited

Loading

afrittoli left a comment •

edited

Loading