Modify spec params to avoid confusion between resourcetemplates and t…

…riggertemplate params
tektoncd · Jun 24, 2020 · d11c7bd · d11c7bd
1 parent 7b586ad
commit d11c7bd
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 45 deletions.
diff --git a/docs/triggertemplates.md b/docs/triggertemplates.md
@@ -47,9 +47,9 @@ spec:
           type: git
           params:
           - name: revision
-            value: $(params.gitrevision)
+            value: $(tt.params.gitrevision)
           - name: url
-            value: $(params.gitrepositoryurl)
+            value: $(tt.params.gitrepositoryurl)
 ```
 
 `TriggerTemplates` currently support the following [Tekton Pipelines](https://github.com/tektoncd/pipelines) resources:
@@ -102,11 +102,11 @@ have an optional `description` and `default` value.
 substitution syntax, where `<name>` is the name of the parameter:
 
 ```YAML
-$(params.<name>)
+$(tt.params.<name>)
 ```
 
-`params` can be referenced in the `resourceTemplates` section of a
-`TriggerTemplate`. The purpose of `params` is to make `TriggerTemplates`
+`tt.params` can be referenced in the `resourceTemplates` section of a
+`TriggerTemplate`. The purpose of `tt.params` is to make `TriggerTemplates`
 reusable.
 
 ## Best Practices

diff --git a/examples/bitbucket/triggertemplate.yaml b/examples/bitbucket/triggertemplate.yaml
@@ -30,6 +30,6 @@ spec:
                 type: git
                 params:
                   - name: revision
-                    value: $(params.gitrevision)
+                    value: $(tt.params.gitrevision)
                   - name: url
-                    value: $(params.gitrepositoryurl)
+                    value: $(tt.params.gitrepositoryurl)
diff --git a/examples/github/triggertemplate.yaml b/examples/github/triggertemplate.yaml
@@ -32,4 +32,4 @@ spec:
                   - name: revision
                     value: $(params.gitrevision)
                   - name: url
-                    value: $(params.gitrepositoryurl)
+                    value: $(tt.params.gitrepositoryurl)
diff --git a/examples/gitlab/gitlab-push-listener.yaml b/examples/gitlab/gitlab-push-listener.yaml
@@ -29,9 +29,9 @@ spec:
                 type: git
                 params:
                   - name: revision
-                    value: $(params.gitrevision)
+                    value: $(tt.params.gitrevision)
                   - name: url
-                    value: $(params.gitrepositoryurl)
+                    value: $(tt.params.gitrepositoryurl)
 ---
 apiVersion: triggers.tekton.dev/v1alpha1
 kind: TriggerBinding

diff --git a/examples/triggertemplates/triggertemplate.yaml b/examples/triggertemplates/triggertemplate.yaml
@@ -33,6 +33,6 @@ spec:
           type: git
           params:
           - name: revision
-            value: $(params.gitrevision)
+            value: $(tt.params.gitrevision)
           - name: url
-            value: $(params.gitrepositoryurl)
+            value: $(tt.params.gitrepositoryurl)
diff --git a/pkg/apis/triggers/v1alpha1/trigger_template_validation.go b/pkg/apis/triggers/v1alpha1/trigger_template_validation.go
@@ -28,8 +28,8 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-// paramsRegexp captures TriggerTemplate parameter names $(params.NAME)
-var paramsRegexp = regexp.MustCompile(`\$\(params.(?P<var>[_a-zA-Z][_a-zA-Z0-9.-]*)\)`)
+// paramsRegexp captures TriggerTemplate parameter names $(tt.params.NAME) or $(params.NAME)
+var paramsRegexp = regexp.MustCompile(`\$\((params|tt.params).([_a-zA-Z][_a-zA-Z0-9.-]*)\)`)
 
 // Validate validates a TriggerTemplate.
 func (t *TriggerTemplate) Validate(ctx context.Context) *apis.FieldError {
@@ -91,16 +91,24 @@ func verifyParamDeclarations(params []ParamSpec, templates []TriggerResourceTemp
 		declaredParamNames[param.Name] = struct{}{}
 	}
 	for i, template := range templates {
-		// Get all params in the template $(params.NAME)
+		// Get all params in the template $(tt.params.NAME) or $(params.NAME)
 		templateParams := paramsRegexp.FindAllSubmatch(template.RawExtension.Raw, -1)
 		for _, templateParam := range templateParams {
-			templateParamName := string(templateParam[1])
+			templateParamName := string(templateParam[2])
 			if _, ok := declaredParamNames[templateParamName]; !ok {
+				// This logic is to get the tag and display error dynamically for both tt.params and params.
+				// TODO(#606)
+				var tag string
+				if string(templateParam[1]) == "params" {
+					tag = "params"
+				} else {
+					tag = "tt.params"
+				}
 				fieldErr := apis.ErrInvalidValue(
-					fmt.Sprintf("undeclared param '$(params.%s)'", templateParamName),
+					fmt.Sprintf("undeclared param '$(%s.%s)'", tag, templateParamName),
 					fmt.Sprintf("[%d]", i),
 				)
-				fieldErr.Details = fmt.Sprintf("'$(params.%s)' must be declared in spec.params", templateParamName)
+				fieldErr.Details = fmt.Sprintf("'$(%s.%s)' must be declared in spec.params", tag, templateParamName)
 				return fieldErr
 			}
 		}

diff --git a/pkg/apis/triggers/v1alpha1/trigger_template_validation_test.go b/pkg/apis/triggers/v1alpha1/trigger_template_validation_test.go
@@ -36,8 +36,17 @@ var v1beta1ResourceTemplate = runtime.RawExtension{
 	Raw: []byte(`{"kind":"PipelineRun","apiVersion":"tekton.dev/v1beta1","metadata":{"creationTimestamp":null},"spec":{},"status":{}}`),
 }
 var paramResourceTemplate = runtime.RawExtension{
+	Raw: []byte(`{"kind":"PipelineRun","apiVersion":"tekton.dev/v1alpha1","metadata":{"creationTimestamp":null},"spec": "$(tt.params.foo)","status":{}}`),
+}
+var deprecatedParamResourceTemplate = runtime.RawExtension{
 	Raw: []byte(`{"kind":"PipelineRun","apiVersion":"tekton.dev/v1alpha1","metadata":{"creationTimestamp":null},"spec": "$(params.foo)","status":{}}`),
 }
+var invalidParamResourceTemplate = runtime.RawExtension{
+	Raw: []byte(`{"kind":"PipelineRun","apiVersion":"tekton.dev/v1alpha1","metadata":{"creationTimestamp":null},"spec": "$(.foo)","status":{}}`),
+}
+var bothParamResourceTemplate = runtime.RawExtension{
+	Raw: []byte(`{"kind":"PipelineRun","apiVersion":"tekton.dev/v1alpha1","metadata":{"creationTimestamp":null},"spec": {"$(params1.foo)", "$(params.bar)", "$(tt.params.baz)"},"status":{}}`),
+}
 
 func TestTriggerTemplate_Validate(t *testing.T) {
 	tcs := []struct {
@@ -130,14 +139,43 @@ func TestTriggerTemplate_Validate(t *testing.T) {
 				b.TriggerResourceTemplate(paramResourceTemplate))),
 			want: nil,
 		}, {
-			name: "params used in resource template are not declared",
+			name: "tt.params used in resource template are not declared",
 			template: b.TriggerTemplate("tt", "foo", b.TriggerTemplateSpec(
 				b.TriggerResourceTemplate(paramResourceTemplate))),
+			want: &apis.FieldError{
+				Message: "invalid value: undeclared param '$(tt.params.foo)'",
+				Paths:   []string{"spec.resourcetemplates[0]"},
+				Details: "'$(tt.params.foo)' must be declared in spec.params",
+			},
+		}, {
+			name: "params used in resource template are not declared",
+			template: b.TriggerTemplate("tt", "foo", b.TriggerTemplateSpec(
+				b.TriggerResourceTemplate(deprecatedParamResourceTemplate))),
 			want: &apis.FieldError{
 				Message: "invalid value: undeclared param '$(params.foo)'",
 				Paths:   []string{"spec.resourcetemplates[0]"},
 				Details: "'$(params.foo)' must be declared in spec.params",
 			},
+		}, {
+			name: "both params and tt.params used in resource template are not declared",
+			template: b.TriggerTemplate("tt", "foo", b.TriggerTemplateSpec(
+				b.TriggerResourceTemplate(bothParamResourceTemplate))),
+			want: &apis.FieldError{
+				Message: "invalid value: undeclared param '$(params.bar)'",
+				Paths:   []string{"spec.resourcetemplates[0]"},
+				Details: "'$(params.bar)' must be declared in spec.params",
+			},
+		}, {
+			name: "invalid params used in resource template are not declared",
+			template: b.TriggerTemplate("tt", "foo", b.TriggerTemplateSpec(
+				b.TriggerResourceTemplate(invalidParamResourceTemplate))),
+			want: nil,
+		}, {
+			name: "invalid params used in resource template are declared",
+			template: b.TriggerTemplate("tt", "foo", b.TriggerTemplateSpec(
+				b.TriggerTemplateParam("foo", "desc", "val"),
+				b.TriggerResourceTemplate(invalidParamResourceTemplate))),
+			want: nil,
 		}}
 
 	for _, tc := range tcs {

diff --git a/pkg/sink/sink_test.go b/pkg/sink/sink_test.go
@@ -153,7 +153,7 @@ func TestHandleEvent(t *testing.T) {
 			Kind:       "PipelineResource",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "$(params.name)",
+			Name:      "$(tt.params.name)",
 			Namespace: namespace,
 			Labels: map[string]string{
 				"app":  "$(params.foo)",
@@ -273,7 +273,7 @@ func TestHandleEventWithInterceptors(t *testing.T) {
 			Type: pipelinev1alpha1.PipelineResourceTypeGit,
 			Params: []pipelinev1alpha1.ResourceParam{{
 				Name:  "url",
-				Value: "$(params.url)",
+				Value: "$(tt.params.url)",
 			}},
 		},
 	}
@@ -413,7 +413,7 @@ func TestHandleEventWithWebhookInterceptors(t *testing.T) {
 			Kind:       "PipelineResource",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "$(params.name)",
+			Name:      "$(tt.params.name)",
 			Namespace: namespace,
 		},
 		Spec: pipelinev1alpha1.PipelineResourceSpec{
@@ -1001,7 +1001,7 @@ func getResources(t *testing.T, triggerBindingParam string) (*v1alpha1.TriggerBi
 			Type: pipelinev1.PipelineResourceTypeGit,
 			Params: []pipelinev1.ResourceParam{{
 				Name:  "url",
-				Value: "$(params.url)",
+				Value: "$(tt.params.url)",
 			}},
 		},
 	}

diff --git a/pkg/template/event_test.go b/pkg/template/event_test.go
@@ -395,8 +395,8 @@ func TestResolveResources(t *testing.T) {
 		template: bldr.TriggerTemplate("tt", ns, bldr.TriggerTemplateSpec(
 			bldr.TriggerTemplateParam("p1", "desc", ""),
 			bldr.TriggerTemplateParam("p2", "desc", ""),
-			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(params.p1)-$(params.p2)"}`)}),
-			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt2": "$(params.p1)-$(params.p2)"}`)}),
+			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(tt.params.p1)-$(tt.params.p2)"}`)}),
+			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt2": "$(tt.params.p1)-$(tt.params.p2)"}`)}),
 		)),
 		params: []triggersv1.Param{
 			bldr.Param("p1", "val1"),
@@ -410,7 +410,7 @@ func TestResolveResources(t *testing.T) {
 		name: "replace JSON string in templates",
 		template: bldr.TriggerTemplate("tt", ns, bldr.TriggerTemplateSpec(
 			bldr.TriggerTemplateParam("p1", "desc", ""),
-			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(params.p1)"}`)}),
+			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(tt.params.p1)"}`)}),
 		)),
 		params: []triggersv1.Param{
 			bldr.Param("p1", `{"a": "b"}`),
@@ -423,7 +423,7 @@ func TestResolveResources(t *testing.T) {
 		name: "replace JSON string with special chars in templates",
 		template: bldr.TriggerTemplate("tt", ns, bldr.TriggerTemplateSpec(
 			bldr.TriggerTemplateParam("p1", "desc", ""),
-			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(params.p1)"}`)}),
+			bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: []byte(`{"rt1": "$(tt.params.p1)"}`)}),
 		)),
 		params: []triggersv1.Param{
 			bldr.Param("p1", `{"a": "v\\r\\n烈"}`),

diff --git a/pkg/template/resource.go b/pkg/template/resource.go
@@ -110,12 +110,17 @@ func ApplyParamsToResourceTemplate(params []triggersv1.Param, rt json.RawMessage
 // applyParamToResourceTemplate returns the TriggerResourceTemplate with the
 // param value substituted for all matching param variables in the template
 func applyParamToResourceTemplate(param triggersv1.Param, rt json.RawMessage) json.RawMessage {
-	// Assume the param is valid
-	paramVariable := fmt.Sprintf("$(params.%s)", param.Name)
-	// Escape quotes so that that JSON strings can be appended to regular strings.
-	// See #257 for discussion on this behavior.
-	paramValue := strings.Replace(param.Value, `"`, `\"`, -1)
-	return bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1)
+	// The changes are for backward compatibility with both $(params) and $(tt.params)
+	// TODO(#606)
+	for _, tag := range []string{"params", "tt.params"} {
+		// Assume the param is valid
+		paramVariable := fmt.Sprintf("$(%s.%s)", tag, param.Name)
+		// Escape quotes so that that JSON strings can be appended to regular strings.
+		// See #257 for discussion on this behavior.
+		paramValue := strings.Replace(param.Value, `"`, `\"`, -1)
+		rt = bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1)
+	}
+	return rt
 }
 
 // UID generates a random string like the Kubernetes apiserver generateName metafield postfix.

diff --git a/pkg/template/resource_test.go b/pkg/template/resource_test.go
@@ -126,11 +126,11 @@ func Test_applyParamToResourceTemplate(t *testing.T) {
 		}
 		rtNoParamVars             = json.RawMessage(`{"foo": "bar"}`)
 		wantRtNoParamVars         = json.RawMessage(`{"foo": "bar"}`)
-		rtNoMatchingParamVars     = json.RawMessage(`{"foo": "$(params.no.matching.path)"}`)
-		wantRtNoMatchingParamVars = json.RawMessage(`{"foo": "$(params.no.matching.path)"}`)
-		rtOneParamVar             = json.RawMessage(`{"foo": "bar-$(params.oneid)-bar"}`)
+		rtNoMatchingParamVars     = json.RawMessage(`{"foo": "$(tt.params.no.matching.path)"}`)
+		wantRtNoMatchingParamVars = json.RawMessage(`{"foo": "$(tt.params.no.matching.path)"}`)
+		rtOneParamVar             = json.RawMessage(`{"foo": "bar-$(tt.params.oneid)-bar"}`)
 		wantRtOneParamVar         = json.RawMessage(`{"foo": "bar-onevalue-bar"}`)
-		rtMultipleParamVars       = json.RawMessage(`{"$(params.oneid)": "bar-$(params.oneid)-$(params.oneid)$(params.oneid)$(params.oneid)-$(params.oneid)-bar"}`)
+		rtMultipleParamVars       = json.RawMessage(`{"$(tt.params.oneid)": "bar-$(tt.params.oneid)-$(tt.params.oneid)$(tt.params.oneid)$(tt.params.oneid)-$(tt.params.oneid)-bar"}`)
 		wantRtMultipleParamVars   = json.RawMessage(`{"onevalue": "bar-onevalue-onevalueonevalueonevalue-onevalue-bar"}`)
 	)
 	type args struct {
@@ -180,9 +180,19 @@ func Test_applyParamToResourceTemplate(t *testing.T) {
 					Name:  "p1",
 					Value: `{"a":"b"}`,
 				},
-				rt: json.RawMessage(`{"foo": "$(params.p1)"}`),
+				rt: json.RawMessage(`{"foo": "$(tt.params.p1)"}`),
 			},
 			want: json.RawMessage(`{"foo": "{\"a\":\"b\"}"}`),
+		}, {
+			name: "deprecated params in resourcetemplate",
+			args: args{
+				param: triggersv1.Param{
+					Name:  "p1",
+					Value: `{"a":"b"}`,
+				},
+				rt: json.RawMessage(`{"p1": "$(params.p1)"}`),
+			},
+			want: json.RawMessage(`{"p1": "{\"a\":\"b\"}"}`),
 		},
 	}
 	for _, tt := range tests {
@@ -196,7 +206,10 @@ func Test_applyParamToResourceTemplate(t *testing.T) {
 }
 
 func Test_ApplyParamsToResourceTemplate(t *testing.T) {
-	rt := json.RawMessage(`{"oneparam": "$(params.oneid)", "twoparam": "$(params.twoid)", "threeparam": "$(params.threeid)"`)
+	rt := json.RawMessage(`{"oneparam": "$(tt.params.oneid)", "twoparam": "$(tt.params.twoid)", "threeparam": "$(tt.params.threeid)"`)
+	rt1 := json.RawMessage(`{"deprecatedParam": "$(params.oneid)"`)
+	rt2 := json.RawMessage(`{"actualParam": "$(tt.params.oneid)", "deprecatedParam": "$(params.twoid)"`)
+	rt3 := json.RawMessage(`{"actualParam": "$(tt.params.oneid)", "invalidParam": "$(tt.params1.invalidid)", "deprecatedParam": "$(params.twoid)"`)
 	type args struct {
 		params []triggersv1.Param
 		rt     json.RawMessage
@@ -222,7 +235,7 @@ func Test_ApplyParamsToResourceTemplate(t *testing.T) {
 				},
 				rt: rt,
 			},
-			want: json.RawMessage(`{"oneparam": "onevalue", "twoparam": "$(params.twoid)", "threeparam": "$(params.threeid)"`),
+			want: json.RawMessage(`{"oneparam": "onevalue", "twoparam": "$(tt.params.twoid)", "threeparam": "$(tt.params.threeid)"`),
 		},
 		{
 			name: "multiple params",
@@ -236,6 +249,39 @@ func Test_ApplyParamsToResourceTemplate(t *testing.T) {
 			},
 			want: json.RawMessage(`{"oneparam": "onevalue", "twoparam": "twovalue", "threeparam": "threevalue"`),
 		},
+		{
+			name: "deprecated params",
+			args: args{
+				params: []triggersv1.Param{
+					{Name: "oneid", Value: "deprecatedParamValue"},
+				},
+				rt: rt1,
+			},
+			want: json.RawMessage(`{"deprecatedParam": "deprecatedParamValue"`),
+		},
+		{
+			name: "both params and tt.params together",
+			args: args{
+				params: []triggersv1.Param{
+					{Name: "oneid", Value: "actualValue"},
+					{Name: "twoid", Value: "deprecatedParamValue"},
+				},
+				rt: rt2,
+			},
+			want: json.RawMessage(`{"actualParam": "actualValue", "deprecatedParam": "deprecatedParamValue"`),
+		},
+		{
+			name: "valid and invalid params together",
+			args: args{
+				params: []triggersv1.Param{
+					{Name: "oneid", Value: "actualValue"},
+					{Name: "invalidid", Value: "invalidValue"},
+					{Name: "twoid", Value: "deprecatedParamValue"},
+				},
+				rt: rt3,
+			},
+			want: json.RawMessage(`{"actualParam": "actualValue", "invalidParam": "$(tt.params1.invalidid)", "deprecatedParam": "deprecatedParamValue"`),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/test/eventlistener_test.go b/test/eventlistener_test.go
@@ -85,7 +85,7 @@ func TestEventListenerCreate(t *testing.T) {
 			Name:      "pr1",
 			Namespace: namespace,
 			Labels: map[string]string{
-				"$(params.oneparam)": "$(params.oneparam)",
+				"$(tt.params.oneparam)": "$(tt.params.oneparam)",
 			},
 		},
 		Spec: v1alpha1.PipelineResourceSpec{
@@ -107,15 +107,15 @@ func TestEventListenerCreate(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "pr2",
 			Labels: map[string]string{
-				"$(params.twoparamname)": "$(params.twoparamvalue)",
+				"$(tt.params.twoparamname)": "$(tt.params.twoparamvalue)",
 			},
 		},
 		Spec: v1alpha1.PipelineResourceSpec{
 			Type: "git",
 			Params: []v1alpha1.ResourceParam{
-				{Name: "license", Value: "$(params.license)"},
-				{Name: "header", Value: "$(params.header)"},
-				{Name: "prmessage", Value: "$(params.prmessage)"},
+				{Name: "license", Value: "$(tt.params.license)"},
+				{Name: "header", Value: "$(tt.params.header)"},
+				{Name: "prmessage", Value: "$(tt.params.prmessage)"},
 			},
 		},
 	}