Address more review feedback

pkg/template/event.go

@@ -18,28 +18,28 @@ package template
 
 import (
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
-	"golang.org/x/xerrors"
 )
 
 // ResolveParams takes a given trigger binding and produces the resulting
 // resource params.
 func ResolveParams(bindings []*triggersv1.TriggerBinding, body []byte, header http.Header, params []pipelinev1.ParamSpec) ([]pipelinev1.Param, error) {
 	out, err := MergeBindingParams(bindings)
 	if err != nil {
-		return nil, xerrors.Errorf("error merging trigger params: %q", err)
+		return nil, fmt.Errorf("error merging trigger params: %w", err)
 	}
 	event, err := newEvent(body, header)
 	if err != nil {
-		return nil, xerrors.Errorf("failed to create Event: %q", err)
+		return nil, fmt.Errorf("failed to create Event: %w", err)
 	}
 	out, err = applyEventValuesToParams(out, event)
 	if err != nil {
-		return nil, xerrors.Errorf("failed to ApplyEventValuesToParams: %q", err)
+		return nil, fmt.Errorf("failed to ApplyEventValuesToParams: %w", err)
 	}
 	return MergeInDefaultParams(out, params), nil
 }
@@ -65,7 +65,7 @@ func newEvent(body []byte, headers http.Header) (*event, error) {
 	var data interface{}
 	if len(body) > 0 {
 		if err := json.Unmarshal(body, &data); err != nil {
-			return nil, xerrors.Errorf("failed to unmarshal request body: %q", err)
+			return nil, fmt.Errorf("failed to unmarshal request body: %w", err)
 		}
 	}
 	joinedHeaders := make(map[string]string, len(headers))
@@ -83,11 +83,10 @@ func newEvent(body []byte, headers http.Header) (*event, error) {
 // with values from the event body and headers.
 func applyEventValuesToParams(params []pipelinev1.Param, ec *event) ([]pipelinev1.Param, error) {
 	for idx, p := range params {
-		// TODO: We need tests for binding values that are not jsonpath expressions
 		if isTektonExpr(p.Value.StringVal) {
 			val, err := ParseJSONPath(ec, p.Value.StringVal)
 			if err != nil {
-				return nil, xerrors.Errorf("failed to replace JSONPath value for param %s: %s: %q", p.Name, p.Value, err)
+				return nil, fmt.Errorf("failed to replace JSONPath value for param %s: %s: %w", p.Name, p.Value, err)
 			}
 			params[idx].Value = pipelinev1.ArrayOrString{Type: pipelinev1.ParamTypeString, StringVal: val}
 		}

pkg/template/event_test.go

@@ -27,7 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/rand"
 )
 
-// TODO: 2. Split NewResourcesTests into separate tests for ResolveParams and ResolveResources
+// TODO(#252): Split NewResourcesTests into separate tests for ResolveParams and ResolveResources
 func Test_NewResources(t *testing.T) {
 	type args struct {
 		body    []byte
@@ -211,25 +211,26 @@ func Test_NewResources(t *testing.T) {
 			json.RawMessage(`{"rt1": "bar-1"}`),
 		},
 	}, {
-		name: "bindings with default values",
+		name: "bindings with static values",
 		args: args{
 			body: json.RawMessage(`{"foo": "bar"}`),
 			binding: ResolvedTrigger{
 				TriggerTemplate: bldr.TriggerTemplate("tt", "ns", bldr.TriggerTemplateSpec(
-					bldr.TriggerTemplateParam("p1", "", "defaultVal"),
+					bldr.TriggerTemplateParam("p1", "", ""),
 					bldr.TriggerTemplateParam("p2", "", ""),
 					bldr.TriggerResourceTemplate(json.RawMessage(`{"p1": "$(params.p1)", "p2": "$(params.p2)"}`)),
 				),
 				),
 				TriggerBindings: []*triggersv1.TriggerBinding{
 					bldr.TriggerBinding("tb", "ns", bldr.TriggerBindingSpec(
+						bldr.TriggerBindingParam("p1", "static_value"),
 						bldr.TriggerBindingParam("p2", "$(body.foo)"),
 					)),
 				},
 			},
 		},
 		want: []json.RawMessage{
-			json.RawMessage(`{"p1": "defaultVal", "p2": "bar"}`),
+			json.RawMessage(`{"p1": "static_value", "p2": "bar"}`),
 		},
 	}, {
 		name: "event value is JSON string",

pkg/template/jsonpath.go

@@ -19,7 +19,7 @@ var (
 
 	// jsonRegexp is a regular expression for JSONPath expressions
 	// with or without the enclosing {} and the leading . inside the curly
-	// braces e.g.  'a.b' or '.a.b' or '{a.b}' or '{.a.b}
+	// braces e.g.  'a.b' or '.a.b' or '{a.b}' or '{.a.b}'
 	jsonRegexp = regexp.MustCompile(`^\{\.?([^{}]+)\}$|^\.?([^{}]+)$`)
 )
 
@@ -115,7 +115,7 @@ func TektonJSONPathExpression(expr string) (string, error) {
 	if !isTektonExpr(expr) {
 		return "", errors.New("Expression not wrapped in $()")
 	}
-	return relaxedJSONPathExpression(unwrapTektonExpr(expr))
+	return relaxedJSONPathExpression(tektonVar.ReplaceAllString(expr, "$1"))
 }
 
 // RelaxedJSONPathExpression attempts to be flexible with JSONPath expressions, it accepts:
@@ -147,11 +147,6 @@ func relaxedJSONPathExpression(pathExpression string) (string, error) {
 	return fmt.Sprintf("{.%s}", fieldSpec), nil
 }
 
-// unwrapTektonExpr takes an expression of the form $(abc) and returns abc.
-func unwrapTektonExpr(expr string) string {
-	return tektonVar.ReplaceAllString(expr, "$1")
-}
-
 // IsTektonExpr returns true if the expr is wrapped in $()
 func isTektonExpr(expr string) bool {
 	return tektonVar.MatchString(expr)

pkg/template/jsonpath_test.go

@@ -74,16 +74,16 @@ func TestParseJSONPath(t *testing.T) {
 			var data interface{}
 			err := json.Unmarshal([]byte(tt.in), &data)
 			if err != nil {
-				t.Errorf("Could not unmarshall body : %q", err)
+				t.Fatalf("Could not unmarshall body : %q", err)
 			}
 			got, err := template.ParseJSONPath(data, tt.expr)
 			if err != nil {
 				t.Errorf("ParseJSONPath() error = %v", err)
 				return
 			}
 
-			if diff := cmp.Diff(got, strings.Replace(tt.want, " ", "", -1)); diff != "" {
-				t.Errorf("ParseJSONPath() -got,+want: %s", diff)
+			if diff := cmp.Diff(strings.Replace(tt.want, " ", "", -1), got); diff != "" {
+				t.Errorf("ParseJSONPath() -want,+got: %s", diff)
 			}
 		})
 	}
@@ -143,3 +143,20 @@ func TestTektonJSONPathExpression(t *testing.T) {
 		})
 	}
 }
+
+func TestTektonJSONPathExpression_Error(t *testing.T) {
+	tests := []string{
+		"{.metadata.name}", // not wrapped in $()
+		"",
+		"$({asd)",
+		"$({)",
+	}
+	for _, expr := range tests {
+		t.Run(expr, func(t *testing.T) {
+			_, err := template.TektonJSONPathExpression(expr)
+			if err == nil {
+				t.Errorf("TektonJSONPathExpression() did not get expected error for expression = %s", expr)
+			}
+		})
+	}
+}

pkg/template/resource.go

@@ -24,7 +24,6 @@ import (
 
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
-	"golang.org/x/xerrors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/apimachinery/pkg/util/rand"
@@ -50,15 +49,15 @@ func ResolveTrigger(trigger triggersv1.EventListenerTrigger, getTB getTriggerBin
 	for _, b := range trigger.Bindings {
 		tb2, err := getTB(b.Name, metav1.GetOptions{})
 		if err != nil {
-			return ResolvedTrigger{}, xerrors.Errorf("error getting TriggerBinding %s: %s", b.Name, err)
+			return ResolvedTrigger{}, fmt.Errorf("error getting TriggerBinding %s: %w", b.Name, err)
 		}
 		tb = append(tb, tb2)
 	}
 
 	ttName := trigger.Template.Name
 	tt, err := getTT(ttName, metav1.GetOptions{})
 	if err != nil {
-		return ResolvedTrigger{}, xerrors.Errorf("Error getting TriggerTemplate %s: %s", ttName, err)
+		return ResolvedTrigger{}, fmt.Errorf("error getting TriggerTemplate %s: %w", ttName, err)
 	}
 	return ResolvedTrigger{TriggerBindings: tb, TriggerTemplate: tt}, nil
 }
@@ -93,8 +92,8 @@ func ApplyParamsToResourceTemplate(params []pipelinev1.Param, rt json.RawMessage
 func applyParamToResourceTemplate(param pipelinev1.Param, rt json.RawMessage) json.RawMessage {
 	// Assume the param is valid
 	paramVariable := fmt.Sprintf("$(params.%s)", param.Name)
-	// Escape quotes for backwards compatibility. Needed since we are appending JSON
-	//
+	// 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.StringVal, `"`, `\"`, -1)
 	return bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1)
 }