Upgrading to CloudEvents 1.0 by matzew · Pull Request #2157 · knative/eventing

matzew · 2019-11-07T11:10:53Z

Proposed Changes

going 1.0

Release Note

usage of CloudEvents 1.0 in Knative Eventing

matzew · 2019-11-07T11:11:08Z

/assign @nachocano

mind taking a look, dude ?

knative-prow-robot · 2019-11-07T11:11:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew]

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

knative-metrics-robot · 2019-11-07T13:02:42Z

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

File	Old Coverage	New Coverage	Delta
pkg/channel/fanout/fanout_handler.go	84.6%	76.9%	-7.7

matzew · 2019-11-07T13:42:26Z

hrm... integratation test ...

vaikas · 2019-11-07T23:11:58Z


 const (
-	// V03TTLAttribute is the name of the CloudEvents 0.3 extension attribute used to store the
+	// V1TTLAttribute is the name of the CloudEvents 0.3 extension attribute used to store the


nit: 0.3 -> 1.0?

vaikas · 2019-11-07T23:13:12Z

 	event.SetSource("/mycontext")
 	event.SetID("A234-1234-1234")
-	event.SetExtension("comExampleExtension", "value")
+	event.SetExtension("comexampleextension", "value")


just jotting this down here, but 0.3 used to default encoding, and now you have to manually set it, might want to add that in here as well?

vaikas · 2019-11-07T23:13:58Z

-	event := cloudevents.NewEvent(cloudevents.VersionV02)
+	event := cloudevents.NewEvent(cloudevents.VersionV1)
 	event.SetID(uuid.New().String())
 	event.SetType(common.GCEventType)


These should also set the encoding?

vaikas · 2019-11-07T23:15:43Z

I do wonder if these are failing because the encoding is not set?

nachocano · 2019-11-08T02:28:15Z

sorry for the delay @matzew ... thanks for doing this!
Couple of questions:

are we always outputting V1 events now? If that is the case, we will need to update the examples in knative/docs as a follow up. Or we are outputting the same input version?
can you check that sending in v0.3 still works? maybe from some of the existing eventing-contrib sources?

matzew · 2019-11-08T02:31:18Z

I was going to update the docs, yes!

and let me test w/ 0.3

thanks for the pointers, @nachocano ! 🤗

devguyio · 2019-11-08T13:57:44Z

I would recommend that we deprecate 0.3 properly, so make sure both work, then deprecate 0.3 and then remove it.

matzew · 2019-11-08T16:55:48Z

makes sense! I will update it next week!

On Fri 8. Nov 2019 at 14:57, Ahmed Abdalla ***@***.***> wrote: I would recommend that we deprecate 0.3 properly, so make sure both work, then deprecate 0.3 and then remove it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2157?email_source=notifications&email_token=AABGPTX4BVBBA4A6QCYJIK3QSVV5RA5CNFSM4JKE75N2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDSFPVI#issuecomment-551835605>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTULP7K6G7CDUHIJF6LQSVV5RANCNFSM4JKE75NQ> .

-- Sent from Gmail Mobile

akashrv · 2019-11-25T18:54:54Z

-	delete(originalV3.Extensions, ttlKey)
-	event.Context = originalV3
+	delete(originalV1.Extensions, ttlKey)
+	event.Context = originalV1


Is there are reason why we bump all events to V1? Can't broker/trigger be just pass through?

n3wscott · 2019-11-27T15:44:46Z

/retest

knative-prow-robot · 2019-11-27T16:09:32Z

@matzew: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-eventing-integration-tests	`b9206a5`	link	`/test pull-knative-eventing-integration-tests`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

akashrv · 2019-12-03T00:39:20Z


 	// Remove the TTL attribute that is used by the Broker.
-	originalV3 := event.Context.AsV03()
+	originalV1 := event.Context.AsV1()


Please revert this file as this has been now fixed correctly.
/hold

akashrv · 2019-12-03T00:40:23Z


 const (
-	// V03TTLAttribute is the name of the CloudEvents 0.3 extension attribute used to store the
+	// V1TTLAttribute is the name of the CloudEvents 1.0 extension attribute used to store the


Revert this file too. This has been now fixed correctly. refer to #2244

matzew · 2019-12-10T17:27:55Z

/close

knative-prow-robot · 2019-12-10T17:27:58Z

@matzew: Closed this PR.

Details

In response to this:

/close

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.

matzew · 2019-12-10T17:29:29Z

done via:

Upgrading to CloudEvents 1.0

ec57225

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 7, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 7, 2019

knative-prow-robot requested review from Harwayne and adrcunha November 7, 2019 11:11

knative-prow-robot assigned nachocano Nov 7, 2019

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Nov 7, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2019

move one moar

be3ca23

matzew mentioned this pull request Nov 7, 2019

[WIP] update some of eventing to use 1.0 #2062

Closed

vaikas reviewed Nov 7, 2019

View reviewed changes

some more V1 usage

b9206a5

adrcunha removed their request for review November 15, 2019 20:08

akashrv reviewed Nov 25, 2019

View reviewed changes

akashrv reviewed Dec 3, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019

knative-prow-robot closed this Dec 10, 2019

Conversation

matzew commented Nov 7, 2019

Proposed Changes

Uh oh!

matzew commented Nov 7, 2019

Uh oh!

knative-prow-robot commented Nov 7, 2019

Uh oh!

knative-metrics-robot commented Nov 7, 2019

Uh oh!

matzew commented Nov 7, 2019

Uh oh!

vaikas Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas commented Nov 7, 2019

Uh oh!

nachocano commented Nov 8, 2019

Uh oh!

matzew commented Nov 8, 2019

Uh oh!

devguyio commented Nov 8, 2019

Uh oh!

matzew commented Nov 8, 2019 via email

Uh oh!

akashrv Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Nov 27, 2019

Uh oh!

knative-prow-robot commented Nov 27, 2019

Uh oh!

akashrv Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

akashrv Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

matzew commented Dec 10, 2019

Uh oh!

knative-prow-robot commented Dec 10, 2019

Uh oh!

matzew commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants