migrate to cloudevents sdk v2 #2243

n3wscott · 2020-03-18T16:29:52Z

While I was here, I also refactored some of the CloudEvents integration, some major points:

you now send spec version 1.0 formatted events
ID is not the task run name, this was a bad idea if you want to send more than one event for that run. The task run name is now used as the subject.
ID is now a UUID.
The event that is created for sending is only created once, and then used for all deliveries. This means that all subscribers get the same event content and ID.
The event payload is marshaled inside the Event object. I show examples in the code and tests of how to set and get the payload as your custom struct.
Removed a lot of the layers passing clients and event components around. Better to build the event and then call send on the client directly. This deleted a lot of code that was doing many things, like create an event, and then attempt to send it.

I think the integration tests had been failing (will confirm in moments) because in the new SDK if you set data as a []byte, this tells us you want to base64 encode the data out. So the receiver was getting base64 encoded data and not understanding it.

tekton-robot · 2020-03-18T16:30:07Z

Hi @n3wscott. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dibyom · 2020-03-18T16:47:30Z

/ok-to-test

n3wscott · 2020-03-18T19:45:40Z

/retest

mattmoor · 2020-03-18T21:04:19Z

/retest

n3wscott · 2020-03-18T21:34:54Z

The errors do not look related to my change...

/retest

n3wscott · 2020-03-19T01:54:05Z

hm


Result | FAILURE
-- | --
Tests | 0 failed / 44 succeeded
Started | 2020-03-18 17:49 PDT
Elapsed | 48m18s

cool....

/retest

n3wscott · 2020-03-19T03:04:02Z

/retest

i ... someone should make a better one of these.

vdemeester · 2020-03-19T08:00:04Z

cc @afrittoli ^^
This tests has been flaky at some point, might be back to flaky with those changes

dibyom · 2020-03-19T15:02:54Z

I0319 03:32:23.673]   status:
I0319 03:32:23.673]     cloudEvents:
I0319 03:32:23.673]     - status:
I0319 03:32:23.673]         condition: Failed
I0319 03:32:23.673]         message: "1 error occurred:\n\t* Post http://sink.default:8080: EOF\n\n"
I0319 03:32:23.673]         retryCount: 1
I0319 03:32:23.673]         sentAt: "2020-03-19T03:21:54Z"
I0319 03:32:23.674]       target: http://sink.default:8080
I0319 03:32:23.674]     completionTime: "2020-03-19T03:21:45Z"

n3wscott · 2020-03-19T16:20:59Z

The sdk was not correctly setting the http request content length and was a real issue. Fixed with cloudevents/sdk-go#405

mattmoor · 2020-03-19T20:43:58Z

Thanks for putting this together @n3wscott 🎉

mattmoor · 2020-03-20T14:38:28Z

@vdemeester @dibyom @afrittoli What do we need to move this along? 😇

vdemeester

/meow

/cc @sbwsg @bobcatfish

tekton-robot · 2020-03-20T15:08:48Z

@vdemeester:

In response to this:

/meow

/cc @sbwsg @bobcatfish

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

afrittoli

Thanks for this!
My only question is whether we have less test coverage now.

afrittoli · 2020-03-20T14:59:51Z

pkg/reconciler/taskrun/resources/cloudevent/cloudevent_test.go

