From d11c7bd4804a89850fa4902a2e5e65f8035fef2b Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 29 May 2020 20:32:09 +0530 Subject: [PATCH] Modify spec params to avoid confusion between resourcetemplates and triggertemplate params --- docs/triggertemplates.md | 10 ++-- examples/bitbucket/triggertemplate.yaml | 4 +- examples/github/triggertemplate.yaml | 2 +- examples/gitlab/gitlab-push-listener.yaml | 4 +- .../triggertemplates/triggertemplate.yaml | 4 +- .../v1alpha1/trigger_template_validation.go | 20 +++++-- .../trigger_template_validation_test.go | 40 ++++++++++++- pkg/sink/sink_test.go | 8 +-- pkg/template/event_test.go | 8 +-- pkg/template/resource.go | 17 ++++-- pkg/template/resource_test.go | 60 ++++++++++++++++--- test/eventlistener_test.go | 10 ++-- 12 files changed, 142 insertions(+), 45 deletions(-) diff --git a/docs/triggertemplates.md b/docs/triggertemplates.md index e36e5e132..9685f20d2 100644 --- 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 `` is the name of the parameter: ```YAML -$(params.) +$(tt.params.) ``` -`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 index a910ad380..4fa420338 100644 --- 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) \ No newline at end of file + value: $(tt.params.gitrepositoryurl) \ No newline at end of file diff --git a/examples/github/triggertemplate.yaml b/examples/github/triggertemplate.yaml index 6f1aa88ba..56c8bbc8b 100644 --- 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 index a179d875e..1705b6a19 100644 --- 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 index 47ebaeed4..92962cc94 100644 --- 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 index 765f26f3f..0698281fe 100644 --- 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[_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 index 7c4a80f89..dc9fff600 100644 --- 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 index 81e043b10..4f6a1b2a3 100644 --- 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 index 5e897684e..35e81689e 100644 --- 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 index 2f27dea27..8342fde06 100644 --- 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 index 1c191f948..57bd394f6 100644 --- 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 index fe98bcb46..ba5f6cbf0 100644 --- 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)"}, }, }, }