From 017ba82b6fd500a32ddc56b81fdbaeebaf3a8615 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Mon, 9 Dec 2019 13:20:34 -0500 Subject: [PATCH] Address more review feedback --- pkg/template/event.go | 13 ++++++------- pkg/template/event_test.go | 9 +++++---- pkg/template/jsonpath.go | 9 ++------- pkg/template/jsonpath_test.go | 23 ++++++++++++++++++++--- pkg/template/resource.go | 9 ++++----- 5 files changed, 37 insertions(+), 26 deletions(-) diff --git a/pkg/template/event.go b/pkg/template/event.go index c1c0e5095..59b9f8321 100644 --- a/pkg/template/event.go +++ b/pkg/template/event.go @@ -18,12 +18,12 @@ 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 @@ -31,15 +31,15 @@ import ( 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} } diff --git a/pkg/template/event_test.go b/pkg/template/event_test.go index 2cd20a112..d1250199e 100644 --- a/pkg/template/event_test.go +++ b/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", diff --git a/pkg/template/jsonpath.go b/pkg/template/jsonpath.go index ce9244be0..057153843 100644 --- a/pkg/template/jsonpath.go +++ b/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) diff --git a/pkg/template/jsonpath_test.go b/pkg/template/jsonpath_test.go index 4a361419a..fb0563800 100644 --- a/pkg/template/jsonpath_test.go +++ b/pkg/template/jsonpath_test.go @@ -74,7 +74,7 @@ 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 { @@ -82,8 +82,8 @@ func TestParseJSONPath(t *testing.T) { 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) + } + }) + } +} diff --git a/pkg/template/resource.go b/pkg/template/resource.go index c47111b69..614887296 100644 --- a/pkg/template/resource.go +++ b/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,7 +49,7 @@ 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) } @@ -58,7 +57,7 @@ func ResolveTrigger(trigger triggersv1.EventListenerTrigger, getTB getTriggerBin 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) }