Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update NATS ClusterChannelProvisioner to NATS CRD #5517

Merged
merged 40 commits into from
Sep 17, 2019
Merged

Update NATS ClusterChannelProvisioner to NATS CRD #5517

merged 40 commits into from
Sep 17, 2019

Conversation

anishj0shi
Copy link
Member

@anishj0shi anishj0shi commented Sep 2, 2019 •
edited
Loading

Description

Changes proposed in this pull request:

  • Update Kubernetes dependencies to 1.12.9
  • Update knative deps to v0.8
  • Change knavtive channels.eventing refs to channel.messaging.
  • knative Channel and subscription Creation is generic based on knative config-map
  • change Get knative channel strategy from labels
  • Fix Unit Tests

Related issue(s)

@anishj0shi anishj0shi self-assigned this Sep 2, 2019
@anishj0shi anishj0shi added area/eventing Issues or PRs related to eventing WIP labels Sep 2, 2019
@anishj0shi anishj0shi mentioned this pull request Sep 9, 2019
Merged
@anishj0shi anishj0shi marked this pull request as ready for review September 12, 2019 09:08
sayanh
sayanh suggested changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@sayanh sayanh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@anishj0shi On another note, I just found this

kyma/components/event-bus/cmd/event-publish-service/handlers/publish.go

Lines 160 to 161 in 4ae04fa

channelName := eventBusUtil.GetChannelName(&publishRequest.SourceID, &publishRequest.EventType,
&publishRequest.EventTypeVersion)

the publish service gets the channel name using the same algorithm instead of hitting the apiserver. Almost overlooked this. Querying with labels won't be enough.

Edit: Not true, it indeed hits the api server and gets the channel URL.

sayanh
sayanh reviewed Sep 16, 2019
View reviewed changes
components/event-bus/cmd/event-publish-service/publisher/publisher.go Outdated
payload *[]byte) (*api.Error, string, string) {

// knative Channel reference should not be nil
if channel == nil {
Copy link
Contributor

@sayanh sayanh Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

channel will never be nil as in GetChannelByLabels(), error is thrown if no channels are found

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, it was designed to be a public method. Now I've removed this check as the method is now private

montaro
montaro approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@montaro montaro left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the good work!

sayanh
sayanh approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@sayanh sayanh left a comment

Choose a reason for hiding this comment

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

lgtm

k15r
k15r reviewed Sep 17, 2019
View reviewed changes
components/event-bus/internal/knative/util/kn-eventing.go Outdated
evclientset "github.com/knative/eventing/pkg/client/clientset/versioned"
eventingv1alpha1 "github.com/knative/eventing/pkg/client/clientset/versioned/typed/eventing/v1alpha1"
messagingv1alpha1 "github.com/knative/eventing/pkg/client/clientset/versioned/typed/messaging/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

could we rename this? it is kinda confusing if you compare it to line 15

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@anishj0shi anishj0shi merged commit c7ed676 into kyma-project:master Sep 17, 2019
anishj0shi pushed a commit that referenced this pull request Sep 23, 2019
@montaro @anishj0shi
Upgrade knative eventing chart to 0.8.0 (#5526)
dfe9433 
- Upgrade knative eventing chart to 0.8.0

- Pre and Post Upgrade hooks

- Working NATS Provisioner

- remove old NATS deployment

- Bump latest image from #5517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@montaro montaro montaro approved these changes

@k15r k15r k15r approved these changes

@sayanh sayanh sayanh approved these changes

@abbi-gaurav abbi-gaurav Awaiting requested review from abbi-gaurav

@devguyio devguyio Awaiting requested review from devguyio

@antoineco antoineco Awaiting requested review from antoineco

@joek joek Awaiting requested review from joek

@lilitgh lilitgh Awaiting requested review from lilitgh

@marcobebway marcobebway Awaiting requested review from marcobebway

@nachtmaar nachtmaar Awaiting requested review from nachtmaar

@rakesh-garimella rakesh-garimella Awaiting requested review from rakesh-garimella

@venturasr venturasr Awaiting requested review from venturasr

Assignees

@montaro montaro

@anishj0shi anishj0shi

Labels
area/eventing Issues or PRs related to eventing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anishj0shi @k15r @montaro @sayanh