Migrate inmemorychannel to use v2 cloudevents bindings by nlopezgi · Pull Request #2813 · knative/eventing

nlopezgi · 2020-03-23T18:48:34Z

Signed-off-by: Nicolas Lopez ngiraldo@google.com

Fixes #2664

Proposed Changes

InMemoryChannel has a new message_dispatcher.go that uses the bindings API

ptal @slinkydeveloper

slinkydeveloper · 2020-03-23T18:52:04Z

/assign

n3wscott · 2020-03-23T18:59:38Z

This might help you: cloudevents/sdk-go#418

nlopezgi · 2020-03-23T19:25:17Z

This might help you: cloudevents/sdk-go#418

Thanks for the pointer!

nlopezgi · 2020-03-24T15:08:33Z

this is ready for review ptal @slinkydeveloper

slinkydeveloper · 2020-03-24T15:54:35Z

Cool! Now let's see if tests like it

nlopezgi · 2020-03-24T16:14:38Z

not sure why the unit tests failed, they pass locally on my env, will wait for integration tests to see if those pass or fail.

nlopezgi · 2020-03-24T16:45:25Z

Looks like integration tests failed as well. Will debug and ping back once I have them passing.

n3wscott · 2020-03-24T17:15:29Z

-	logger       *zap.Logger
+type InMemoryMessageDispatcher struct {
+	handler              *swappable.MessageHandler
+	httpBindingsReceiver *kncloudevents.HttpMessageReceiver


nice nice nice

n3wscott · 2020-03-24T17:18:37Z

try locally with --race it might be that something is assuming it can bind to a port and it is true if the tests are not run at the same time?

nlopezgi · 2020-03-24T23:37:37Z

Looks like the integration tests are not liking the new inmemorychannel message_dispatcher. I tried to debug but could not find anything off (particularly from comparing my changes vs the ones in channel/message_receiver.go vs channel/event_receiver.go that jumps out as the cause of the issues. Error logs point to the inmemyrchannel-controller being unable to properly create triggers.
Please let me know if anyone has ideas as to how to debug this.

wrt unit tests I dont know why they are failing either, the strangest bit is the test result does not match what I get in my local dev env. I also tried running tests with --race but did not get any different results locally.

slinkydeveloper · 2020-04-06T10:21:09Z

I'm trying to run the tests by myself today and check if i can find the problem hidden somewhere

slinkydeveloper · 2020-04-06T19:23:08Z

This should finally fix the tracing :) #2917

E2E tests: https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/2915/pull-knative-eventing-integration-tests/1247229335597748225

nlopezgi · 2020-04-06T23:00:44Z

This should be ready to go once #2917 is in

slinkydeveloper · 2020-04-07T13:05:09Z

/reopen

knative-prow-robot · 2020-04-07T13:05:11Z

@slinkydeveloper: Reopened this PR.

Details

In response to this:

/reopen

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.

slinkydeveloper · 2020-04-07T13:05:37Z

#2917 done!

knative-metrics-robot · 2020-04-07T13:09: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/inmemorychannel/message_dispatcher.go	Do not exist	18.8%

knative-prow-robot · 2020-04-07T13:09:54Z

@nlopezgi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`25f765c`	link	`/test pull-knative-eventing-go-coverage`

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.

nlopezgi · 2020-04-07T13:54:07Z

/unhold

nlopezgi · 2020-04-07T13:54:45Z

This is now ready for review @n3wscott @slinkydeveloper integration tests and unit tests are all passing.

slinkydeveloper · 2020-04-07T14:17:57Z

@nlopezgi you need to cover a little bit more pkg/inmemorychannel/message_dispatcher.go to make the coverage check happy

slinkydeveloper

/lgtm let's get this in

n3wscott · 2020-04-07T15:14:35Z

/lgtm
/approve

knative-prow-robot · 2020-04-07T15:14:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, nlopezgi, slinkydeveloper

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~~ [n3wscott]

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

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 23, 2020

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

mattmoor reviewed Mar 23, 2020

View reviewed changes

knative-prow-robot requested review from evankanderson and n3wscott March 23, 2020 18:48

knative-prow-robot assigned slinkydeveloper Mar 23, 2020

slinkydeveloper reviewed Mar 23, 2020

View reviewed changes

Comment thread pkg/kncloudevents/good_client.go Outdated

nlopezgi force-pushed the issue2664 branch from f544b85 to c419104 Compare March 23, 2020 19:29

nlopezgi changed the title ~~[WIP]: migrate inmemorychannel to use v2 cloudevents bindings~~ Migrate inmemorychannel to use v2 cloudevents bindings Mar 24, 2020

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 24, 2020

nlopezgi force-pushed the issue2664 branch from fe6149f to b73d082 Compare March 24, 2020 15:28

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 24, 2020

slinkydeveloper reviewed Mar 24, 2020

View reviewed changes

Comment thread pkg/inmemorychannel/message_dispatcher.go Outdated

n3wscott reviewed Mar 24, 2020

View reviewed changes

nlopezgi force-pushed the issue2664 branch 2 times, most recently from 6cf3b0f to f5f975a Compare March 24, 2020 23:36

vaikas reviewed Mar 25, 2020

View reviewed changes

Comment thread pkg/inmemorychannel/message_dispatcher.go Outdated

Comment thread pkg/inmemorychannel/message_dispatcher.go

Nicolas Lopez added 5 commits April 5, 2020 18:28

fix error message

8bb50cf

nit: change context->ctx

55e57f5

trying fix suggested by n3wscott

529933e

adding patch and create permissions to dispatcher

067b262

fixing nits

25f765c

nlopezgi force-pushed the issue2664 branch from 0923c4a to 25f765c Compare April 5, 2020 22:28

This was referenced Apr 6, 2020

[WIP] Testing imc conversion to bindings #2915

Closed

Tracing fix for MessageDispatcher #2917

Merged

knative-prow-robot closed this in #2917 Apr 7, 2020

knative-prow-robot reopened this Apr 7, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2020

slinkydeveloper approved these changes Apr 7, 2020

View reviewed changes

knative-prow-robot assigned n3wscott Apr 7, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020

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

knative-prow-robot merged commit ca7609c into knative:master Apr 7, 2020

slinkydeveloper mentioned this pull request May 6, 2020

Added slinkydeveloper as approver #3091

Merged

Conversation

nlopezgi commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

slinkydeveloper commented Mar 23, 2020

Uh oh!

Uh oh!

n3wscott commented Mar 23, 2020

Uh oh!

nlopezgi commented Mar 23, 2020

Uh oh!

nlopezgi commented Mar 24, 2020

Uh oh!

slinkydeveloper commented Mar 24, 2020

Uh oh!

Uh oh!

nlopezgi commented Mar 24, 2020

Uh oh!

nlopezgi commented Mar 24, 2020

Uh oh!

n3wscott Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Mar 24, 2020

Uh oh!

nlopezgi commented Mar 24, 2020

Uh oh!

Uh oh!

Uh oh!

slinkydeveloper commented Apr 6, 2020

Uh oh!

slinkydeveloper commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlopezgi commented Apr 6, 2020

Uh oh!

slinkydeveloper commented Apr 7, 2020

Uh oh!

knative-prow-robot commented Apr 7, 2020

Uh oh!

slinkydeveloper commented Apr 7, 2020

Uh oh!

knative-metrics-robot commented Apr 7, 2020

Uh oh!

knative-prow-robot commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlopezgi commented Apr 7, 2020

Uh oh!

nlopezgi commented Apr 7, 2020

Uh oh!

slinkydeveloper commented Apr 7, 2020

Uh oh!

slinkydeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Apr 7, 2020

Uh oh!

knative-prow-robot commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nlopezgi commented Mar 23, 2020 •

edited

Loading

slinkydeveloper commented Apr 6, 2020 •

edited

Loading

knative-prow-robot commented Apr 7, 2020 •

edited

Loading