-	failingClientBehaviour     = FakeClientBehaviour{SendSuccessfully: false}
-)
-
-func TestSendCloudEvent(t *testing.T) {


Why is this test all removed?

There is no need to wrap using the client send function. Just use the client directly.

afrittoli · 2020-03-20T15:03:28Z

pkg/reconciler/taskrun/resources/cloudevent/cloud_event_controller.go

+	logger = logger.With(zap.String("taskrun", tr.Name))
+
+	// Make the event we would like to send:
+	event, err := EventForTaskRun(tr)


Nice, you separated creating the event from the delivery. Is this so we only build it once, in case of multiple deliveries?

yeah, the controller now uses the client directly, and all the custom stuff is doing is understanding how to take a TaskRun and turn it into an event.Event.

n3wscott · 2020-03-20T19:53:25Z

My only question is whether we have less test coverage now.

Well... I deleted a lot of code, and so the coverage is about the same.

afrittoli · 2020-03-20T20:05:48Z

My only question is whether we have less test coverage now.

Well... I deleted a lot of code, and so the coverage is about the same.

cool, thanks

/lgtm

mattmoor · 2020-03-20T21:26:41Z

🙏 tide

mattmoor · 2020-03-20T21:50:02Z

@afrittoli anything we can do to kick Prow here? Seems stuck.

afrittoli · 2020-03-20T22:14:21Z

/lgtm
/approve

afrittoli · 2020-03-20T22:39:36Z

@afrittoli anything we can do to kick Prow here? Seems stuck.

{"base-sha":"212e3d47ae76ef5c059006cbd2c242963a6ca1a9","branch":"master","component":"tide","controller":"sync","error":"failed merging [2243]: [PR is unmergable. Do the Tide merge requirements match the GitHub settings for the repo? This branch can't be rebased]","file":"prow/tide/tide.go:357","func":"k8s.io/test-infra/prow/tide.(*Controller).Sync.func2","level":"error","msg":"Error syncing subpool.","org":"tektoncd","repo":"pipeline","time":"2020-03-20T22:37:51Z"}

That sounds... wierd

afrittoli · 2020-03-20T22:43:18Z

@afrittoli anything we can do to kick Prow here? Seems stuck.

{"base-sha":"212e3d47ae76ef5c059006cbd2c242963a6ca1a9","branch":"master","component":"tide","controller":"sync","error":"failed merging [2243]: [PR is unmergable. Do the Tide merge requirements match the GitHub settings for the repo? This branch can't be rebased]","file":"prow/tide/tide.go:357","func":"k8s.io/test-infra/prow/tide.(*Controller).Sync.func2","level":"error","msg":"Error syncing subpool.","org":"tektoncd","repo":"pipeline","time":"2020-03-20T22:37:51Z"}

That sounds... wierd

@mattmoor @n3wscott The list of commits looks a bit unusual, there are 9 commits and at least on of them is a merge commit. Could you try and squash them into one or two commits, then I'll re-approve the patch? Thanks!

afrittoli · 2020-03-20T22:43:46Z

/hold

While I was here, I also refactored some of the CloudEvents integration, some major points: you now send spec version 1.0 formatted events ID is not the task run name, this was a bad idea if you want to send more than one event for that run. The task run name is now used as the subject. ID is now a UUID. The event that is created for sending is only created once, and then used for all deliveries. This means that all subscribers get the same event content and ID. The event payload is marshaled inside the Event object. I show examples in the code and tests of how to set and get the payload as your custom struct. Removed a lot of the layers passing clients and event components around. Better to build the event and then call send on the client directly. This deleted a lot of code that was doing many things, like create an event, and then attempt to send it. Signed-off-by: Scott Nichols <[email protected]>

n3wscott · 2020-03-20T23:03:16Z

ok smashed it all into one commit

/unhold

mattmoor · 2020-03-20T23:09:10Z

/hold cancel

mattmoor · 2020-03-20T23:13:24Z

@afrittoli this should be RFAL

n3wscott · 2020-03-20T23:47:35Z

looks like this failed:

 ERROR: test demo-pipeline-run-1=SucceededFalse but should be succeededtrue
I0320 23:33:53.708] ERROR: test demo-pipelinerun-with-resourcespec=SucceededFalse but should be succeededtrue
I0320 23:33:53.708]

/retest

afrittoli · 2020-03-21T08:49:28Z

/lgtm
/approve

