Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Updated MessageDispatcher usage #1082

Merged
merged 42 commits into from
Apr 2, 2020
Merged

Updated MessageDispatcher usage #1082

merged 42 commits into from
Apr 2, 2020

Conversation

slinkydeveloper
Copy link
Contributor

Updates accordingly to knative/eventing#2847

Signed-off-by: Francesco Guardiani [email protected]

@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 26, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 26, 2020
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from 913e000 to 2b3f744 Compare March 26, 2020 17:09
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from 2b3f744 to b51d05e Compare March 30, 2020 06:52
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from b51d05e to 66a2c33 Compare March 30, 2020 16:04
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2020
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from b738ba6 to 6784a9c Compare March 31, 2020 07:32
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2020
@slinkydeveloper slinkydeveloper mentioned this pull request Apr 1, 2020
Closed
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from 6bdab93 to 82fe75a Compare April 1, 2020 09:30
@slinkydeveloper
Copy link
Contributor Author

Something is broken in nats dependency tree, i'm trying to find the root cause

@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented Apr 1, 2020 •
edited
Loading

Ok I've found the problem: the version of github.com/nats-io/nats-streaming-server (used only one time here in unit tests) clashes with the version of stan brought in by the latest cloudevents/sdk-go version. nats-streaming-server transitively depends on raft, same for sdk-go, but on two different versions: 1.1.1 and 1.1.2. Between these two there is a breaking change that breaks nats-streaming-server. I can't update nats-streaming-server because it depends on nats server 2 which uses go mods and go mod convention v2, which dep doesn't understand (golang/dep#1962 (comment)).

So i'm going to comment this test and replace with a dummy one to unblock this PR

@pierDipi
Copy link
Member

pierDipi commented Apr 1, 2020

@slinkydeveloper rebase master, stanutil_test.go has been already removed. https://github.com/knative/eventing-contrib/tree/master/natss/pkg/stanutil

@slinkydeveloper
Copy link
Contributor Author

@pierDipi already rebased, but the go coverage blames me that i don't cover the stanutil... Anyway I've added a dummy test, let's see if this does the job

@slinkydeveloper
Copy link
Contributor Author

/retest pull-knative-eventing-contrib-integration-tests

@slinkydeveloper
Fixed DispatchMessage invocation
32018bb 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Reverted the mess
73b6c21 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
error
ae228e8 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Reenabled all e2e-tests
dcd204a 
Added panic check

Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Reverted the mess in e2e tests
79af29d 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper slinkydeveloper force-pushed the update_message_dispatcher branch from 7dbf9a1 to 79af29d Compare April 2, 2020 10:42
@slinkydeveloper
Reenabled cluster wise logs
850964f 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Copy link
Contributor Author

Applying this patch knative/eventing#2897 fixes everything except the TestBrokerChannelFlow/TestBrokerChannelFlow-{NatssChannel_messaging.knative.dev/v1alpha1}

@slinkydeveloper
Added some panic info
8aa94a0 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Tried to fix the dispatcher
2315bec 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
run only wrong test
2cdefc4 
Signed-off-by: Francesco Guardiani <[email protected]>
@matzew
Copy link
Member

matzew commented Apr 2, 2020 •
edited
Loading

@slinkydeveloper looking here: https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing-contrib/1082/pull-knative-eventing-contrib-build-tests/1245702147354398721

it says

run hack/update-codegen.sh

did you do that ?

@slinkydeveloper
Added it test for natss, skipped broker_channel_flow_test.go
8c46fe3 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Updates
d4e7219 
Signed-off-by: Francesco Guardiani <[email protected]>
n3wscott
n3wscott reviewed Apr 2, 2020
View reviewed changes
couchdb/source/pkg/apis/addtoscheme_eventing_v1alpha1.go Outdated
eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1"
)

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file was deleted

https://github.com/knative/eventing-contrib/pull/1036/files#diff-ef78e92db3fcd3193635e8fc2dd07f1b

@slinkydeveloper
aaa
c952b44 
Signed-off-by: Francesco Guardiani <[email protected]>
n3wscott
n3wscott reviewed Apr 2, 2020
View reviewed changes
github/pkg/apis/addtoscheme_eventing_v1alpha1.go Outdated
eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1"
)

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was deleted

@slinkydeveloper
Removed stuff
6e3bcb3 
Signed-off-by: Francesco Guardiani <[email protected]>
n3wscott
n3wscott reviewed Apr 2, 2020
View reviewed changes
test/e2e-tests.sh Outdated
@@ -218,7 +243,7 @@ initialize $@ --skip-istio-addon

# TODO: Figure out why kafka channels do not like parallel=12 (which it was)
# https://github.com/knative/eventing-contrib/issues/917
go_test_e2e -timeout=20m -parallel=1 ./test/e2e -channels=messaging.knative.dev/v1alpha1:NatssChannel,messaging.knative.dev/v1alpha1:KafkaChannel || fail_test
go_test_e2e -timeout=20m -parallel=1 ./test/e2e -run=TestBrokerChannelFlow -channels=messaging.knative.dev/v1alpha1:NatssChannel || fail_test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to get reverted before merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@slinkydeveloper
e2e test script correct
22acb0e 
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Removed dead code
b869e8a 
Signed-off-by: Francesco Guardiani <[email protected]>
n3wscott
n3wscott reviewed Apr 2, 2020
View reviewed changes
n3wscott
n3wscott reviewed Apr 2, 2020
View reviewed changes
test/e2e/broker_channel_flow_test.go
@@ -24,5 +24,6 @@ import (
)

func TestBrokerChannelFlow(t *testing.T) {
t.Skip("NATSSChannel broken")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix NATSS channel tests for cloudevents Preview8.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is to make an issue)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1102

@slinkydeveloper
error
e04a05e 
Signed-off-by: Francesco Guardiani <[email protected]>
@n3wscott
Copy link
Contributor

n3wscott commented Apr 2, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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
Copy link

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

File Old Coverage New Coverage Delta
kafka/channel/pkg/dispatcher/dispatcher.go 61.1% 58.9% -2.2
kafka/common/pkg/kafka/consumer_handler.go 88.2% 88.9% 0.7
natss/pkg/stanutil/stanutil.go Do not exist 71.4%

@knative-prow-robot knative-prow-robot merged commit 98cfa38 into knative:master Apr 2, 2020
@pierDipi pierDipi mentioned this pull request Apr 3, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattmoor mattmoor mattmoor left review comments

@n3wscott n3wscott n3wscott left review comments

@devguyio devguyio Awaiting requested review from devguyio

@lberk lberk Awaiting requested review from lberk

@matzew matzew Awaiting requested review from matzew

Assignees

@n3wscott n3wscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@slinkydeveloper @pierDipi @n3wscott @matzew @knative-prow-robot @knative-metrics-robot @mattmoor @googlebot