Drop escaping of strings in the JSON. #823

bigkevmcd · 2020-11-05T14:07:34Z

Changes

Add support for the old escaping mechanism through an annotation on the
TriggerTemplate.

This removes the old replacement of " with \" in parameters, which was yielding
invalid JSON when the strings were already quoted.

Fixes: #777

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
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Change the escaping of parameters into TriggerTemplates.

Previously, parameters were escaped as they were being replaced into a TriggerTemplate, by replacing double-quotes " with an escaped version \", this functionality has been removed, as it was breaking quoted strings, and in some cases, rendering the resulting output unparseable.

action required: If you were relying on the escaping, you can retain the old behaviour by adding an annotation to an affected TriggerTemplate, `tekton.dev/old-escape-quotes: true"`

bigkevmcd · 2020-11-05T16:39:49Z

/test pull-tekton-triggers-integration-tests

bigkevmcd · 2020-11-06T09:11:43Z

/retest

dibyom · 2020-11-10T16:46:43Z

/test pull-tekton-triggers-integration-tests

dibyom · 2020-11-11T15:52:23Z

        {"level":"error","ts":"2020-11-10T16:58:02.248Z","logger":"eventlistener","caller":"sink/sink.go:274","msg":"problem creating obj: &errors.errorString{s:\"couldn't unmarshal json: invalid character 'k' after object key:value pair\"}","knative.dev/controller":"eventlistener","/triggers-eventid":"742ms","/trigger":"","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.CreateResources\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:274\ngithub.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:191\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:101"}
        {"level":"error","ts":"2020-11-10T16:58:02.248Z","logger":"eventlistener","caller":"sink/sink.go:192","msg":"couldn't unmarshal json: invalid character 'k' after object key:value pair","knative.dev/controller":"eventlistener","/triggers-eventid":"742ms","/trigger":"","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:192\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:101"}

wlynch

Overall looks good, just some small tweaks.

/lgtm

pkg/template/event.go

wlynch · 2020-11-11T18:31:37Z

pkg/template/event.go

 	triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1"
 )

+const (
+	OldEscapeAnnotation = "tekton.dev/old-escape-quotes"


Add a comment explaining what this controls, when it can be removed, etc.

wlynch · 2020-11-11T18:33:35Z

docs/triggertemplates.md

@@ -120,3 +120,28 @@ As of Tekton Pipelines version
 embed resource specs. It is a best practice to embed each resource specs in the
 PipelineRun or TaskRun that uses the resource spec. Embedding the resource spec
 avoids a race condition between creating and using resources.
+
+
+## Quote-Escaping


This primarily focuses on what the old behavior was, but doesn't really explain what the current behavior is. Let's add some more details here.

I'm not sure what to put here, the strings are no longer escaped at all, which means that it's up to you to put the correct quoting around params if you expect the values to be quoted or not.

One thing that could be useful is an example of what a templated JSON body would look like now and with the annotation. something like- previously or with the annotation {\"foo\": \"bar\"} and while now it will be {"foo": "bar"}

test/eventlistener_test.go

dibyom · 2020-11-17T16:40:35Z

Looks good. One question -- if I am using a TriggerTemplate that is embedded inside a Trigger resource, can I still use the annotation on the Trigger resource itself or does this only apply TriggerTemplates used via reference?

Add support for the old escaping mechanism through an annotation on the TriggerTemplate. This removes the old replacement of " with \" in parameters, which was yielding invalid JSON when the strings were already quoted.

tekton-robot · 2020-11-19T09:29:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/resource.go	98.3%	98.3%	0.0

dibyom · 2020-11-19T19:04:30Z

can I still use the annotation on the Trigger resource itself or does this only apply TriggerTemplates used via reference?

It doesn't work for embedded trigger templates yet but at the WG we decided this would be fine to start off with. We can add the annotation support for embedded templates if users ask for it.

dibyom · 2020-11-19T19:11:13Z

/approve
/lgtm

tekton-robot · 2020-11-19T19:11:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [dibyom]

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 release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Nov 5, 2020

tekton-robot requested review from ncskier and wlynch November 5, 2020 14:07

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2020

bigkevmcd force-pushed the fix-trigger-templates branch 2 times, most recently from 390d2e6 to d6fab86 Compare November 6, 2020 12:16

bigkevmcd force-pushed the fix-trigger-templates branch 3 times, most recently from 3e8c705 to 2ddda3d Compare November 11, 2020 17:36

wlynch reviewed Nov 11, 2020

View reviewed changes

tekton-robot assigned wlynch Nov 11, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020

bigkevmcd force-pushed the fix-trigger-templates branch from 2ddda3d to b2339ed Compare November 16, 2020 08:47

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2020

bigkevmcd force-pushed the fix-trigger-templates branch 3 times, most recently from 7be544b to edd3154 Compare November 16, 2020 10:42

Drop escaping of strings in the JSON.

8b5988b

Add support for the old escaping mechanism through an annotation on the TriggerTemplate. This removes the old replacement of " with \" in parameters, which was yielding invalid JSON when the strings were already quoted.

bigkevmcd force-pushed the fix-trigger-templates branch from edd3154 to 8b5988b Compare November 19, 2020 09:27

tekton-robot assigned dibyom Nov 19, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2020

tekton-robot merged commit 0cf3804 into tektoncd:master Nov 19, 2020

dibyom mentioned this pull request Dec 9, 2020

TriggerBinding doesn't support multiline string param? #772

Closed

bigkevmcd mentioned this pull request Feb 2, 2021

Escaping quotes in TriggerTemplates. tektoncd/community#321

Closed

dibyom mentioned this pull request Jun 15, 2021

Resolving newline in TriggerBinding param #953

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop escaping of strings in the JSON. #823

Drop escaping of strings in the JSON. #823

bigkevmcd commented Nov 5, 2020 •

edited

Loading

bigkevmcd commented Nov 5, 2020

bigkevmcd commented Nov 6, 2020

dibyom commented Nov 10, 2020

dibyom commented Nov 11, 2020

wlynch left a comment

wlynch Nov 11, 2020

wlynch Nov 11, 2020

bigkevmcd Nov 16, 2020

dibyom Nov 17, 2020

dibyom commented Nov 17, 2020

tekton-robot commented Nov 19, 2020

dibyom commented Nov 19, 2020

dibyom commented Nov 19, 2020

tekton-robot commented Nov 19, 2020

Drop escaping of strings in the JSON. #823

Drop escaping of strings in the JSON. #823

Conversation

bigkevmcd commented Nov 5, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

bigkevmcd commented Nov 5, 2020

bigkevmcd commented Nov 6, 2020

dibyom commented Nov 10, 2020

dibyom commented Nov 11, 2020

wlynch left a comment

Choose a reason for hiding this comment

wlynch Nov 11, 2020

Choose a reason for hiding this comment

wlynch Nov 11, 2020

Choose a reason for hiding this comment

bigkevmcd Nov 16, 2020

Choose a reason for hiding this comment

dibyom Nov 17, 2020

Choose a reason for hiding this comment

dibyom commented Nov 17, 2020

tekton-robot commented Nov 19, 2020

dibyom commented Nov 19, 2020

dibyom commented Nov 19, 2020

tekton-robot commented Nov 19, 2020

bigkevmcd commented Nov 5, 2020 •

edited

Loading