tekton-robot · 2020-03-21T08:49:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester

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~~ [afrittoli,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

afrittoli · 2020-03-21T08:49:38Z

Thanks!

tekton-robot requested a review from abayer March 18, 2020 16:29

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 18, 2020

tekton-robot requested a review from bobcatfish March 18, 2020 16:29

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

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 18, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 18, 2020

n3wscott force-pushed the ce-preview4 branch from 5e51fc6 to a8d604b Compare March 18, 2020 17:13

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 18, 2020

n3wscott force-pushed the ce-preview4 branch from b0b2df7 to 959efe4 Compare March 19, 2020 00:14

vdemeester approved these changes Mar 20, 2020

View reviewed changes

tekton-robot requested a review from a user March 20, 2020 15:08

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

afrittoli reviewed Mar 20, 2020

View reviewed changes

tekton-robot assigned afrittoli Mar 20, 2020

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

afrittoli removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Mar 20, 2020

tekton-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2020

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 20, 2020

n3wscott force-pushed the ce-preview4 branch from 6d7d4bc to 4202b69 Compare March 20, 2020 23:02

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2020

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

tekton-robot merged commit fbde4aa into tektoncd:master Mar 21, 2020

afrittoli added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate to cloudevents sdk v2 #2243

migrate to cloudevents sdk v2 #2243

n3wscott commented Mar 18, 2020 •

edited

Loading

tekton-robot commented Mar 18, 2020

dibyom commented Mar 18, 2020

n3wscott commented Mar 18, 2020

mattmoor commented Mar 18, 2020

n3wscott commented Mar 18, 2020

n3wscott commented Mar 19, 2020

n3wscott commented Mar 19, 2020

vdemeester commented Mar 19, 2020

dibyom commented Mar 19, 2020

n3wscott commented Mar 19, 2020

mattmoor commented Mar 19, 2020

mattmoor commented Mar 20, 2020

vdemeester left a comment

tekton-robot commented Mar 20, 2020

afrittoli left a comment

afrittoli Mar 20, 2020

n3wscott Mar 20, 2020

afrittoli Mar 20, 2020

n3wscott Mar 20, 2020

n3wscott commented Mar 20, 2020

afrittoli commented Mar 20, 2020

mattmoor commented Mar 20, 2020

mattmoor commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

n3wscott commented Mar 20, 2020

mattmoor commented Mar 20, 2020

mattmoor commented Mar 20, 2020

n3wscott commented Mar 20, 2020

afrittoli commented Mar 21, 2020

tekton-robot commented Mar 21, 2020

afrittoli commented Mar 21, 2020

migrate to cloudevents sdk v2 #2243

migrate to cloudevents sdk v2 #2243

Conversation

n3wscott commented Mar 18, 2020 • edited Loading

tekton-robot commented Mar 18, 2020

dibyom commented Mar 18, 2020

n3wscott commented Mar 18, 2020

mattmoor commented Mar 18, 2020

n3wscott commented Mar 18, 2020

n3wscott commented Mar 19, 2020

n3wscott commented Mar 19, 2020

vdemeester commented Mar 19, 2020

dibyom commented Mar 19, 2020

n3wscott commented Mar 19, 2020

mattmoor commented Mar 19, 2020

mattmoor commented Mar 20, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 20, 2020

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Mar 20, 2020

Choose a reason for hiding this comment

n3wscott Mar 20, 2020

Choose a reason for hiding this comment

afrittoli Mar 20, 2020

Choose a reason for hiding this comment

n3wscott Mar 20, 2020

Choose a reason for hiding this comment

n3wscott commented Mar 20, 2020

afrittoli commented Mar 20, 2020

mattmoor commented Mar 20, 2020

mattmoor commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

afrittoli commented Mar 20, 2020

n3wscott commented Mar 20, 2020

mattmoor commented Mar 20, 2020

mattmoor commented Mar 20, 2020

n3wscott commented Mar 20, 2020

afrittoli commented Mar 21, 2020

tekton-robot commented Mar 21, 2020

afrittoli commented Mar 21, 2020

n3wscott commented Mar 18, 2020 •

edited

Loading